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

server: require the DLP bit for all incoming/outgoing connections #2500

Merged
merged 2 commits into from Feb 1, 2019

Conversation

Projects
None yet
4 participants
@Roasbeef
Copy link
Member

Roasbeef commented Jan 18, 2019

In this commit, we modify our default local feature bits to require the
Data Loss Protection (DLP) feature to be active. Once full Static
Channel Backups are implemented, if we connect to a peer that doesn't
follow the DLP protocol, then the SCBs are useless, as we may not be
able to recover funds. As a result, in prep for full SCB deployment,
we'll now ensure that any peer we connect to, knows of the DLP bit. This
could be a bit more relaxed and allow connections to non-DLP peers,
but reject channel requests to/from them. However, this implementation is
much simpler.

server: require the DLP bit for all incoming/outgoing connections
In this commit, we modify our default local feature bits to require the
Data Loss Protection (DLP) feature to be active. Once full Static
Channel Backups are implemented, if we connect to a peer that doesn't
follow the DLP protocol, then the SCBs are useless, as we may not be
able to recover funds. As a result, in prep for full SCB deployment,
we'll now ensure that any peer we connect to, knows of the DLP bit. This
could be a bit more relaxed and allow _connections_ to non-DLP peers,
but reject channel requests to/from them. However, this implementation is
much simpler.
@cfromknecht

This comment has been minimized.

Copy link
Collaborator

cfromknecht commented Jan 18, 2019

We also need to modify peer.handleInitMsg to disconnect if the other peer doesn't advertise optional or required support DLP. Right now, we will rely on the other peer to DC if they aren't advertising, so a bug on the remote end could cause connections to be accepted that should have been disconnected.

@Roasbeef Roasbeef added this to the 0.6 milestone Jan 18, 2019

@Roasbeef

This comment has been minimized.

Copy link
Member Author

Roasbeef commented Jan 19, 2019

Pushed up a commit to now disconnect the peers that don't have this new feature bit. Before we merge this, I want to refactor this area to be a bit more general so we have a single place in the peer or server where manage which feature bits we advertise/require.

@wpaulino
Copy link
Collaborator

wpaulino left a comment

LGTM 🛰

@cfromknecht

This comment has been minimized.

Copy link
Collaborator

cfromknecht commented Jan 30, 2019

@Roasbeef are you still planning to the refactoring here?

@Roasbeef

This comment has been minimized.

Copy link
Member Author

Roasbeef commented Jan 30, 2019

Ideally yes. If y'all don't consider it a blocker, then we can have this land now, and I can follow up with a refactor+tests.

@cfromknecht
Copy link
Collaborator

cfromknecht left a comment

LGTM 🔑 can defer testing for another PR

@Roasbeef Roasbeef merged commit 5167b02 into lightningnetwork:master Feb 1, 2019

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.01%) to 56.29%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kind84

This comment has been minimized.

Copy link

kind84 commented Feb 1, 2019

I think that this merge has some issues, my node cannot bootstrap connections saying "unable to start peer: data loss protection required".

@cfromknecht

This comment has been minimized.

Copy link
Collaborator

cfromknecht commented Feb 1, 2019

@kind84 indeed, see #2570 for fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.