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

chanbackup: always combine with on-disk state when swapping, refuse to start w/ invalid files #4379

Merged
merged 2 commits into from
Jun 19, 2020

Conversation

Roasbeef
Copy link
Member

@Roasbeef Roasbeef commented Jun 16, 2020

In this commit, we fix a bug that could possibly cause a user's on disk
back up file to be wiped out, if they ever started another lnd node
with different channel state. To remedy this, before we swap out the
channel state with what's on disk, we'll first read out the contents of
the on-disk SCB file and combine that with what we have in memory.

We also add an additional defense against starting up with an
invalid SCB state. With this commit, we'll now fail to start up if we're
unable to update or read the existing SCB state from disk. This will
prevent an lnd node from starting up with an invalid SCB file, an SCB
file it can't decrypt, or an SCB left over from another node.

Fixes #4377

@Roasbeef Roasbeef added safety General label for issues/PRs related to the safety of using the software P1 MUST be fixed or reviewed bug fix labels Jun 16, 2020
@Roasbeef Roasbeef added this to the 0.10.2 milestone Jun 16, 2020
@Roasbeef Roasbeef requested review from guggero and cfromknecht June 16, 2020 00:44
@Roasbeef Roasbeef changed the title chanbackup: always combine with on-disk state when swapping, reuse to start w/ invalid files chanbackup: always combine with on-disk state when swapping, refuse to start w/ invalid files Jun 16, 2020
Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Solid fix, LGTM 💯

Tested locally on regtest.

@Roasbeef Roasbeef force-pushed the scb-union-bug-fix branch from 0c1334c to 09fb32e Compare June 17, 2020 00:58
}
}
for _, memChannel := range s.backupState {
combinedBackup[memChannel.FundingOutpoint] = memChannel
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps we should check that if we find a collision we are setting it with the same value? just as a sanity check

Copy link
Member Author

Choose a reason for hiding this comment

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

What would the behavior be if the value is different? I think it's better to replace the existing, as the new one might have a new field, or be using a new SCB version. If we want to replace, then this logic implements that as is.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's also possible to just leave the duplicate there too.

Copy link
Contributor

Choose a reason for hiding this comment

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

that's true, i was thinking even a warning would be nice to see. you're right though the newer one is probably more correct

Copy link
Member Author

Choose a reason for hiding this comment

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

log warning added

Copy link
Member Author

Choose a reason for hiding this comment

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

Realize now, that the way it is, it'll always print that log statement in the typical case...

Roasbeef added 2 commits June 17, 2020 17:44
In this commit, we fix a bug that could possibly cause a user's on disk
back up file to be wiped out, if they ever started _another_ lnd node
with different channel state. To remedy this, before we swap out the
channel state with what's on disk, we'll first read out the contents of
the on-disk SCB file and _combine_ that with what we have in memory.

Fixes lightningnetwork#4377
In this commit, we add an additional defense against starting up with an
invalid SCB state. With this commit, we'll now fail to start up if we're
unable to update or read the existing SCB state from disk. This will
prevent an lnd node from starting up with an invalid SCB file, an SCB
file it can't decrypt, or an SCB left over from another node.
@Roasbeef Roasbeef force-pushed the scb-union-bug-fix branch from 09fb32e to 496e29d Compare June 18, 2020 00:45
@Roasbeef Roasbeef requested a review from cfromknecht June 18, 2020 02:47
Copy link
Contributor

@cfromknecht cfromknecht left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fix P1 MUST be fixed or reviewed safety General label for issues/PRs related to the safety of using the software
Projects
None yet
Development

Successfully merging this pull request may close these issues.

channels.backup can be overwritten as empty if a corrupted channel.db is removed
3 participants