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

chanbackup: update on-disk backup file with unconfirmed channels #3993

Merged
merged 5 commits into from
Mar 10, 2020
Merged

chanbackup: update on-disk backup file with unconfirmed channels #3993

merged 5 commits into from
Mar 10, 2020

Conversation

guggero
Copy link
Collaborator

@guggero guggero commented Feb 10, 2020

Fixes #3816.

Currently there is a discrepancy between the backups obtained through the gRPC call and the content of the automatically created channel.backup file. The RPC backups do contain pending channels while the backup file does not.
This PR aims to fix that discrepancy.

@guggero guggero added safety General label for issues/PRs related to the safety of using the software recovery Related to the backup/restoration of LND data (e.g. wallet seeds) backups SCB Related to static channel backup labels Feb 10, 2020
@guggero guggero added this to the 0.10.0 milestone Feb 10, 2020
@guggero guggero requested review from cfromknecht and removed request for Roasbeef February 10, 2020 16:15
cfromknecht
cfromknecht reviewed Feb 12, 2020
View reviewed changes
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.

solid pr, no issues with the existing code. my only other question is are these pending channels ever cleaned up if the channels never confirm? it seems like they could continue to inflate the size of our channel.backup indefinitely unless they are later pruned, no?

fundingmanager_test.go Outdated Show resolved Hide resolved
fundingmanager_test.go Outdated Show resolved Hide resolved
lntest/itest/lnd_test.go Show resolved Hide resolved
@guggero
fundingmgr+channelnotifier: add channel config to pending chan ntfn
182835d 
To be able to write a new channel backup file for pending channels,
we need to include the channel configuration in the pending channel
notification event.
@guggero
Copy link
Collaborator Author

guggero commented Feb 14, 2020

are these pending channels ever cleaned up if the channels never confirm?

Good question. What happens with them in the channel DB? Are they detected and removed after a certain time? Or would you need to call abandonchannel on them? If the latter is true then we can just add the close notification to AbandonChannel.
But still, you might want to keep the backup in case it ever confirms for whatever reason. It's a few hundred bytes per channel and their number should be very limited so I'm not too worried about the size of the backup file.

halseth
halseth suggested changes Mar 4, 2020
View reviewed changes
fundingmanager_test.go Show resolved Hide resolved
channel_notifier.go Outdated Show resolved Hide resolved
@halseth
Copy link
Contributor

halseth commented Mar 4, 2020

are these pending channels ever cleaned up if the channels never confirm?

Good question. What happens with them in the channel DB? Are they detected and removed after a certain time? Or would you need to call abandonchannel on them? If the latter is true then we can just add the close notification to AbandonChannel.
But still, you might want to keep the backup in case it ever confirms for whatever reason. It's a few hundred bytes per channel and their number should be very limited so I'm not too worried about the size of the backup file.

Yes, currently they will stay around (only if you're the initiator) until abandonchannel is called. There is an existing issue/PR to address this: #1755, for now I think it is okay the backup stays around until abandon channel is called.

@guggero
channel_notifier: update backup on pending chan
d8c62c3 
To fix the discrepancy between getting the channel backups via
RPC where all pending channels are included, we also update the
channel.backup file on disk whenever we get a pending channel
event notification.
@guggero
itest: test unconfirmed channel backup file
8a2c02f
@guggero
rpcserver: don't skip pending channels in backup subscription
4e0c276 
The synchronous call to get all channel backups also include
channels that are pending at the moment of the call. A previous
commit added pending channels to the file based backup as well. So
this is the last backup method that needs to be adjusted to also
contain unconfirmed channels.
@guggero
rpcserver+itest: remove channel from backup when abandoning it
ab024b9
@guggero
Copy link
Collaborator Author

guggero commented Mar 4, 2020

I added a commit to remove the channel from the backup file if abaondonchannel is called.

@guggero guggero requested a review from halseth March 4, 2020 10:00
halseth
halseth approved these changes Mar 4, 2020
View reviewed changes
Copy link
Contributor

@halseth halseth left a comment

Choose a reason for hiding this comment

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

LGTM 💯

Roasbeef
Roasbeef approved these changes Mar 10, 2020
View reviewed changes
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🌊

@Roasbeef Roasbeef merged commit cbef26b into lightningnetwork:master Mar 10, 2020
@guggero guggero deleted the unconfirmed-chanbackup branch March 10, 2020 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Roasbeef Roasbeef Roasbeef approved these changes

@halseth halseth halseth approved these changes

@cfromknecht cfromknecht Awaiting requested review from cfromknecht

Assignees
No one assigned
Labels
backups recovery Related to the backup/restoration of LND data (e.g. wallet seeds) safety General label for issues/PRs related to the safety of using the software SCB Related to static channel backup
Projects
None yet
Milestone
0.10.0
Development

Successfully merging this pull request may close these issues.

unconfirmed channels are not included in channel backup when using gRPC
4 participants
@guggero @halseth @Roasbeef @cfromknecht