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.

Sign up for GitHub

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

Jump to bottom

rpc: update protoc to 3.6.0 #1467

Closed
wants to merge 1 commit into from
Closed

rpc: update protoc to 3.6.0 #1467

wants to merge 1 commit into from

Conversation

maurycy
Copy link
Contributor

@maurycy maurycy commented Jun 28, 2018 •
edited
Loading 

[4:11:12] i:maurycy:~> protoc --version
libprotoc 3.6.0

ref #1362

@Roasbeef Roasbeef added this to the 0.5 milestone Jun 28, 2018
@Roasbeef
Copy link
Member

The gRPC release looks to be pinned against 1.8.2. Why not just advance to the latest, so 1.13.0?

@maurycy
Copy link
Contributor Author

maurycy commented Jun 28, 2018 •
edited
Loading

@Roasbeef

Good catch! I've updated the google.golang.org/grpc revision to 168a6198bcb0ef175f7dacec0b8691fc141dc9b8 . I use v1.8.2 in another project, hence the mistake.

wpaulino
wpaulino suggested changes Jun 28, 2018
View reviewed changes
lnrpc/rpc.swagger.json Outdated
@@ -17,7 +17,8 @@
"paths": {
"/v1/balance/blockchain": {
"get": {
"summary": "* lncli: `walletbalance`\nWalletBalance returns total unspent outputs(confirmed and unconfirmed), all\nconfirmed unspent outputs and all unconfirmed unspent outputs under control\nof the wallet.",
"summary": "*\nGenSeed is the first method that should be used to instantiate a new lnd\ninstance. This method allows a caller to generate a new aezeed cipher seed\ngiven an optional passphrase. If provided, the passphrase will be necessary\nto decrypt the cipherseed to expose the internal wallet seed.",
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look right. The description for WalletBalance is being overwritten with GenSeed's.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wpaulino

Thank you for catching this. Investigating. It seems that the root cause is grpc-ecosystem/grpc-gateway#688

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, need to figure out why this is happening...

Copy link
Contributor Author

@maurycy maurycy Jul 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@halseth

The easiest solution is to use the latest grpc-gateway v1.4.1.

lnrpc/rpc.swagger.json Outdated
@@ -34,7 +35,8 @@
},
"/v1/balance/channels": {
"get": {
"summary": "* lncli: `channelbalance`\nChannelBalance returns the total funds available across all open channels\nin satoshis.",
"summary": "* \nInitWallet is used when lnd is starting up for the first time to fully\ninitialize the daemon and its internal wallet. At the very least a wallet\npassword must be provided. This will be used to encrypt sensitive material\non disk.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here w.r.t. the description being overwritten. There are a couple more cases below.

@ysangkok
Copy link

ysangkok commented Jul 3, 2018

why not upgrade to v3.6.0.1?

@Roasbeef Roasbeef added P3 might get fixed, nice to have rpc Related to the RPC interface protos P2 should be fixed if one has time incomplete WIP PR, not fully finalized, but light review possible and removed P3 might get fixed, nice to have labels Jul 11, 2018
@Roasbeef Roasbeef mentioned this pull request Jul 23, 2018
Closed
@maurycy maurycy changed the title rpc: update protoc to 3.5.1 rpc: update protoc to 3.6.0 Jul 23, 2018
@maurycy
Copy link
Contributor Author

maurycy commented Jul 23, 2018

@ysangkok Done.

This was referenced Jul 24, 2018
Closed
Closed
@maurycy
Copy link
Contributor Author

maurycy commented Jul 26, 2018

@wpaulino @halseth @Roasbeef Could you take a look now? The diff looks fine to me. The solution to the swagger issue is to use the latest grpc-gateway (v1.4.1). I've squashed all the commits into one.

@halseth halseth added the needs testing PR hasn't yet been actively tested on testnet/mainnet label Jul 26, 2018
@Roasbeef
Copy link
Member

Roasbeef commented Aug 9, 2018

Needs rebase!

@maurycy
Copy link
Contributor Author

maurycy commented Aug 9, 2018

@Roasbeef

Done. ;-)

By the way, I'm rebasing this PR every few days.

@halseth halseth mentioned this pull request Aug 9, 2018
Closed
@Roasbeef
Copy link
Member

Looks like maybe some gRPC behavior that we depend on was modified?

    --- FAIL: TestLightningNetworkDaemon/single-hop_send_to_route (0.46s)
    	lnd_test.go:74: Failed: (single-hop send to route): exited with error: 
    		*errors.errorString unable to send payment: rpc error: code = Internal desc = transport: transport: the stream is done or WriteHeader was already called
    		/home/travis/gopath/src/github.com/lightningnetwork/lnd/lnd_test.go:3223 (0xc3b490)
    			testSingleHopSendToRoute: t.Fatalf("unable to send payment: %v", err)

@Roasbeef
Copy link
Member

Yeah I get that consistently now.

@Roasbeef Roasbeef modified the milestones: 0.5, 0.5.1 Aug 15, 2018
@halseth halseth modified the milestones: 0.5.1, 0.5.2 Sep 20, 2018
@Roasbeef
Copy link
Member

#2055 has been merged which upgraded gRPC and also fixed the issue in the RPC implementation for SendPayment that caused the flake in the integration tests.

@ysangkok
Copy link

@maurycy It is not important to me, but I just wanted to note that I mentioned v3.6.0.1, not v3.6.0. Furthermore, v3.6.1 has now been released. But again, it is not important to me. Just wanted to let you know because I found it puzzling how you chose v3.6.0 and not v3.6.0.1, even after v3.6.0.1 had been released. At the time of my note, v3.6.1 was not released yet.

@Roasbeef Roasbeef removed this from the 0.5.2 milestone Jan 16, 2019
@Roasbeef Roasbeef closed this Apr 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@halseth halseth halseth left review comments

@wpaulino wpaulino wpaulino requested changes

Assignees
No one assigned
Labels
incomplete WIP PR, not fully finalized, but light review possible needs testing PR hasn't yet been actively tested on testnet/mainnet P2 should be fixed if one has time protos rpc Related to the RPC interface
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@maurycy @Roasbeef @ysangkok @halseth @wpaulino