Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

switch out grpc for @grpc/grpc-js + ts types #423

Merged
merged 2 commits into from
Feb 18, 2023

Conversation

antonilol
Copy link
Contributor

@antonilol antonilol commented Sep 8, 2022

fixes #295
fixes #425

i managed to get @grpc/grpc-js working with camel_case fields, but still some tests are failing

also added generated typescript types for the grpc calls

all files in src/grpc/types/ are generated with the ./grpc_gen_types.sh bash script, which also runs in gh workflows after updating the proto files

@antonilol
Copy link
Contributor Author

what does failed to getContactAndCheckKeyExchange actually mean? i see it in most of the failed tests. a more descriptive error would help

@Evanfeenstra
Copy link
Contributor

When a contact is added, you send a keysend with your contact_key (end-to-end encryption key)... then they send a keysend back with their contact_key

@antonilol
Copy link
Contributor Author

When a contact is added, you send a keysend with your contact_key (end-to-end encryption key)... then they send a keysend back with their contact_key

ok got it, i think it didnt got a key back from the other party then...

@Evanfeenstra
Copy link
Contributor

@antonilol #425

@antonilol antonilol changed the title switch out grpc for @grpc/grpc-js + generated ts types switch out grpc for @grpc/grpc-js ~+ generated ts types~ Sep 15, 2022
@antonilol antonilol changed the title switch out grpc for @grpc/grpc-js ~+ generated ts types~ switch out grpc for @grpc/grpc-js + ~generated ts types~ Sep 15, 2022
@antonilol antonilol changed the title switch out grpc for @grpc/grpc-js + ~generated ts types~ switch out grpc for @grpc/grpc-js Sep 15, 2022
@antonilol
Copy link
Contributor Author

@Evanfeenstra can you fix package-lock.json on master? it contains merge conflict markers...

@Evanfeenstra
Copy link
Contributor

@Evanfeenstra can you fix package-lock.json on master? it contains merge conflict markers...

ok @antonilol its fixed

@Evanfeenstra
Copy link
Contributor

@antonilol
Copy link
Contributor Author

lets try

@Evanfeenstra
Copy link
Contributor

lets try

that will not fix the errors tho, the stack tests are not build around proxy. I would recommend cloning sphinx-stack and building sphinx-relay locally, then using your local image in sphinx-stack. Then you can see all the logs and what is going on (with Docker Desktop is easiest) @antonilol

@antonilol
Copy link
Contributor Author

lets try

that will not fix the errors tho, the stack tests are not build around proxy. I would recommend cloning sphinx-stack and building sphinx-relay locally, then using your local image in sphinx-stack. Then you can see all the logs and what is going on (with Docker Desktop is easiest) @antonilol

yeah i wanted to do this but stack comes with all types of random errors, like the one i described here stakwork/sphinx-stack#45 (i managed to fix this by running docker-compose up -d en then docker-compose down, and after that the no alice config seemed to work)
now i am getting an endless spam of => try http://host.docker.internal:3001/is_setup in docker logs -f sphinx-stack-relaysetup-1 (i believe port 3001 is from alice.sphinx but there is no alice...)
and bob and carol report this:

(node:1) DeprecationWarning: grpc.load: Use the @grpc/proto-loader module with grpc.loadPackageDefinition instead
[info] 09-15-22T18:49:53 [MISC] Node listening on 3002.
=> initGrpcSubscriptions error Error: 12 UNIMPLEMENTED: unknown service lnrpc.Lightning
    at Object.exports.createStatusError (/relay/node_modules/grpc/src/common.js:91:15)
    at Object.onReceiveStatus (/relay/node_modules/grpc/src/client_interceptors.js:1209:28)
    at InterceptingListener._callNext (/relay/node_modules/grpc/src/client_interceptors.js:568:42)
    at InterceptingListener.onReceiveStatus (/relay/node_modules/grpc/src/client_interceptors.js:618:8)
    at callback (/relay/node_modules/grpc/src/client_interceptors.js:847:24) {
  code: 12,
  metadata: Metadata { _internal_repr: {}, flags: 0 },
  details: 'unknown service lnrpc.Lightning'
}

@antonilol
Copy link
Contributor Author

i got stack working by running just docker-compose up -d, waiting till everything is running and then stopping alice.sphinx, then running sphinx relay and at least i got one test to pass on my machine (queryroutes)

@antonilol
Copy link
Contributor Author

antonilol commented Sep 15, 2022

i got some more tests working now locally with only the bare minimum to get grpcjs working, without some suggested type checks

@antonilol
Copy link
Contributor Author

i got all tests passing locally!!!
i squashed all commits and now waiting for the gh ci tests to pass

@antonilol antonilol changed the title switch out grpc for @grpc/grpc-js switch out grpc for @grpc/grpc-js + ts types Sep 15, 2022
@Evanfeenstra
Copy link
Contributor

i got all tests passing locally!!! i squashed all commits and now waiting for the gh ci tests to pass

what was the issue that was stopping key exchange?

@antonilol
Copy link
Contributor Author

antonilol commented Sep 15, 2022

