chanbackup, server, rpcserver: put close unsigned tx, remote signature and commit height to SCB

[starius]

Change Description

In this PR I'm implementing the proposal from https://github.com/lightningnetwork/lnd/issues/7658#issuecomment-1774983789

I changed chanbackup to add CloseTxInputs optional field to chanbackup.Single structure. When the field is present, the version byte is XORed with closeTxVersion = 128. When this bit is present in the version byte, the field is properly extracted when decoding packaged single backup.

I added chantools scbforceclose command to extract and sign closing tx. https://github.com/lightninglabs/chantools/pull/95

Implemented:

• [x] add global and maybe also lncli flags enabling close tx in SCB (now it is always on)
  • [x] documentation for the flag and the feature
• [x] tests for new features of chanbackup
• [x] integration tests
• [x] lines wrap at 80
• [x] commit structure
• [x] when LND shuts down, update channel.backup with latest close tx inputs

Steps to Test

I tested the following scenario on testnet:

• lncli openchannel
• lncli exportchanbackup --chan_point xxx:x --output_file single.backup
• lncli stop
• chantools --testnet scbforceclose --single_backup xxx
• Manually broadcasted the tx
• Started LND from scratch on another machine
• lncli create. Use existing seed, recover onchain funds.
• lncli restorechanbackup --single_file single.backup
• find the channel in lncli pendingchannels, total_limbo_balance is equal to channel balance minus fees

Pull Request Checklist

Testing

• [x] Your PR passes all CI checks.
• [x] Tests covering the positive and negative (error paths) are included.
• [x] Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

• [x] The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
• [x] Commits follow the Ideal Git Commit Structure.
• [x] Any new logging statements use an appropriate subsystem and logging level.
• [x] There is a change description in the release notes, or [skip ci] in the commit message for small changes.

📝 Please see our Contribution Guidelines for further guidance.

Summary by CodeRabbit

• New Features
  • Enhanced backup configuration flexibility with BackupOption parameters.
  • Added support for force close transaction data in backups.
  • Updated backup commands to include an option for force close transaction inputs.
  • Introduced a new option in LND for including data for unilateral channel closure in an SCB file.
  • Added a new parameter for controlling backup behavior in channel restoration scenarios.
  • Included an option to specify inclusion of inputs of the latest force close transaction in channel backups.
• Bug Fixes
  • Fixed a typo in the log message within the SetupStandbyNodes function.
• Refactor
  • Refined backup creation and serialization/deserialization logic to accommodate new backup options.
  • Updated LightningChannel to use a more modular approach for signing commitment transactions.
• Documentation
  • Improved explanations on commitment transaction sweeping in various scenarios.

[starius]

@guggero Thank you for feedback! Happy New Year!

I updated both PRs and also their descriptions. SCB file now contains unsigned CommitTx, CommitSig of remote party, and for taproot channels also CommitHeight. I kept unresolved the comments where further acctention may be needed.

Please take another look!

[starius]

I fixed few bugs in this PR.

1. Integration test failed. I reproduced the failure using the following command (bitcoind binary is needed in PATH):

make itest icase=channel_backup_restore_basic backend=bitcoind

I found the root cause in log file itest/.logs-tranche0/4-channel_backup_restore_basic-dave-021bb9a5.log .

panic stack trace

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x21ec98]
goroutine 1 [running]:
github.com/btcsuite/btcd/wire.(*MsgTx).BtcEncode(0x0, {0x14691a0, 0x4001c3f500?}, 0x26a2832?, 0x2)
/root/go/pkg/mod/github.com/btcsuite/[email protected]/wire/msgtx.go:736 +0x38
github.com/btcsuite/btcd/wire.(*MsgTx).Serialize(...)
/root/go/pkg/mod/github.com/btcsuite/[email protected]/wire/msgtx.go:828
github.com/lightningnetwork/lnd/chanbackup.(*Single).Serialize(0x40004141a0, {0x14691a0?, 0x4001c3f470})
/root/lnd/chanbackup/single.go:363 +0x560
github.com/lightningnetwork/lnd/chanbackup.Multi.PackToWriter({0xa8?, {0x4000414000?, 0x400000d110?, 0x4000169f88?}}, {0x14691a0, 0x4001c3f440}, {0xffff6c95df30, 0x400000d110})
/root/lnd/chanbackup/multi.go:84 +0x240
github.com/lightningnetwork/lnd/chanbackup.(*SubSwapper).updateBackupFile(0x40001e40e0, {0x0, 0x0, 0x0?})
/root/lnd/chanbackup/pubsub.go:238 +0x48c
github.com/lightningnetwork/lnd/chanbackup.(*SubSwapper).Start.func1()
/root/lnd/chanbackup/pubsub.go:157 +0x6c
sync.(*Once).doSlow(0x4000716c60?, 0x0?)
/usr/local/go/src/sync/once.go:74 +0x100
sync.(*Once).Do(...)
/usr/local/go/src/sync/once.go:65
github.com/lightningnetwork/lnd/chanbackup.(*SubSwapper).Start(0x400049a420?)
/root/lnd/chanbackup/pubsub.go:151 +0x50
github.com/lightningnetwork/lnd.(*server).Start.func1()
/root/lnd/server.go:2072 +0x2120
sync.(*Once).doSlow(0x4?, 0x11b931e?)
/usr/local/go/src/sync/once.go:74 +0x100
sync.(*Once).Do(...)
/usr/local/go/src/sync/once.go:65
github.com/lightningnetwork/lnd.(*server).Start(0x1bf?)
/root/lnd/server.go:1864 +0x6c
github.com/lightningnetwork/lnd.Main(0x400002ab00, {{0x0?, 0x40001822a0?, 0x4000182300?}}, 0x400010f080, {0x40001a2480, 0x40001822a0, 0x4000182300, 0x40001823c0, {0x0}})
/root/lnd/lnd.go:684 +0x3164
main.main()
/root/lnd/cmd/lnd/main.go:38 +0x198

LND panicked trying to serialize a backup with nil CommitTx inside non-nil CloseTxInputs. This is possible when DLP is active and this is exactly what is tested in the failing test (channel_backup_restore_basic).

I changed buildCloseTxInputs to return nil *CloseTxInputs producing classic SCB file in this case.

2. Also I found a mistake in error handling in Single.Serialize, returning nil instead of error. Fixed it.

3. Also I figured out that wire.MsgTx can be serialized directly to io.Writer, without a temp buffer. Putting length of its serialization is redundant. I changed it to read and write tx directly.

[Roasbeef]

@coderabbitai review

[coderabbitai[bot]]

[!IMPORTANT]

Review skipped

Auto reviews are limited to specific labels.

Labels to auto review (1)

• llm-review

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

This update introduces a new feature allowing the inclusion of close transaction inputs in channel backups, enhancing data recovery options. It involves adding a BackupConfig structure, modifying functions to accept backup options, and updating serialization/deserialization logic to handle the new CloseTxInputs data. Additionally, it refactors signing logic in LightningChannel for better modularity and clarity, and corrects minor issues across the codebase.

Changes

Files	Change Summary
chanbackup/backup.go, chanbackup/pubsub.go, rpcserver.go, server.go	Introduced BackupConfig and BackupOption for configurable backup creation; added support for including close transaction inputs.
chanbackup/single.go, chanbackup/single_test.go	Added CloseTxInputs struct and updated serialization/deserialization logic; added tests for new backup version.
cmd/lncli/commands.go	Changed ChanBackup field type and modified value assignment.
contractcourt/commit_sweep_resolver.go	Enhanced explanations on commitment sweeping scenarios.
docs/recovery.md	Added information on --backupclosetxinputs option for SCB files and fund recovery.
lnrpc/lightning.proto	Added with_close_tx_inputs field to specify force close transaction inclusion.
lnwallet/channel.go	Refactored signing logic for commitment transactions and added taproot channel support.

🐇✨
In the land of code and byte,
A rabbit hopped with sheer delight.
"New backups," it squeaked, "are here to stay,
With close tx inputs, hooray, hooray!
Through refactor and tweak, we hop along,
Ensuring our data's robust and strong."
🌟🐾

Possibly related issues

• lightningnetwork/lnd#7658: The changes related to including the last state in backups and close summaries align with enhancing data recovery options.
• lightningnetwork/lnd#8366: Addressing pending_force_close channels post-SCB restoration could involve handling close transaction inputs.
• lightningnetwork/lnd#8280: Inspecting the mempool post-SCB restoration may relate to the changes in handling close transaction inputs.
• lightningnetwork/lnd#8392: Resolving issues with force close transactions aligns with the enhancements for close transaction inputs.
• lightningnetwork/lnd#8448: Addressing "limbo balance" recovery could intersect with changes related to force close transaction handling.

---

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

• X
• Mastodon
• Reddit
• LinkedIn

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai generate interesting stats about this repository and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[starius]

I addressed feedback from @coderabbitai

• added options to API requests and LND option to explicitly enable inclusion of CloseTxInputs into backups
  • when LND shuts down, update channel.backup with latest close tx inputs - DONE
• documented new options
• changed itest/lnd_channel_backup_test to run the tests in both modes (with the feature enabled/disabled via backupclosetxinputs flag of LND)

[coderabbitai[bot]]

@starius, thank you for addressing the feedback and making those updates. It's great to see the inclusion of options for enabling the CloseTxInputs feature in backups, the documentation of these new options, and the adjustment of integration tests to cover both enabled and disabled states of the feature.

I'll review the changes to ensure everything aligns with best practices and the objectives of the PR. If there are any specific areas you'd like me to focus on or if there's additional context that might help with the review, please let me know.

Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai[bot]]

[!TIP] For best results, initiate chat on the files or code changes.

@starius, it's great to hear that you've made these updates based on the feedback. Adding options for enabling the CloseTxInputs feature, updating the channel.backup file upon LND shutdown, documenting the new options, and adjusting the integration tests to cover both enabled and disabled states of the feature are significant improvements.

Given the detailed updates you've provided, I'll proceed with reviewing the changes based on the descriptions you've shared. If there are specific files or sections of code you'd like me to focus on, please let me know, and I'll be happy to take a closer look.

Keep up the excellent work, and feel free to reach out if you have any questions or need further assistance.

[starius]

There are no tests that the backup with CloseTxInputs can be used to produce a commit transaction. To add such tests, the code signing commit tx should be available to such a test. Currently the code is in https://github.com/lightninglabs/chantools/pull/95 in file cmd/chantools/scbforceclose.go in function signCloseTx. One approach is to move the function to LND and test it here, unit tests and itest. Another approach is to make an optional itest depending on chantools which runs chantools scbforceclose command. Any ideas on should this be tested in the first place and which of the options is preferred?

[guggero]

@saubyk this is a nice security/recovery feature. Should we prioritize it for v0.19?

@starius, removing my request for review until prioritized (same for https://github.com/lightninglabs/chantools/pull/95).

[saubyk]

@saubyk this is a nice security/recovery feature. Should we prioritize it for v0.19?

Tagged for 0.19

[lightninglabs-deploy]

@starius, remember to re-request review from reviewers when ready

[starius]

Rebased on top of 0.18, added release notes, updated PR description. Retested manually using chantools scbforceclose.

[starius]

@guggero Thanks for the review! 💯

I fixed the comments.

Also I moved function SignCloseTx from chantools PR here and used it in itest which tests the following cases:

• Make sure channel.backup file is updated upon LND shutdown.
• Make sure channel.backup file has sufficient information combined with seed and password to produce a force close tx and sweep the funds.
• Make sure that if the latest channel version is used in channel.backup file in scbforceclose recovery method, another peer does not penalize.
• Control for the previous test case: make sure that if an outdated channel state in channel.backup is used, another node can penalize.

These changes are done in two latest commits.

[starius]

Rebased. Moved the release notes change to 0.18.3.

[starius]

@guggero Thank you for review! I addresses the feedback.

Also, I checked the failures in CI. The failures of itests on windows and "unit tests (btcd unit-cover)" seem to be unrelated to the PR.

One of new itests failed on neutrino backend:

    lnd_channel_backup_test.go:290: 
        	Error Trace:	/home/runner/work/lnd/lnd/itest/lnd_channel_backup_test.go:2287
        	            				/home/runner/work/lnd/lnd/itest/lnd_channel_backup_test.go:290
        	            				/home/runner/work/lnd/lnd/itest/lnd_channel_backup_test.go:940
        	            				/home/runner/work/lnd/lnd/itest/lnd_channel_backup_test.go:705
        	Error:      	Not equal: 
        	            	expected: 1
        	            	actual  : 0
        	Test:       	TestLightningNetworkDaemon/tranche00/07-of-156/neutrino/channel_backup_restore_commit_types/scbforceclose_carol_penalizes/restore_from_backup_file_anchors

This itests is passing on my machine. It could happen in CI, because Carol didn't manage to broadcast the penalty transaction in time before ht.MineBlocksAndAssertNumTxes(1, 1) was called, so the mined block contained no transactions. Race condition. Might this be the case?

I added require.Eventually waiting for mempool for having 1 transaction before MineBlocksAndAssertNumTxes.

There are other MineBlocksAndAssertNumTxes in itests. I'm curious if they might be race conditions?

[guggero]

Race condition. Might this be the case?

Yes, unfortunately the neutrino itest isn't super stable on CI yet, mainly due to force close timing issues. I think @yyforyongyu is looking into that with https://github.com/lightningnetwork/lnd/pull/8892.

So if the test passes for you locally, that's fine.

[starius]

Neutrino itests are passing now in CI.

Windows itests fail, but they are flacky in master branch. Failures seem to be unrelated to the PR.

btcd unit-cover tests fail in master with the same error:

Error: cannot read source of file "/home/runner/work/lnd/lnd/fn/func.go": open /home/runner/work/lnd/lnd/fn/func.go: no such file or directory

[starius]

I rebased the PR and fixed itest build errors (changes in ht interface and chanRestoreViaRPC function).

Also removed waiting for tx in mempool and added waiting for ClosedChannels call next to it, since it failed sometimes, because a channel was still closing during the call (so it saw 0 closed channels).

Added commit "lntest: fix typo".

[lightninglabs-deploy]

@bhandras: review reminder @starius, remember to re-request review from reviewers when ready