chanbackup, server, rpcserver: put close unsigned tx, remote signature and commit height to SCB #8183
Conversation
starius
force-pushed
the
close-tx-in-static-backup
branch
from
151feee
to
7b2eb0b
Compare
There was a problem hiding this comment.
Took a first look. I think the general idea is nice, this should help in some extreme edge cases.
Though the way we currently update those channels the commitment TX within the backup would be outdated very quickly for normal channels. So as a safety precaution we should not include a signature to make it hard for people to accidentally breach themselves.
starius
force-pushed
the
close-tx-in-static-backup
branch
2 times, most recently
from
eebf352
to
e7bc2ea
Compare
starius
changed the title
|
@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
force-pushed
the
close-tx-in-static-backup
branch
from
e7bc2ea
to
a1b38f8
Compare
|
I fixed few bugs in this PR.
I found the root cause in log file panic stack tracepanic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x21ec98] 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
|
|
@coderabbitai review |
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update introduces a new feature allowing the inclusion of close transaction inputs in channel backups, enhancing data recovery options. It involves adding a Changes
Possibly related issues
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (10)
- chanbackup/backup.go (3 hunks)
- chanbackup/pubsub.go (3 hunks)
- chanbackup/single.go (6 hunks)
- chanbackup/single_test.go (4 hunks)
- cmd/lncli/commands.go (1 hunks)
- contractcourt/commit_sweep_resolver.go (1 hunks)
- lntest/harness.go (1 hunks)
- lnwallet/channel.go (4 hunks)
- rpcserver.go (3 hunks)
- server.go (1 hunks)
Files not reviewed due to errors (1)
- server.go (Error: unable to parse review)
Files skipped from review due to trivial changes (1)
- lntest/harness.go
Additional comments: 21
chanbackup/backup.go (3)
- 89-98: The
WithCloseTxInputsfunction correctly implements theBackupOptiontype to modifyBackupConfig. Verify that all calls to this function correctly reflect the user's intention regarding the inclusion ofCloseTxInputs.- 53-112: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [104-130]
In
FetchBackupForChan, the handling ofincludeCloseTxInputsoption is correct. Ensure that thebuildCloseTxInputsfunction is robust and handles all edge cases, especially since it directly affects the backup's content based on this option.
- 124-145: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [137-164]
In
FetchStaticChanBackups, the handling ofincludeCloseTxInputsoption is consistent withFetchBackupForChan. Double-check that the iteration over channels and the conditional inclusion ofCloseTxInputsare correctly implemented and tested.chanbackup/pubsub.go (3)
- 101-103: The addition of the
includeCloseTxInputsfield in theSubSwapperstruct is consistent with the overall feature addition. Ensure that this field is used appropriately in all relevant backup operations withinSubSwapper.- 97-120: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [111-144]
The
NewSubSwapperfunction correctly processesBackupOptionparameters to configure theSubSwapperinstance. Confirm that all instances ofSubSwappercreation throughout the codebase are updated to pass the necessaryBackupOptionparameters.
- 280-289: The conditional inclusion of
CloseTxInputsbased on theincludeCloseTxInputsfield within thebackupUpdatermethod is correctly implemented. Verify that thebuildCloseTxInputsfunction correctly handles all channel types and scenarios.chanbackup/single_test.go (3)
- 5-5: The addition of the
encoding/heximport is necessary for the new test cases involving hex encoding. Confirm that it's used appropriately in all test cases.- 99-132: The modifications to
assertSingleEqualto include checks forCloseTxInputsare correct. Ensure that all relevant test cases now properly account forCloseTxInputswhen comparingSingleinstances.- 292-375: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [248-366]
The new test cases in
TestSinglePackUnpackfor different versions withCloseTxInputsare well-structured. Verify that these cases cover all scenarios, including edge cases forCloseTxInputshandling.contractcourt/commit_sweep_resolver.go (1)
- 28-35: The added comments provide clear explanations of the logic for handling CSV delays and CLTV expiration. This enhances code readability and maintainability.
chanbackup/single.go (5)
- 55-58: Introduced
closeTxVersionMaskto indicate the presence ofCloseTxInputsin backups. This approach allows backward compatibility and future extensibility.- 145-152: Added
CloseTxInputsstruct toSinglefor storing force close transaction data. This is crucial for enhancing the robustness of SCBs by including necessary data for force-closing channels.- 358-383: In
Serialize, the conditional inclusion ofCloseTxInputsbased on its presence and version-specific handling forCommitHeightis correctly implemented. Ensure that future versions also consider compatibility with this feature.- 485-487: In
Deserialize, correctly handling the version mask to determine the presence ofCloseTxInputs. This ensures backward compatibility and correct parsing of new backup formats.- 600-628: The deserialization logic for
CloseTxInputscorrectly handles the conditional parsing based on the version and the presence ofCloseTxInputs. This maintains the integrity of the backup data structure.cmd/lncli/commands.go (1)
- 2418-2421: The change from
[]bytetostringfor theChanBackupfield and the use ofhex.EncodeToStringforchanBackup.ChanBackupis observed. Ensure that all related serialization, deserialization, and cryptographic operations are correctly updated to handle thestringtype without loss of data or functionality.lnwallet/channel.go (5)
- 6224-6251: The definition of
SignedCommitTxInputsandTaprootSignedCommitTxInputsstructures is clear and well-documented. However, ensure that all fields, especially those involving cryptographic operations (OurKey,TheirKey,SignDesc), are securely handled throughout their lifecycle to prevent any potential security vulnerabilities.- 6265-6285: In
GetSignedCommitTx, the handling of taproot channels involves re-deriving the local nonce usingchanneldb.NewMusigVerificationNonce. Ensure that error handling for this operation is robust, as failure in nonce derivation could lead to critical issues in transaction signing.- 6295-6301: The creation of the musig session with
NewPartialMusigSessionand decoding of the remote signature withremoteSig.Decodeare critical operations. Verify that the error handling forDecodeis comprehensive and that the musig session creation is securely managed, especially in terms of key handling and nonce generation.- 6347-6365: For non-taproot channels, the generation of the witness stack in
defaultcase usesecdsa.ParseDERSignatureandsigner.SignOutputRaw. Ensure that the cryptographic operations are secure and that the generated witness stack correctly adheres to the expected format for multi-sig spends. Additionally, verify thatSignOutputRawis called with the correct parameters, particularlySignDesc, to prevent any issues in signature generation.- 6374-6397: The refactoring of
getSignedCommitTxto useSignedCommitTxInputsand potentiallyTaprootSignedCommitTxInputsis a significant change. Ensure that this refactoring does not introduce any regressions, especially in the handling of taproot and non-taproot channels. Verify that theGetSignedCommitTxfunction is called with the correct inputs and that the channel's state is correctly used to populate these inputs.
starius
force-pushed
the
close-tx-in-static-backup
branch
from
a1b38f8
to
4163908
Compare
|
I addressed feedback from @coderabbitai
|
|
@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 performedReview triggered.
|
|
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. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 10
Configuration used: CodeRabbit UI
Commits
Files that changed from the base of the PR and between b76f733 and 4163908b7ff4b6f5a77a874649f94a297ac67393.Files ignored due to path filters (3)
lnrpc/lightning.pb.gois excluded by!**/*.pb.golnrpc/lightning.pb.gw.gois excluded by!**/*.pb.gw.golnrpc/lightning.swagger.jsonis excluded by!**/*.json
Files selected for processing (13)
- chanbackup/backup.go (3 hunks)
- chanbackup/pubsub.go (3 hunks)
- chanbackup/single.go (6 hunks)
- chanbackup/single_test.go (4 hunks)
- cmd/lncli/commands.go (7 hunks)
- config.go (1 hunks)
- contractcourt/commit_sweep_resolver.go (1 hunks)
- docs/recovery.md (2 hunks)
- itest/lnd_channel_backup_test.go (10 hunks)
- lnrpc/lightning.proto (3 hunks)
- lnwallet/channel.go (4 hunks)
- rpcserver.go (4 hunks)
- server.go (3 hunks)
Learnings (1)
rpcserver.go (1)
Added flags `lncli exportchanbackup --with_close_tx_inputs` and `lnd --backupclosetxinputs` enable inclusion of `CloseTxInputs` in backups. Documentation updated in docs/recovery.md.
Additional comments (36)
chanbackup/backup.go (2)
- 82-87: The introduction of the
BackupConfigstruct with theincludeCloseTxInputsfield is a good addition for configuring backup creation. This allows for flexibility in including or excluding close transaction inputs in backups, which can be useful depending on the user's needs or storage constraints.- 89-98: The
WithCloseTxInputsfunction is a well-implementedBackupOptionthat enables users to specify whether SCBs should contain inputs needed for force-closing channels. This is a valuable feature for enhancing the robustness of SCBs, especially in scenarios where unilateral channel closure is required.chanbackup/pubsub.go (3)
- 101-103: Adding the
includeCloseTxInputsfield toSubSwapperis a logical extension of the backup configuration options introduced inbackup.go. This field allows the backup subsystem to be aware of whether close transaction inputs should be included in backups, which is crucial for the newchantools scbforceclosefunctionality.- 97-120: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [111-144]
The modification of
NewSubSwapperto handleBackupOptionvariadic parameters is a good design choice, as it aligns with the changes made inbackup.goand allows for consistent backup configuration across different parts of the system. The initialization ofincludeCloseTxInputsbased onBackupConfigensures that the backup subsystem's behavior is configurable and flexible.
- 281-291: Updating
backupUpdaterto conditionally includeCloseTxInputsinSinglecreation based on theincludeCloseTxInputsfield is a necessary change to support the new backup configuration options. This ensures that the backup subsystem can create backups that are consistent with the user's configuration preferences.chanbackup/single_test.go (2)
- 99-132: Adding checks and comparisons for
CloseTxInputsfields inassertSingleEqualis crucial for ensuring that the new functionality works as expected. This update ensures that tests will accurately verify the presence and correctness ofCloseTxInputsin SCBs, which is essential for the reliability of channel backups.- 292-375: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [248-366]
The addition of test cases with
CloseTxInputsinversionTestCasesis a thorough approach to testing the new SCB functionality. These cases cover various scenarios, including different versions and the presence ofCloseTxInputs, ensuring comprehensive test coverage for the changes introduced in this PR.contractcourt/commit_sweep_resolver.go (1)
- 28-36: The detailed explanations provided in the
commitSweepResolverabout the conditions under which commitments can be swept are valuable for understanding the logic behind the resolution process. It's important to ensure that these conditions are accurately represented in the code and that the resolver behaves as expected under various scenarios, including CSV delays and channel leases.Verification successful
To further verify the handling of CSV delays and channel leases in the
commitSweepResolver, we need to inspect the methods within this resolver that implement the logic described in the comments. Specifically, we should look for methods that calculate delays or check conditions before sweeping commitments.
To accurately verify the handling of CSV delays and channel leases in the
commitSweepResolver, we need to inspect the implementation details of thegetCommitTxConfHeightandhasCLTVmethods. These methods appear to be directly related to the conditions described in the comments.
The analysis of the
getCommitTxConfHeightandhasCLTVmethods confirms that thecommitSweepResolverimplements logic that matches the detailed explanations provided in the comments regarding handling CSV delays and channel leases. ThegetCommitTxConfHeightmethod is used to wait for the necessary block confirmations before proceeding, and thehasCLTVmethod checks for the presence of a CLTV condition for channel leases. This verifies that the resolver's implementation accurately represents the conditions under which commitments can be swept, as described.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the logic for handling CSV delays and channel leases in the # commitSweepResolver matches the detailed explanations. ast-grep --lang go --pattern $'commitSweepResolver { $$$ }'Length of output: 1398
Script:
#!/bin/bash # Search for methods within commitSweepResolver that handle CSV delays and channel leases rg --type go "func \(c \*commitSweepResolver\) " ./contractcourt/commit_sweep_resolver.goLength of output: 658
Script:
#!/bin/bash # Inspect the getCommitTxConfHeight and hasCLTV methods for handling CSV delays and channel leases rg --type go -A 10 "func \(c \*commitSweepResolver\) getCommitTxConfHeight\(" ./contractcourt/commit_sweep_resolver.go echo "-----" rg --type go -A 5 "func \(c \*commitSweepResolver\) hasCLTV\(" ./contractcourt/commit_sweep_resolver.goLength of output: 785
chanbackup/single.go (6)
- 56-58: The addition of the
closeTxVersionMaskconstant is correctly implemented and follows a common pattern for encoding additional information within a byte. This approach allows for backward compatibility and future extensibility.- 155-175: The
CloseTxInputsstruct is well-designed and aligns with the objectives of enhancing SCB functionality. The inclusion ofCommitHeightspecifically for taproot channels is a thoughtful consideration for future channel types.- 145-152: The addition of the
CloseTxInputsfield to theSinglestruct is well-executed, with clear documentation and consideration for backward compatibility. This change is crucial for the PR's objectives and is correctly integrated into the existing structure.- 358-382: The updates to the
Serializemethod to handleCloseTxInputsare mostly correct. However, consider refining the error handling for theCommitTxserialization to ensure that any serialization failure is appropriately managed.- 597-632: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [485-628]
The updates to the
Deserializemethod to handleCloseTxInputsare correctly implemented. The method appropriately uses thecloseTxVersionMaskto detect the presence ofCloseTxInputsand handles their deserialization efficiently, ensuring backward compatibility.
- 142-178: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-629]
Overall, the changes to the
chanbackup/single.gofile are well-implemented, with clear documentation and adherence to coding standards. The introduction ofCloseTxInputsand the corresponding updates to serialization and deserialization logic are crucial for enhancing SCB functionality and are executed with consideration for backward compatibility and future extensibility.itest/lnd_channel_backup_test.go (7)
- 81-90: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [59-87]
The introduction of the
backupclosetxinputsflag to control backup behavior is well-implemented. However, it's crucial to ensure that this flag's effect is thoroughly tested across different scenarios within this test suite, especially in the context of SCBs and force-closing channels.
- 420-434: The loop to test both states of the
backupclosetxinputsflag (trueandfalse) is a good approach to ensure that the new functionality works as expected under different configurations. This pattern is consistently applied throughout the test file, which is excellent for coverage.- 444-449: The
newChanRestoreScenariofunction correctly incorporates thebackupclosetxinputsflag into the node arguments. This ensures that the nodes being tested are initialized with the intended backup behavior, which is crucial for the validity of the tests.- 470-497: Testing the channel backup and restore functionality for unconfirmed channels with both states of the
backupclosetxinputsflag is a comprehensive approach. It's important to verify that the system behaves correctly in scenarios where channels might not be fully established before a backup is needed.- 627-641: The loop to test different commitment types and zero-conf channels with both states of the
backupclosetxinputsflag is thorough. It ensures that the backup and restore functionalities are compatible with various channel configurations, which is essential for the robustness of the feature.- 1178-1185: The
testDataLossProtectionfunction's approach to testing data loss protection with both states of thebackupclosetxinputsflag is commendable. It's crucial to ensure that the system can handle scenarios where a node loses state, especially in the context of the new backup functionality.- 1206-1210: The conditional inclusion of the
--backupclosetxinputsflag based on the test parameter is correctly implemented. This ensures that the nodeDaveis tested under both configurations, which is necessary for verifying the feature's impact on data loss protection scenarios.cmd/lncli/commands.go (1)
- 2412-2421: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [2393-2436]
The addition of the
--with_close_tx_inputsflag to theexportChanBackupCommandis correctly implemented. The flag's description clearly explains its purpose, which aligns with the PR objectives to enhance SCB functionality by including essential data for force-closing channels. The logic to check if the flag is set and pass this information to theExportChannelBackupandExportAllChannelBackupsRPC calls is correctly implemented. This change should enable users to include the necessary data for force-closing channels in their SCB exports, improving the robustness of channel backups.config.go (5)
- 354-355: The addition of the
BackupCloseTxInputsboolean field to theConfigstruct introduces a new configuration option for including inputs in the backup to produce the latest force close transaction. This change aligns with the PR's objective to enhance SCB functionality. Ensure that documentation and user guides are updated to reflect this new option and its implications for channel backup and recovery processes.- 351-358: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [2024-2048]
The
supplyEnvValuefunction introduces a mechanism to handle environment variables within configuration values, supporting various formats including default values. This enhancement improves flexibility in configuration management. However, ensure that the documentation clearly explains how to use this feature, including examples of supported formats, to help users correctly configure their environment variables.
- 351-358: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [2024-2048]
The
supplyEnvValuefunction includes logic to redact sensitive information such as passwords when logging configuration values. This is a good security practice to prevent accidental exposure of sensitive data in logs. Continue to apply this approach consistently across all logging of configuration values to enhance security.
- 351-358: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [2024-2048]
The use of regular expressions in functions like
extractBtcdRPCParamsandextractBitcoindRPCParamsfor parsing configuration files is appropriate for the use case. Given that these operations are performed once at startup, the impact on performance is minimal. However, consider precompiling regular expressions that are used multiple times or in loops to optimize performance in future enhancements.
- 351-358: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [2024-2048]
The configuration parsing and validation logic is well-structured, with clear separation of concerns and comprehensive error handling. The use of reflective programming to flatten the configuration struct into a map for easier processing of deprecated options is a clever approach. Ensure that error messages are clear and informative, providing users with actionable guidance on resolving configuration issues.
lnrpc/lightning.proto (3)
- 4469-4472: The addition of the
with_close_tx_inputsfield toExportChannelBackupRequestis consistent with the PR's objective to enhance SCBs by including additional data for force-closing channels. This field allows users to specify whether they want the backup to include inputs of the latest force close transaction, which can be crucial for recovering funds without counterparty cooperation.- 4506-4508: The addition of the
with_close_tx_inputsfield toChanBackupExportRequestaligns with the enhancement of SCBs. This change enables the inclusion of close transaction inputs in the backup, providing more comprehensive data for channel recovery processes.- 4550-4552: Including the
with_close_tx_inputsfield inChannelBackupSubscriptionis a valuable addition for subscribing to channel backup updates. This enables subscribers to receive backups that contain the inputs of the latest force close transaction, enhancing the robustness of the backup data.rpcserver.go (4)
- 7267-7267: The addition of
chanbackup.WithCloseTxInputs(in.WithCloseTxInputs)in theExportChannelBackupfunction correctly implements the feature to include close transaction inputs in the backup. This aligns with the PR's objective to enhance SCB functionality.- 7438-7438: Similarly, the inclusion of
chanbackup.WithCloseTxInputs(in.WithCloseTxInputs)in theExportAllChannelBackupsfunction is consistent with the PR's goal to improve backup robustness by allowing the inclusion of close transaction inputs.- 7538-7539: The introduction of the
closeTxvariable in theSubscribeChannelBackupsfunction to handle close transaction inputs is a thoughtful addition. It ensures that the functionality to include close transaction inputs is efficiently managed within the subscription logic.- 7570-7570: The use of
chanbackup.WithCloseTxInputs(closeTx)in theSubscribeChannelBackupsfunction's internal call toFetchStaticChanBackupsis appropriate and ensures that the close transaction inputs are included in the backups as per the user's request. This is a crucial part of implementing the new feature as described in the PR.lnwallet/channel.go (2)
- 6461-6488: The introduction of the
SignedCommitTxInputsstruct is a positive change for code modularity and clarity. It encapsulates all necessary inputs for signing a commitment transaction, which aligns well with the principles of clean code and separation of concerns. This change should make the codebase more maintainable and easier to understand for new contributors.- 6490-6500: The
TaprootSignedCommitTxInputsstruct is well-defined and serves a specific purpose for taproot channels, which is a good practice in terms of code organization and clarity. However, it's important to ensure that all necessary fields are included and accurately documented for future reference and maintenance.
|
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 lightninglabs/chantools#95 in file |
starius
force-pushed
the
close-tx-in-static-backup
branch
from
5854317
to
a8b55e2
Compare
|
@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 lightninglabs/chantools#95). |
There was a problem hiding this comment.
- ✅, successfully verified that also custom-channels are backuped correctly, see testdata
- dumpchanbackup is handled here: chantools scbforceclose: extract close tx from SCB and sign it lightninglabs/chantools#95 => must be merged when this PR is merged.
- @starius please open a tracking issue related to the timestamp.
Previous to this change taproot assets channels and simple taproot channels were considered the same in the context of chanbackup package, since they stored the same data. In the following commits we are adding the data needed to produce a signed commitment transaction from a SCB file and in order to do that we need to add more fields and a custom channel gets one additional field (TapscriptRoot) compared to a simple taproot channel. So now we have to distinguish these kinds of channels in chanbackup package. See PR lightningnetwork#8183 for more details.
starius
force-pushed
the
close-tx-in-static-backup
branch
from
fb44fd6
to
621a963
Compare
|
I added small commit "itest: test channel.backup changes upon shutdown" in which I add a check in SCB itest that channel.backup file content changes upon node shutdown. This is needed to make sure that this feature actually works. Just few lines of code in itest, no new dependencies. |
|
@ziggie1984 |
| // Shutdown to keep its directory including channel.backup file. | ||
| ht.SuspendNode(dave) | ||
|
|
||
| // Read Dave's channel.backup file again to make sure it was updated |
There was a problem hiding this comment.
Could you add in the comment why it needs to be updated, it will only update if the state changes, otherwise the closeInput info also remains the same ?
There was a problem hiding this comment.
Done.
// Read Dave's channel.backup file again to make sure it was updated
// upon Dave's shutdown. In case LND state is lost and DLP protocol
// fails, the channel.backup file and the commit tx in it are the
// measure of last resort to recover funds from the channel. The file
// is updated upon LND server shutdown to update the commit tx just in
// case it is used this way. If an outdated commit tx is broadcasted,
// the funds may be lost in a justice transaction. The file is encrypted
// and we can't decrypt it here, so we just check that the content of
// the file has changed.
multi2, err := os.ReadFile(backupFilePath)
require.NoError(ht, err)
require.NotEqual(ht, multi, multi2)
it will only update if the state changes, otherwise the closeInput info also remains the same
The file is encrypted and it changes even if the content is the same.
We can't decrypt it in itest to check closeInputs (this would bring hdkeyring.go dependency which we don't want in this PR).
So I just checked that the file content has changed -- this means ManualUpdate was at least triggered.
starius
force-pushed
the
close-tx-in-static-backup
branch
from
621a963
to
42b0cbb
Compare
|
@yyforyongyu: review reminder |
| // output paying to us, in the case that the remote party broadcasts their | ||
| // version of the commitment transaction. We can sweep this output immediately, | ||
| // as it doesn't have a time-lock delay. | ||
| // output paying to us (local channel balance). In the case that the local |
| case inputs.Taproot.IsSome(): | ||
| // Extract Taproot from fn.Option. It is safe to call | ||
| // UnsafeFromSome because we just checked that it is some. | ||
| taproot := inputs.Taproot.UnsafeFromSome() |
There was a problem hiding this comment.
non-blocking: we prolly need to move this method to a _dev.go file and only build it when it's in a test env, otherwise it's very tempting to use this API cc @ProofOfKeags
There was a problem hiding this comment.
It would be great to have Unwrap() (T, bool) method, then this code could be rewritten as:
if taproot, has := inputs.Taproot.Unwrap(); has {
...
chanbackup/backup.go
Outdated
| // in loss of funds! This may happen if an outdated channel backup is attempted | ||
| // to be used to force close the channel. | ||
| func buildCloseTxInputs(targetChan *channeldb.OpenChannel, | ||
| ) fn.Option[CloseTxInputs] { |
There was a problem hiding this comment.
line wrapping needs to be fixed, reference
chanbackup/single.go
Outdated
| return encoded | ||
| } | ||
|
|
||
| // Decode decodes the encoding of the version from a channel backup. |
| if err != nil { | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
nit: could do !s.Version.IsTaproot() to save some indentations here
There was a problem hiding this comment.
Done. Also added a comment to explain what happens after the check:
if !s.Version.IsTaproot() {
return
}
// Write fields needed for taproot channels.
err = lnwire.WriteElements(
&singleBytes, inputs.CommitHeight,
)
if err != nil {
return
}
...
chanbackup/single.go
Outdated
| @@ -543,6 +685,47 @@ func (s *Single) Deserialize(r io.Reader) error { | |||
| } | |||
| } | |||
|
|
|||
| if hasCloseTx { | |||
There was a problem hiding this comment.
nit: same here, could do,
if !hasCloseTx {
return nil
}There was a problem hiding this comment.
Done. Also added a comment:
if !hasCloseTx {
return nil
}
// Deserialize CloseTxInputs if it is present in serialized data.
commitTx := &wire.MsgTx{}
if err := commitTx.Deserialize(r); err != nil {
return err
}
...
It used to be base64, which is not compatible with verifychanbackup, expecting hex.
It is used for sweeping time-locked outputs as well as non time-locked outputs.
This pure function creates signed commit transaction, using various inputs passed as struct TaprootSignedCommitTxInputs and a signer. This is needed to be able to store the inputs without a signature in SCB and sign the transaction in chantools scbforceclose. See https://github.com/lightningnetwork/lnd/pull/8183/files#r1423959791
Previous to this change taproot assets channels and simple taproot channels were considered the same in the context of chanbackup package, since they stored the same data. In the following commits we are adding the data needed to produce a signed commitment transaction from a SCB file and in order to do that we need to add more fields and a custom channel gets one additional field (TapscriptRoot) compared to a simple taproot channel. So now we have to distinguish these kinds of channels in chanbackup package. See PR lightningnetwork#8183 for more details.
starius
force-pushed
the
close-tx-in-static-backup
branch
from
42b0cbb
to
35a11cd
Compare
|
@yyforyongyu Thanks for review! I addressed the feedback. |
chanbackup/single.go
Outdated
|
|
||
| // Serialize CloseTxInputs if it is provided. Fill err if it fails. | ||
| var err error | ||
| s.CloseTxInputs.WhenSome(func(inputs CloseTxInputs) { |
There was a problem hiding this comment.
nit: We could use fn.MapOptionZ() here to allow returning an error, which would follow the intended "no mutations within callbacks" paradigm of the fn library:
// Serialize CloseTxInputs if it is provided. Fill err if it fails.
err := fn.MapOptionZ(s.CloseTxInputs, func(inputs CloseTxInputs) error {
err := inputs.CommitTx.Serialize(&singleBytes)
if err != nil {
return err
}
err = lnwire.WriteElements(
&singleBytes,
uint16(len(inputs.CommitSig)), inputs.CommitSig,
)
if err != nil {
return err
}
if !s.Version.IsTaproot() {
return nil
}
// Write fields needed for taproot channels.
err = lnwire.WriteElements(
&singleBytes, inputs.CommitHeight,
)
if err != nil {
return err
}
if s.Version.HasTapscriptRoot() {
opt := inputs.TapscriptRoot
var tapscriptRoot chainhash.Hash
tapscriptRoot, err = opt.UnwrapOrErr(
errEmptyTapscriptRoot,
)
if err != nil {
return err
}
err = lnwire.WriteElements(
&singleBytes, tapscriptRoot[:],
)
if err != nil {
return err
}
}
return nil
})
if err != nil {
return fmt.Errorf("failed to encode CloseTxInputs: %w", err)
}
There was a problem hiding this comment.
Applied the suggested fix. Thanks!
The field is optional. It stores inputs needed to produce signed commit tx using chantools scbforceclose, which calls function GetSignedCommitTx. New backups have this field filled if commit tx is available (for all cases except when DLP is active). If a backup has this data, the field is filled from it, otherwise it is kept empty. Modified test function genRandomOpenChannelShell to cover new types of channels (simple taproot channel and custom channel) and to cover combinations of bits. Make sure that TapscriptRoot field is properly packed and unpacked.
This method inserts channel updates and waits for them to be processed. It will be used to update channel.backup upon LND shutdown.
This is needed to keep channel.backup up-to-date if the node is stopped.
Also updated release-notes.
starius
force-pushed
the
close-tx-in-static-backup
branch
from
35a11cd
to
65df996
Compare
Change Description
In this PR I'm implementing the proposal from #7658 (comment)
I changed
chanbackupto addCloseTxInputsoptional field tochanbackup.Singlestructure. When the field is present, the version byte is XORed withcloseTxVersion = 128. When this bit is present in the version byte, the field is properly extracted when decoding packaged single backup.I added
chantools scbforceclosecommand to extract and sign closing tx. lightninglabs/chantools#95Implemented:
Steps to Test
I tested the following scenario on testnet:
lncli openchannellncli exportchanbackup --chan_point xxx:x --output_file single.backuplncli stopchantools --testnet scbforceclose --single_backup xxxlncli create. Use existing seed, recover onchain funds.lncli restorechanbackup --single_file single.backuplncli pendingchannels,total_limbo_balanceis equal to channel balance minus feesPull Request Checklist
Testing
Code Style and Documentation
[skip ci]in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.
Summary by CodeRabbit
BackupOptionparameters.SetupStandbyNodesfunction.LightningChannelto use a more modular approach for signing commitment transactions.