i got all tests passing locally!!! i squashed all commits and now waiting for the gh ci tests to pass

what was the issue that was stopping key exchange?

there were 3 issues:

  1. keysend didnt work because grpcjs thought a bytebuffer always had a length of 0, i replaced it with a nodejs native buffer
  2. map<uint64, something> in proto will cause grpcjs to hash the uint64 before giving it to the client, resulting in the keys of the dest_custom_records of an InvoiceHTLC being hashed, then no key 133773310 existed anymore (subscribeInvoices line 27)
  3. lightning[cmd] did not have any arguments (the first one is undefined then and it gave an error). (subscribeInvoices line 24)

@antonilol antonilol marked this pull request as ready for review September 15, 2022 22:53
@Evanfeenstra
Copy link
Contributor

@antonilol so this file https://github.com/stakwork/sphinx-relay/blob/97ea3a3a34e42d5102c9396760ba706a0206d176/protobuf_long.patch is needed in order to make the map<uint64, something> work? Do we need to do anything specific to make it work? I'm not familiar with npm patches

@antonilol antonilol force-pushed the grpcjs branch 2 times, most recently from 71c7dd8 to 691cd60 Compare October 4, 2022 21:14
@kevkevinpal
Copy link
Contributor

Hey @antonilol were you able to manually test this change I pulled in the changes and I was only able to send out messages, receive wasn't working

do you know if this would fail if you're running nodejs 12 or is it required it be updated?

@antonilol
Copy link
Contributor Author

Hey @antonilol were you able to manually test this change I pulled in the changes and I was only able to send out messages, receive wasn't working

do you know if this would fail if you're running nodejs 12 or is it required it be updated?

i use node v16, and it works. with node v12.22.12 i cant npm install because this protobuf patch doesnt apply. if i just start it on node v12 i get an error that mcrypt was compiled for a newer version of nodejs

@antonilol
Copy link
Contributor Author

rebased

@kevkevinpal
Copy link
Contributor

Ok was able to get this running on nodejs 16 and it seems to be working fine with manual testing, just need to remember to update things from node 12 to 16 now

Copy link
Contributor

@kevkevinpal kevkevinpal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like linter is failing but otherwise I am good with these changes

@antonilol
Copy link
Contributor Author

antonilol commented Oct 19, 2022

Looks like linter is failing

this is fixed in #445, eslint misses some files and previous changes added warnings, can you merge it before this one?

@kevkevinpal
Copy link
Contributor

Looks like linter is failing

this is fixed in #445, eslint misses some files and previous changes added warnings, can you merge it before this one?

merged

@antonilol antonilol force-pushed the grpcjs branch 2 times, most recently from 791cae2 to bce6caa Compare October 20, 2022 10:11
@antonilol
Copy link
Contributor Author

rebased, got the warning count right and lint check passed now

@antonilol
Copy link
Contributor Author

all tests passed except one port in use error (stakwork/sphinx-stack#47)

@kevkevinpal
Copy link
Contributor

all tests passed except one port in use error (stakwork/sphinx-stack#47)

test are passing now 🥳

@antonilol
Copy link
Contributor Author

rebased and ready to merge. did you manage to upgrade the lite nodes to nodejs v16?

@kevkevinpal
Copy link
Contributor

rebased and ready to merge. did you manage to upgrade the lite nodes to nodejs v16?

Sorry seeing this now, I'm trying to upgrade the nodes on our end just had to add more storage since the clusters we're getting full after trying to reinstall the node_modules. I have a script to auto update it just might take a long time update, I'm getting stuck on this node_modules when installing [##################] / reify:macaroon: timing reifyNode:node_modules/typescript not sure if you know why this in one in particular takes so long

@antonilol
Copy link
Contributor Author

I'm getting stuck on this node_modules when installing [##################] / reify:macaroon: timing reifyNode:node_modules/typescript not sure if you know why this in one in particular takes so long

On my devices it also takes more time than other things done during npm install, on my bitcoin node it was significantly slower than on my pc and laptop, i think it is some cpu bound task, like compiling

@antonilol
Copy link
Contributor Author

rebased this and #362

@kevkevinpal
Copy link
Contributor

rebased this and #362

thanks for rebasing I'm still in the process updating the clusters just ran into some issues and it takes a while to update each one so I'm doing them at night

Thanks for your patience

notes:
 * grpcjs doesnt like ByteBuffer, it has been replaced with the nodejs
   native Buffer
 * grpcjs doesnt like calls without an argument, add {} instead
   see src/grpc/subscribe.ts:24
 * grpcjs converts uint64 in a map key (map<uint64, ...>) to some hash,
   this is fixed with protobuf_long.patch which is automatically applied
   after running `npm install`
   this pr would fix the bug upstream protobufjs/protobuf.js#1669
@Evanfeenstra Evanfeenstra merged commit 3b35d23 into stakwork:master Feb 18, 2023
@antonilol antonilol deleted the grpcjs branch April 6, 2023 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@grpc/grpc-js $@500000 build fails on node.js v17 due to depreciated grpc
3 participants