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

Disable option_dataloss_protect #2155

Merged
merged 6 commits into from Dec 10, 2018

Conversation

Projects
None yet
3 participants
@rustyrussell
Copy link
Contributor

rustyrussell commented Dec 10, 2018

My test node has started getting disconnects from lnd nodes on reconnection. I think it's our fault, but I had a great deal of trouble with this code the first time, so I'm disabling it now to avoid going down that rabbit hole again.

The option is subsumed by option_simplified_commitment anyway, which is coming in Two Weeks(TM)

rustyrussell added some commits Dec 10, 2018

configure: don't turn on EXPERIMENTAL_FEATURES just because DEVELOPER.
It's about to become even more bleeding-edge.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
disconnect: add force option to disconnect even with a live channel.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
channeld: get local peer features from lightningd.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
pytest: add trivial test to make sure reconnect works with no channel…
… activity.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
channeld: don't assume we offered option_data_loss_protect.
Check it was negotiated.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

@rustyrussell rustyrussell requested a review from cdecker as a code owner Dec 10, 2018

@rustyrussell rustyrussell added this to the v0.6.3 milestone Dec 10, 2018

@rustyrussell rustyrussell changed the title Disalbe option_dataloss_protect Disable option_dataloss_protect Dec 10, 2018

channeld: only enable option_data_loss_protect if EXPERIMENTAL_FEATURES.
We have an incompatibility with lnd it seems: I've lost channels on
reconnect with 'sync error'.  Since I never got this code to be reliable,
disable it for next release since I suspect it's our fault :(

And reenable the check which didn't work, for others to untangle.

I couldn't get option_data_loss_protect to be reliable, and I disabled
the check.  This was a mistake, I should have either spent even more
time trying to get to the bottom of this (especially, writing test
vectors for the spec and testing against other implementations).

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

@rustyrussell rustyrussell force-pushed the rustyrussell:guilt/option_dataloss_protect-trouble branch from f18d1c8 to f6d0a6d Dec 10, 2018

@cdecker

This comment has been minimized.

Copy link
Member

cdecker commented Dec 10, 2018

This will effectively make us incompatible with BLW again since they require the option with all their peers.

I'm ok with that, if fixing this takes more effort than to just roll over to the next commitment.

@niftynei
Copy link
Collaborator

niftynei left a comment

lgtm

@rustyrussell rustyrussell merged commit 6aa511f into ElementsProject:master Dec 10, 2018

1 of 2 checks passed

ackbot Need at least 1 ACKs
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment