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

lnd: implement "safe mode" node stand up #3601

Closed
wants to merge 5 commits into from

Conversation

mlerner
Copy link
Contributor

@mlerner mlerner commented Oct 15, 2019

This PR implements safe mode for lnd by disabling all RPC and automated force close requests, as described in the original issue here: #3287

  • Add new --safemode config parameter to the lnd binary.

  • If safe mode is enabled, reject all RPC level force close requests.

  • If safe mode is enabled, reject all automated force close requests by channel arbitrators.

@Roasbeef Roasbeef added database enhancement safety security labels Oct 23, 2019
@Roasbeef Roasbeef removed request for Roasbeef and joostjager Oct 23, 2019
@joostjager joostjager added this to WIP in v0.9.0-beta via automation Oct 23, 2019
@joostjager joostjager added this to the 0.9.0 milestone Oct 23, 2019
@@ -232,6 +236,9 @@ func newActiveChannelArbitrator(channel *channeldb.OpenChannel,
ShortChanID: channel.ShortChanID(),
BlockEpochs: blockEpoch,
ForceCloseChan: func() (*lnwallet.LocalForceCloseSummary, error) {
if c.cfg.SafeMode {
Copy link
Collaborator

@halseth halseth Oct 24, 2019

Choose a reason for hiding this comment

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

this will beed to be checked when the force close request is first sent. Otherwise I think we'll get into a state where we will attempt to force close the channel on every restart.

Copy link
Collaborator

@cfromknecht cfromknecht Oct 31, 2019

Choose a reason for hiding this comment

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

indeed, and we should mark it before we mark the channel borked and save the force close, o/w we will broadcast if the node is restarted and we're not in safe mode

Copy link
Collaborator

@halseth halseth Nov 1, 2019

Choose a reason for hiding this comment

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

I don't think we should mark the channel at all in safe mode though, just to not alter any state?

Copy link
Collaborator

@cfromknecht cfromknecht Nov 2, 2019

Choose a reason for hiding this comment

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

sorry, check* it before we mark

yes we want to make sure no state is altered

@@ -273,6 +273,7 @@ type config struct {

UnsafeDisconnect bool `long:"unsafe-disconnect" description:"Allows the rpcserver to intentionally disconnect from peers with open channels. USED FOR TESTING ONLY."`
UnsafeReplay bool `long:"unsafe-replay" description:"Causes a link to replay the adds on its commitment txn after starting up, this enables testing of the sphinx replay logic."`
SafeMode bool `long:"safemode" description:"Start in safe mode that forbids broadcasting of all commitment transactions"`
Copy link
Collaborator

@cfromknecht cfromknecht Oct 31, 2019

Choose a reason for hiding this comment

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

perhaps we should call this something more descriptive than safe mode, e.g. ForbidForceClose?

Copy link
Collaborator

@carlaKC carlaKC Nov 1, 2019

Choose a reason for hiding this comment

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

+1, I think the term safe in lightning is quite a strong word 😅

@@ -232,6 +236,9 @@ func newActiveChannelArbitrator(channel *channeldb.OpenChannel,
ShortChanID: channel.ShortChanID(),
BlockEpochs: blockEpoch,
ForceCloseChan: func() (*lnwallet.LocalForceCloseSummary, error) {
if c.cfg.SafeMode {
Copy link
Collaborator

@cfromknecht cfromknecht Oct 31, 2019

Choose a reason for hiding this comment

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

indeed, and we should mark it before we mark the channel borked and save the force close, o/w we will broadcast if the node is restarted and we're not in safe mode

@@ -130,6 +130,11 @@ func Main(lisCfg ListenerCfg) error {
}
}()

if cfg.SafeMode {
ltndLog.Infof("Starting LND in safe mode." +
"All commitment transactions are forbidden in this state.")
Copy link
Collaborator

@carlaKC carlaKC Nov 1, 2019

Choose a reason for hiding this comment

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

logging nit" "broadcast of commitment transactions is forbidden"?

@@ -273,6 +273,7 @@ type config struct {

UnsafeDisconnect bool `long:"unsafe-disconnect" description:"Allows the rpcserver to intentionally disconnect from peers with open channels. USED FOR TESTING ONLY."`
UnsafeReplay bool `long:"unsafe-replay" description:"Causes a link to replay the adds on its commitment txn after starting up, this enables testing of the sphinx replay logic."`
SafeMode bool `long:"safemode" description:"Start in safe mode that forbids broadcasting of all commitment transactions"`
Copy link
Collaborator

@carlaKC carlaKC Nov 1, 2019

Choose a reason for hiding this comment

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

+1, I think the term safe in lightning is quite a strong word 😅

@mlerner
Copy link
Contributor Author

@mlerner mlerner commented Nov 1, 2019

Thanks for the feedback @carlaKC @cfromknecht @halseth. I'm going to start updating the PR with your suggested changes.

// force close message to a node in safe mode should fail.
ctxt, _ = context.WithTimeout(ctxb, channelCloseTimeout)
_, _, err = net.CloseChannel(ctxt, carol, chanPoint, true)
if err == nil {
Copy link
Collaborator

@Crypt-iQ Crypt-iQ Nov 2, 2019

Choose a reason for hiding this comment

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

You could check whether or not the error matches the "can not force close channels" text

@Crypt-iQ
Copy link
Collaborator

@Crypt-iQ Crypt-iQ commented Nov 2, 2019

Might want to add a unit test to test the new behavior

@halseth
Copy link
Collaborator

@halseth halseth commented Nov 12, 2019

Friendly ping @mlerner

@joostjager joostjager moved this from WIP to TODO in v0.9.0-beta Nov 12, 2019
@Roasbeef Roasbeef removed this from the 0.9.0 milestone Jan 7, 2020
@Roasbeef Roasbeef added this to the 0.10.0 milestone Jan 7, 2020
@Roasbeef Roasbeef added this to In progress in v0.10.0-beta via automation Jan 17, 2020
@Roasbeef Roasbeef moved this from In progress to Blocked in v0.10.0-beta Jan 17, 2020
@Roasbeef Roasbeef removed this from the 0.10.0 milestone Feb 18, 2020
@Roasbeef Roasbeef removed this from Blocked in v0.10.0-beta Feb 18, 2020
@Roasbeef Roasbeef removed the v0.9.0 label Feb 18, 2020
@halseth halseth removed their assignment Feb 18, 2020
@mlerner mlerner closed this Dec 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
database enhancement safety security
Projects
No open projects
v0.9.0-beta
  
TODO
Development

Successfully merging this pull request may close these issues.

None yet

7 participants