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). |
itest/hdkeychain.go
Outdated
| return currentKey, nil | ||
| } | ||
|
|
||
| type hdKeyRing struct { |
There was a problem hiding this comment.
why do we need this hdKeyRing? By searching DeriveNextKey it seems we've already had several implementations already, can we use one of those instead? Also isn't there an RPC service RPCKeyRing we can use?
There was a problem hiding this comment.
Can you elaborate on this, please? I can't find a suitable in-memory implementation of keychain.KeyRing, keychain.ECDHRing, and input.Signer (the types I need in SignCloseTx) which could be created from seed and passphrase.
To create RPCKeyRing, the following objects are needed: keychain.SecretKeyRing, lnwallet.WalletController, and *lncfg.RemoteSigner. I don't know how to make them from seed and passphrase.
I copy-pasted these two types from chantools. If there is a better way to create them, we can consider using that approach in chantools as well.
itest/hdsigner.go
Outdated
| "github.com/lightningnetwork/lnd/keychain" | ||
| ) | ||
|
|
||
| type hdSigner struct { |
There was a problem hiding this comment.
same question re this, why do we need another input.Signer?
There was a problem hiding this comment.
Same as above: I don't know how to create input.Signer from seed and passphrase using existing types :)
itest/lnd_channel_backup_test.go
Outdated
| // Use the latest SCB to produce force close tx. | ||
| scbForCloseTx = backupAfterShutdown | ||
| } | ||
| crs.forceCloseTx, err = chanbackup.SignCloseTx( |
There was a problem hiding this comment.
Instead of creating key rings and signers outside of the SignCloseTx, I think it should be handled inside the method. This way we only need to pass scbForCloseTx and mnemonic and password, and the chanbackup handles the creation of signers and king rings.
Also by looking at this method alone it's not an RPC point? If it's a method meant to be imported by other packages to be used, I'm not sure if it should be tested in integration tests.
There was a problem hiding this comment.
If we make it too easy for the user to make signed transactions, they are more likely to misuse the feature and get funds lost. Currently it is not trivial to use the function outside of chantools, where we warn the user many times. The implementations of dependencies (keychain.KeyRing, keychain.ECDHRing, and input.Signer) live as private types in itest and are not directly usable from outside.
This function is intended to be used from chantools only. We don't have chantools in itest, so the only way of emulating using chantools in itest is to call the function directly. The code in chantools is really a wrapper around this function: lightninglabs/chantools@1fe8248
There was a problem hiding this comment.
In that case we should add unit tests for this method, not itest. You can import the lntest package in chantools to build the exact same test, and we should not use the itest in lnd to test programs importing it.
itest/lnd_channel_backup_test.go
Outdated
| ht.EnsureConnected(carol, dave) | ||
|
|
||
| // Wait for Carol to publish a penalty transaction. | ||
| require.Eventually(ht, func() bool { |
There was a problem hiding this comment.
I think MineBlocksAndAssertNumTxes already waits the tx?
There was a problem hiding this comment.
Great catch!
I added require.Eventually because I got a failure in CI in ht.MineBlocksAndAssertNumTxes(1, 1) call (it got 0 txs), I wrote a comment about it and thought mistakenly that MineBlocksAndAssertNumTxes doesn't wait.
I checked the code of MineBlocksAndAssertNumTxes and AssertNumTxsInMempool and now I see that it was not the reason of that failure, because AssertNumTxsInMempool waits for 1 minute. I don't know why there were no transactions in mempool in that run of the test. Penalty transaction has to be there... I'll try to undo require.Eventually and reproduce it locally and debug.
There was a problem hiding this comment.
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).
However I can't reproduce the original failure locally. For some reason Carol node didn't issue a justice transaction in that run in CI. Do you have any ideas why it could happen?
starius
force-pushed
the
close-tx-in-static-backup
branch
from
e057dc7
to
b9a5202
Compare
|
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". |
There was a problem hiding this comment.
Great work on this feature, definitely important work 👌
Well thought through code design only had minor comment on this one.
Regarding the itest however I am in doubt whether we need this test in general because it introduces a lot of new code regarding the signer, keyring etc. Moreover I think most of the tests here are already covered in other itests, for example the sweeping logic or the penalty case. Would it maybe possible to move all this tests into the chantools repo instead ?
I want to outline that these itests definiitly took a lot of work and are very detailed in general so kudos for this work 💪. I am brining this discussion up because when for example changing the anchor sweeper logic we realised that we had to change the itests in so many different places which took a lot of work and IMO was quite unnecessary. Thats why I want to avoid to many itests which basically test the same thing.
lnwallet/channel.go
Outdated
| SignDesc *input.SignDescriptor | ||
|
|
||
| // Taproot holds fields needed in case of a taproot channel. | ||
| // If and only if the channel is of taproot type, this field is filled. |
There was a problem hiding this comment.
Nit: Iff the channel => instead of (If and only if )
lnwallet/channel.go
Outdated
| CommitSig []byte | ||
|
|
||
| // OurKey is our key to be used within the 2-of-2 output script | ||
| // for the owner of this channel config. |
There was a problem hiding this comment.
Nit: what do you mean by this channel config ? Maybe cpy&paste relict ?
There was a problem hiding this comment.
Yes, this was a copy-paste relict. Fixed.
| // the total number of commitment updates to this point. This can be | ||
| // viewed as sort of a "commitment height" as this number is | ||
| // monotonically increasing. | ||
| CommitHeight uint64 |
There was a problem hiding this comment.
Q: why do we need the commitment height only for taproot channels only ? Probably to find the Nonce in the shachain ?
There was a problem hiding this comment.
Yes. I added a comment in the code:
// CommitHeight is the update number that this ChannelDelta represents
// the total number of commitment updates to this point. This can be
// viewed as sort of a "commitment height" as this number is
// monotonically increasing. This number is used to make a signature
// for a taproot channel, since it is used by shachain nonce producer
// (TaprootNonceProducer).
CommitHeight uint64| @@ -6473,29 +6473,72 @@ func (lc *LightningChannel) AbsoluteThawHeight() (uint32, error) { | |||
| return lc.channelState.AbsoluteThawHeight() | |||
| } | |||
|
|
|||
| // getSignedCommitTx function take the latest commitment transaction and | |||
There was a problem hiding this comment.
not sure I understand the comment, but don't we need to store "TheirSig" in the SCB ? In CloseTxInputs ?
There was a problem hiding this comment.
We store their part of the signature in the field CommitSig.
| localCommit := lc.channelState.LocalCommitment | ||
|
|
||
| inputs := SignedCommitTxInputs{ | ||
| CommitTx: localCommit.CommitTx, |
There was a problem hiding this comment.
I wonder if it is considered good design in general to pass pointer in a function which alter the underlying value of the pointer ? Not referring to this case here but more a general question, because I think we should always discourage this or are there some advantages to it ?
itest/lnd_channel_backup_test.go
Outdated
| // let's start up Carol again. | ||
| require.NoError(ht, restartCarol(), "restart carol failed") | ||
| if c.forceCloseTx == nil { | ||
| // DLP case. |
There was a problem hiding this comment.
Nit: Move the comment above the if clause
itest/lnd_channel_backup_test.go
Outdated
|
|
||
| // Upon reconnection, the nodes should detect that Dave is out of sync. | ||
| // Carol should force close the channel using her latest commitment. | ||
| // For scbforceclose case: we detect the manually broadcasts Dave's tx. |
There was a problem hiding this comment.
According to the test it looks to me that assertFundsRecoveredStartCarol is only called in the scbforceclose case ?
There was a problem hiding this comment.
Fixed. It was a relict of my original approach trying to have a single "assert" function for all cases. The function became too branched and complicated and I split it into separate "assert" functions, but forgot to update this comment.
itest/lnd_channel_backup_test.go
Outdated
| // are uneconomical. | ||
| ht.AssertNumPendingSweeps(dave, 1) | ||
|
|
||
| // Carol has two sweeps. |
There was a problem hiding this comment.
can you add which sweep Carol has here in addition to the anchor ?
There was a problem hiding this comment.
Done.
// Carol has two sweeps: an anchor and her balance from the
// channel. Carol's funds are available immediately, as they are
// not timelocked.
itest/lnd_channel_backup_test.go
Outdated
| // After Carol's output matures, she should also reclaim her | ||
| // funds. | ||
| // | ||
| // The commit sweep resolver publishes the sweep tx at | ||
| // defaultCSV-1 and we already mined one block after the | ||
| // commitmment was published, so take that into account. | ||
| ht.MineEmptyBlocks(int(defaultCSV - blocksMined)) | ||
|
|
||
| // Carol should have two sweep requests - one for her commit | ||
| // output and the other for her anchor. | ||
| ht.AssertNumPendingSweeps(carol, 2) | ||
|
|
||
| // Mine a block to trigger the sweep in Carol. | ||
| ht.MineEmptyBlocks(1) | ||
| ht.MineBlocksAndAssertNumTxes(1, 1) | ||
|
|
||
| // Now the channel should be fully closed also from | ||
| // Carol's POV. | ||
| ht.AssertNumPendingForceClose(carol, 0) | ||
|
|
||
| // We'll now mine the remaining blocks to prompt Dave to sweep | ||
| // his CLTV-constrained output. | ||
| resp := dave.RPC.PendingChannels() | ||
| blocksTilMaturity := | ||
| resp.PendingForceClosingChannels[0].BlocksTilMaturity | ||
| require.Positive(ht, blocksTilMaturity) | ||
|
|
||
| ht.MineEmptyBlocks(int(blocksTilMaturity)) | ||
|
|
||
| // Dave should have two sweep requests - one for his commit | ||
| // output and the other for his anchor. | ||
| ht.AssertNumPendingSweeps(dave, 2) | ||
|
|
||
| // Mine a block to trigger the sweep. | ||
| ht.MineEmptyBlocks(1) | ||
| ht.MineBlocksAndAssertNumTxes(1, 1) | ||
|
|
||
| // Now Dave should consider the channel fully closed. | ||
| ht.AssertNumPendingForceClose(dave, 0) |
There was a problem hiding this comment.
I think there is no need to test the sweeper here again imo, checking whether the sweeps where requested is sufficient enough.
There was a problem hiding this comment.
Fixed. Removed extra checks.
itest/lnd_channel_backup_test.go
Outdated
| } else { | ||
| // Carol should sweep her funds immediately, as they are not | ||
| // timelocked. We also expect Carol and Dave sweep their anchors | ||
| // if it's an anchor channel. Dave's funds are timelocked, so | ||
| // expect just one sweep. | ||
| if lntest.CommitTypeHasAnchors(commitType) { | ||
| ht.AssertNumPendingSweeps(carol, 2) | ||
| } else { | ||
| ht.AssertNumPendingSweeps(carol, 1) | ||
| } | ||
| ht.AssertNumPendingSweeps(dave, 1) | ||
|
|
||
| // Mine one block to trigger the sweeper to sweep. | ||
| ht.MineEmptyBlocks(1) | ||
| blocksMined++ | ||
|
|
||
| // Expect one tx - the commitment sweep from Carol. For | ||
| // anchor channels, we expect the two anchor sweeping | ||
| // txns to be failed due they are uneconomical. | ||
| ht.MineBlocksAndAssertNumTxes(1, 1) | ||
| blocksMined++ | ||
|
|
||
| // Now Carol should consider the channel fully closed. | ||
| ht.AssertNumPendingForceClose(carol, 0) | ||
|
|
||
| // After Dave's output matures, he should also reclaim his | ||
| // funds. | ||
| // | ||
| // The commit sweep resolver publishes the sweep tx at | ||
| // defaultCSV-1 and we already have blocks mined after the | ||
| // commitmment was published, so take that into account. | ||
| ht.MineEmptyBlocks(int(defaultCSV - blocksMined)) | ||
|
|
||
| // Mine one block to trigger the sweeper to sweep. | ||
| ht.MineEmptyBlocks(1) | ||
|
|
||
| // Dave's sweeper swept the outputs of force close tx. | ||
| if lntest.CommitTypeHasAnchors(commitType) { | ||
| ht.AssertNumPendingSweeps(dave, 2) | ||
| } else { | ||
| ht.AssertNumPendingSweeps(dave, 1) | ||
| } | ||
|
|
||
| // Assert the sweeping tx is mined. | ||
| ht.MineBlocksAndAssertNumTxes(1, 1) | ||
|
|
||
| // Funds are now in Dave's wallet. | ||
| ht.AssertNumPendingForceClose(dave, 0) | ||
|
|
||
| // Now the channel should be fully closed also from | ||
| // Carol's POV. | ||
| ht.AssertNumPendingForceClose(carol, 0) |
There was a problem hiding this comment.
Same here, I think we can reduce the test quite a bit, because if something changes for example with the anchor sweep all those tests would need to be changed, so I think we should not repeat tests wdyt ?
|
Would love to see a follow-up PR with the HTLC-Sigs included for the outgoing HTLCs 😊 |
There was a problem hiding this comment.
If we decide to support the feature of signing close tx in lnd, then we should make it into a RPC endpoint. If we decide to only make it a method that can be imported by other projects, i.e., the SignCloseTx method introduced here, then it should be only affecting the module chanbackup (ideally we should package it but that's beyond the scope), which means we should write unit tests, not itest.
We can easily duplicate the itest written in this PR since lntest is made for that. This is also how other services like loop/naut build their tests, not the other way around.
itest/lnd_channel_backup_test.go
Outdated
| // Use the latest SCB to produce force close tx. | ||
| scbForCloseTx = backupAfterShutdown | ||
| } | ||
| crs.forceCloseTx, err = chanbackup.SignCloseTx( |
There was a problem hiding this comment.
In that case we should add unit tests for this method, not itest. You can import the lntest package in chantools to build the exact same test, and we should not use the itest in lnd to test programs importing it.
|
@starius, remember to re-request review from reviewers when ready |
starius
force-pushed
the
close-tx-in-static-backup
branch
from
b9a5202
to
9d65000
Compare
|
I rebased the PR, made I addressed @ziggie1984 's feedback, in particular used fn.Option for optional parts of the backup and removed includeCloseTxInputs from |
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
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.
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
9d65000
to
9484522
Compare
|
@yyforyongyu I removed function SignCloseTx and the itest from this PR. Please take another look! |
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.