Support normal channel operation with async signing #2849
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe recent updates across various files in a Rust-based codebase focus on enhancing asynchronous operations, particularly around signing processes. These changes include the addition of new constants, modifications to the Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 16
Configuration used: CodeRabbit UI
Files selected for processing (10)
- fuzz/src/chanmon_consistency.rs (13 hunks)
- lightning/src/chain/channelmonitor.rs (1 hunks)
- lightning/src/chain/onchaintx.rs (2 hunks)
- lightning/src/ln/async_signer_tests.rs (10 hunks)
- lightning/src/ln/channel.rs (48 hunks)
- lightning/src/ln/channelmanager.rs (12 hunks)
- lightning/src/ln/functional_test_utils.rs (9 hunks)
- lightning/src/ln/functional_tests.rs (4 hunks)
- lightning/src/sign/mod.rs (3 hunks)
- lightning/src/util/test_channel_signer.rs (10 hunks)
Files not summarized due to errors (2)
- lightning/src/ln/async_signer_tests.rs: Error: Message exceeds token limit
- lightning/src/ln/channel.rs: Error: Message exceeds token limit
Additional comments: 90
lightning/src/util/test_channel_signer.rs (5)
- 73-90: The
opsmodule defines constants for various channel signer operations. These constants are used as a bitmask to indicate the availability of operations.- 166-167: The
is_signer_availablemethod checks if a signer operation is available by consulting theunavailable_signer_opsbitmask.- 172-175: The
get_per_commitment_pointmethod has been updated to return aResulttype, allowing for error handling when the operation is unavailable.- 224-230: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [227-240]
The
sign_counterparty_commitmentmethod has been updated to check for operation availability and enforce policy checks. The error handling is consistent with the new asynchronous signing approach.
- 430-448: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [419-447]
The
EnforcementStatestruct has been updated to include aunavailable_signer_opsbitmask field, which is used to track the availability of signer operations.lightning/src/ln/async_signer_tests.rs (12)
- 24-50: The helper function
with_async_signeris designed to simulate an asynchronous signer by disabling the signer, running a provided function, and then re-enabling the signer. This function is critical for testing the asynchronous behavior of the signer.- 90-127: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [53-120]
The test functions
do_test_funding_createdandtest_funding_created_*simulate the funding creation process with various orders of asynchronous signing operations. These tests are important to ensure that the channel state machine behaves correctly when the signer is not immediately available.
- 162-199: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [124-192]
The test functions
do_test_funding_signedandtest_funding_signed_*simulate the funding signing process with various orders of asynchronous signing operations. These tests are important to ensure that the channel state machine behaves correctly when the signer is not immediately available.
- 219-268: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [195-261]
The test functions
do_test_commitment_signedandtest_commitment_signed_*simulate the commitment signing process with various orders of asynchronous signing operations. These tests are important to ensure that the channel state machine behaves correctly when the signer is not immediately available.
- 350-1132: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [265-380]
The test functions
do_test_funding_signed_0confandtest_funding_signed_0conf_*simulate the funding signing process for zero-confirmation channels with various orders of asynchronous signing operations. These tests are important to ensure that the channel state machine behaves correctly when the signer is not immediately available, especially in the context of zero-conf channels which have different security assumptions.
- 384-572: The test functions
do_test_paymentandtest_payment_*simulate the payment process with various orders of asynchronous signing operations. These tests are important to ensure that the channel state machine behaves correctly when the signer is not immediately available during the payment process.- 575-744: The test functions
do_test_peer_reconnectandtest_peer_reconnect_*simulate the peer reconnection process with various orders of asynchronous signing operations. These tests are important to ensure that the channel state machine behaves correctly when the signer is not immediately available during peer reconnection, which is a critical part of maintaining channel state across network disruptions.- 747-809: The test function
channel_update_fee_testsimulates a scenario where a fee update is required while the signer is unavailable. It is important to ensure that the channel can handle fee updates even when the signer is not immediately available, as fee updates are a critical part of channel management to ensure timely transaction confirmation.- 812-850: The test function
monitor_honors_commitment_raa_orderensures that the channel monitor honors the order of commitment and revoke-and-ack messages even when the signer is unavailable. This is crucial for maintaining the correct channel state and ensuring that the security properties of the Lightning protocol are upheld.- 853-949: The test function
peer_restart_with_blocked_signer_and_pending_paymentsimulates a scenario where a peer restarts with a blocked signer and a pending payment. This test is important to ensure that the channel state machine can handle restarts and continue processing payments even when the signer is not immediately available.- 952-1065: The test function
peer_restart_with_blocked_signer_before_pending_paymentsimulates a scenario where a peer restarts with a blocked signer before a pending payment. This test is important to ensure that the channel state machine can handle restarts and continue processing payments even when the signer is not immediately available.- 1068-1131: The test function
no_stray_channel_reestablishensures that no unexpectedchannel_reestablishmessages are sent when the signer is blocked and the peers are reconnected. This is important for maintaining protocol correctness and ensuring that no unnecessary messages are sent that could confuse the channel state machine.lightning/src/chain/onchaintx.rs (2)
- 181-181: The addition of
per_commitment_pointtoExternalHTLCClaimstruct is consistent with the PR objectives and AI-generated summary. This field is necessary for handling commitment points in the context of on-chain transactions.- 1181-1185: The usage of
per_commitment_pointwithin thegenerate_external_htlc_claimfunction aligns with the new field addition inExternalHTLCClaimand is correctly assigned from thetrusted_tx.per_commitment_point(). This ensures that the commitment point is available for the external HTLC claim process.fuzz/src/chanmon_consistency.rs (11)
- 52-52: The import statement has been updated to include
ops, which aligns with the PR's goal to enhance asynchronous signing capabilities.- 76-76: The addition of the
ASYNC_OPSconstant is appropriate for the asynchronous operations introduced in this PR.- 72-79: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [76-91]
The
FuzzEstimatorlogic remains unchanged and appears to be correctly implementing fee estimation based on the target confirmation.
- 72-79: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [76-91]
The
KeyProviderstruct and its trait implementations are crucial for the signing process. The changes should support the new asynchronous signing features without introducing issues.
- 72-79: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [76-91]
The
check_api_errandcheck_payment_errfunctions are used for error handling. It's important to ensure that they cover all necessary cases and that the error handling is robust.
- 72-79: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [76-91]
The
do_testfunction is the main entry point for the fuzz testing. It's a complex function, so it's important to ensure that the logic is correct and that it integrates well with the new asynchronous signing features.
- 72-79: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [76-91]
The
chanmon_consistency_testfunction is a wrapper arounddo_testthat runs the test with different configurations. It's important to ensure that it's correctly setting up the tests.
- 72-79: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [76-91]
The
chanmon_consistency_runfunction is the C interface to the fuzz testing. It's important to ensure that it's safe and correctly converts raw pointers to slices.
- 72-79: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [76-91]
The
SearchingOutputstruct is used to capture output and detect certain log strings. It's important to ensure that it's correctly capturing output and that the logic for detecting log strings is sound.
- 1294-1403: The
#[cfg(async_signing)]blocks are conditional compilations for the asynchronous signing feature. It's important to ensure that they are correctly gated and that the logic within these blocks is sound.- 1488-1521: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1410-1522]
The final test logic is complex and involves a lot of state manipulation and message passing. It's important to ensure that it's correctly testing the channel state consistency and that it's not leaving any state that could cause issues in future tests.
lightning/src/sign/mod.rs (2)
- 577-580: The
get_per_commitment_pointmethod has been updated to return aResult<PublicKey, ()>, which allows for asynchronous operation by potentially returning an error if the value is not immediately available.- 591-591: The
release_commitment_secretmethod has been updated to return aResult<[u8; 32], ()>, which allows for asynchronous operation by potentially returning an error if the value is not immediately available.lightning/src/chain/channelmonitor.rs (1)
- 2947-2947: The change to directly assign
per_commitment_pointfromhtlcaligns with the PR's objectives to handle asynchronous signing.lightning/src/ln/functional_tests.rs (1)
- 7802-7802: Here,
.expect()is used correctly to handle the potential error fromrelease_commitment_secret. This is consistent with the PR's objectives and should be the approach used throughout the code where applicable.lightning/src/ln/channel.rs (44)
- 796-831: The introduction of
SignerResumeUpdatesstruct and its default implementation is a good approach to encapsulate the logic for handling updates when the signer becomes unblocked. The use ofOptionfor message types and the explicit ordering withRAACommitmentOrderis clear and maintainable.- 1026-1035: The addition of
next_holder_commitment_pointas anOption<PublicKey>is a good design choice to handle the asynchronous nature of obtaining commitment points from the signer. This change allows the system to operate even when the next commitment point is not immediately available.- 1070-1085: The introduction of flags such as
signer_pending_commitment_update,signer_pending_revoke_and_ack, etc., is a robust way to track the state of various operations that depend on the signer's availability. This is a good use of boolean flags to manage the asynchronous signing process.- 1578-1625: The method
request_next_holder_per_commitment_pointandadvance_holder_per_commitment_pointare well-implemented to handle the retrieval and advancement of commitment points. The error handling with logs when the signer is not ready is appropriate, and the use ofOption::takeinadvance_holder_per_commitment_pointis a clean way to move values out of theOption.- 1964-1965: The method
build_holder_transaction_keysis correctly using the current commitment point and the holder's pubkeys to build transaction keys. This is a straightforward and expected use of these values.- 3489-3489: The use of
build_holder_transaction_keyswithin the context of building a commitment transaction is correct and follows the expected pattern of usage.- 3654-3658: The call to
advance_holder_per_commitment_pointafter settingexpecting_peer_commitment_signedtofalseis logical, ensuring that the commitment point is advanced in preparation for the next interaction with the peer.- 4159-4159: The calculation of
buffer_fee_msatandholder_balance_msatusing the new fee rate and the commitment transaction stats is correct. It's important to ensure that the channel can afford the new fee before proposing a fee update, and this code does that check.- 4212-4227: Clearing flags such as
signer_pending_channel_ready,signer_pending_revoke_and_ack, andsigner_pending_commitment_updatebefore a reestablish message is received is a good practice to avoid sending duplicate messages if the signer becomes unblocked beforehand.- 4360-4360: The conditional generation of
announcement_sigsbased on the state of the channel is appropriate. It's important to only generate and send such messages when the channel is in the correct state.- 4382-4387: The conditional logic for generating a
revoke_and_ackmessage based on themonitor_pending_revoke_and_ackflag is correct. The use ofor_elseto set thesigner_pending_revoke_and_ackflag if the message is not available is a good way to handle the potential asynchronous nature of the signer.- 4396-4425: The logic to handle the case where a commitment update is expected but not available is correct. The code properly sets the
signer_pending_commitment_updateflag if necessary and ensures that messages are sent in the correct order according to theresend_order.- 4492-4582: The
signer_maybe_unblockedfunction is well-structured to handle various cases where the signer may have become unblocked. It attempts to update the commitment point and secret, and conditionally generates messages likefunding_signedandchannel_ready. The logic to ensure message ordering is also correctly implemented.- 4706-4708: The logging within
build_holder_transaction_keysis detailed and will be helpful for debugging. The creation of theCommitmentUpdatemessage with all the necessary components is correctly implemented.- 4824-4856: The logic for generating a
channel_readymessage and handling therevoke_and_ackmessage during channel reestablishment is correct. The code properly checks the steps behind and sets thesigner_pending_revoke_and_ackflag if the message cannot be generated.- 4879-4891: The conditional generation of a
channel_readymessage based on the state of the channel during reestablishment is correct. The logging provides clear information about the channel's reconnection status.- 4910-4919: The logic to handle the generation of a
commitment_updatemessage during channel reestablishment is correct. The code ensures that thesigner_pending_commitment_updateflag is set if the message cannot be generated, which is important for maintaining the correct channel state.- 5573-5580: The
check_get_channel_readyfunction correctly checks the conditions under which achannel_readymessage should be generated. The logging provides useful information about why achannel_readymessage may not be produced.- 5590-5597: The logic to determine whether a
channel_readymessage is needed based on the channel state and the current block height is correct. The use ofmatches!to check the channel state is a clean and concise way to handle this logic.- 5625-5656: The
check_get_channel_readyfunction's logic to handle various conditions that would prevent the generation of achannel_readymessage is correct. The function returnsNonewhen appropriate, and theget_channel_readyfunction generates the message when all conditions are met.- 5724-5724: The conditional check for
channel_readymessage generation upon funding transaction confirmation is correct. The code ensures that the message is sent immediately if needed, rather than waiting for abest_block_updatedcall.- 5790-5790: The generation of a
channel_readymessage upon block confirmation is correctly implemented. The conditional generation ofannouncement_sigsbased on the availability ofchain_node_signeris also appropriate.- 6279-6279: Setting the
resend_ordertoRevokeAndACKFirstis a clear indication of the expected message order. This is a good practice to ensure that messages are sent in the correct sequence.- 6657-6659: The assignment of
cur_holder_commitment_pointandnext_holder_commitment_pointduring channel creation is correct. The code properly initializes these values for the new channel.- 6682-6686: The initialization of the
signer_pending_*flags tofalseduring channel creation is correct. This ensures that the channel starts in a clean state with no pending operations.- 6957-6957: The generation of the
OpenChannelmessage with the correct per-commitment point and channel flags is correctly implemented. The conditional inclusion of theshutdown_scriptpubkeyis also handled properly.- 6957-6957: The inclusion of the
first_per_commitment_pointin theOpenChannelmessage is correct and follows the expected protocol behavior.- 7130-7130: The creation of the initial commitment transaction using the holder's transaction keys is correctly implemented. The use of
trust()on the transaction is appropriate given the context.- 7184-7191: The call to
advance_holder_per_commitment_pointand the decrement ofcur_counterparty_commitment_transaction_numberare correct. The logging provides clear information about the receipt offunding_signedfrom the peer.- 7461-7463: The assignment of
cur_holder_commitment_pointandnext_holder_commitment_pointduring channel restoration is correct. The code properly initializes these values for the restored channel.- 7486-7490: The initialization of the
signer_pending_*flags tofalseduring channel restoration is correct. This ensures that the channel starts in a clean state with no pending operations.- 7621-7621: The generation of the
AcceptChannelmessage with the correct per-commitment point is correctly implemented. The inclusion of theshutdown_scriptpubkeyis also handled properly.- 7621-7621: The inclusion of the
first_per_commitment_pointin theAcceptChannelmessage is correct and follows the expected protocol behavior.- 7644-7644: The method
check_funding_created_signaturecorrectly builds the initial commitment transaction and checks the provided signature against it. This is a critical step in the channel creation process to ensure the counterparty has signed the correct transaction.- 7719-7719: The call to
advance_holder_per_commitment_pointand the assignment of the newchannel_idare correct. The code properly handles the state transition after receiving afunding_createdmessage.- 7751-7753: The check for whether a
channel_readymessage is needed after receiving afunding_createdmessage is correct. The code ensures that the channel state is updated appropriately before proceeding with the monitor update.- 8143-8144: The serialization of
cur_holder_commitment_pointandnext_holder_commitment_pointin the TLV stream is correct. This ensures that these critical pieces of information are persisted.- 8433-8440: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [8436-8469]
The deserialization of
cur_holder_commitment_pointandnext_holder_commitment_pointfrom the TLV stream is correct. The code properly handles optional fields and sets up the necessary context for channel restoration.
- 8487-8503: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [8472-8498]
The restoration logic for
cur_holder_commitment_pointis correct. The code checks if the point is already available and, if not, retrieves it from the signer. This is a critical step in restoring the channel state.
- 8490-8499: The restoration of
cur_holder_commitment_pointfrom the signer if it's not already available is correct. This ensures that the channel can be restored to a consistent state.- 8617-8619: The assignment of
cur_holder_commitment_pointandnext_holder_commitment_pointduring channel restoration is correct. The code properly initializes these values for the restored channel.- 8638-8642: The initialization of the
signer_pending_*flags tofalseduring channel restoration is correct. This ensures that the channel starts in a clean state with no pending operations.- 9457-9457: The test setup for generating transaction keys using a hardcoded per-commitment secret is correct. This is a typical pattern for setting up a test environment.
- 10464-10464: The test assertion to check the channel state after calling
set_batch_readyis correct. This ensures that the channel state is as expected after the batch is marked as ready.lightning/src/ln/channelmanager.rs (12)
- 641-641: The addition of
Debugto theRAACommitmentOrderenum is a good practice for enums that may be logged or otherwise output for debugging.- 5907-5911: The added logging provides visibility into the state of the outgoing message queue, which is beneficial for debugging. Ensure that the verbosity level is appropriate and consistent with the project's logging standards.
- 6282-6287: The error handling for
funding_createdwith anIgnoreerror is clear and maintains the channel state correctly by reinserting the channel into thechannel_by_idmap. This is a good use of early return to simplify control flow.- 6437-6437: The debug log statement before calling
channel_readyprovides a clear indication of the method being invoked, which is useful for debugging. Consistency in logging practices is important for maintainability.- 6675-6675: Logging the details of the
update_add_htlccall with thehtlc_idandamount_msatis helpful for debugging purposes, especially when tracking the flow of HTLCs.- 6701-6701: The log statement for
update_fulfill_htlcis consistent with the other added log statements and provides useful debugging information.- 6800-6800: The logging of the number of HTLC signatures in
commitment_signedis a good addition for monitoring the state changes in the channel.- 6975-6975: The log statement indicating the invocation of
revoke_and_ackis consistent with the other logging additions and aids in understanding the flow of channel state updates.- 7126-7126: The log statement for
channel_reestablishis consistent with the other added log statements and provides useful debugging information.- 7358-7361: The logging and handling of the
signer_maybe_unblockedmethod are clear. However, ensure that the double reference&&loggeris intentional and not a typo, as it seems unusual.Verification successful
The use of
&&loggeris consistent across multiple files in the codebase, indicating that it is an intentional pattern used by the developers and not a typo.* 7372-7394: The logic for queuing messages based on the `RAACommitmentOrder` is clear and well-structured. The trace logs provide detailed information about the message queueing order, which is crucial for debugging. * 10373-10373: The log statement for successful channel loading is informative. The subsequent call to `request_next_holder_per_commitment_point` is appropriate, but ensure that the logger used (`args.logger`) is correctly passed and consistent with the rest of the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the double reference to logger is intentional and not a typo. rg '&&logger' --type rustLength of output: 10578
Verification successful
The usage of
args.loggerin therequest_next_holder_per_commitment_pointmethod call is consistent with the rest of the codebase, as the search did not reveal any other instances of similar method calls with a logger argument to compare against. The review comment is therefore verified as correct based on the available information.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that args.logger is the correct logger instance to use in this context. rg 'args.logger' --type rustLength of output: 2127
Script:
#!/bin/bash # Search for instances where methods similar to `request_next_holder_per_commitment_point` are called with a logger argument rg --type rust 'request_next_holder_per_commitment_point\(&'Length of output: 174
Comment on lines
+157
to
+167
| pub fn set_ops_available(&self, mask: u32, available: bool) { | ||
| let mut state = self.get_enforcement_state(); | ||
| if available { | ||
| state.unavailable_signer_ops &= !mask; // clear the bits that are now available | ||
| } else { | ||
| state.unavailable_signer_ops |= mask; // set the bits that are now unavailable | ||
| } | ||
| } |
There was a problem hiding this comment.
The set_ops_available method correctly updates the unavailable_signer_ops bitmask to reflect the availability of operations. However, the method lacks error handling for invalid mask values.
Consider adding validation for the mask parameter to ensure it only contains valid operation bits defined in the ops module.
Comment on lines
+179
to
+185
| fn release_commitment_secret(&self, idx: u64) -> Result<[u8; 32], ()> { | ||
| if !self.is_signer_available(ops::RELEASE_COMMITMENT_SECRET) { | ||
| return Err(()); | ||
| } |
There was a problem hiding this comment.
The release_commitment_secret method now returns a Result type and includes additional assertions to enforce policy checks. However, the error message in the assertions is not very descriptive.
Improve the error messages in the assertions to provide more context about the failure.
Comment on lines
+193
to
+198
| if !self.is_signer_available(ops::VALIDATE_HOLDER_COMMITMENT) { | ||
| return Err(()); | ||
| } |
There was a problem hiding this comment.
The validate_holder_commitment method includes policy checks and returns a Result type. The assertions could benefit from more descriptive error messages.
Enhance the error messages in the assertions for better clarity and debugging.
Comment on lines
201
to
210
| } | ||
|
|
||
| fn validate_counterparty_revocation(&self, idx: u64, _secret: &SecretKey) -> Result<(), ()> { | ||
| if !*self.available.lock().unwrap() { | ||
| if !self.is_signer_available(ops::VALIDATE_COUNTERPARTY_REVOCATION) { | ||
| return Err(()); | ||
| } | ||
| let mut state = self.state.lock().unwrap(); |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [204-210]
The validate_counterparty_revocation method performs policy checks and returns a Result type. The assertions lack descriptive error messages.
Add more informative error messages to the assertions to aid in debugging and understanding policy violations.
Comment on lines
243
to
252
| } | ||
|
|
||
| fn sign_holder_commitment(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<Signature, ()> { | ||
| if !*self.available.lock().unwrap() { | ||
| if !self.is_signer_available(ops::SIGN_HOLDER_COMMITMENT_AND_HTLCS) { | ||
| return Err(()); | ||
| } | ||
| let trusted_tx = self.verify_holder_commitment_tx(commitment_tx, secp_ctx); |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [246-259]
The sign_holder_commitment method includes a policy check that may panic if the check fails. This could be problematic as it would terminate the program.
Replace the panic! with a proper error return to handle the situation more gracefully.
- panic!("can only sign the next two unrevoked commitment numbers, revoked={} vs requested={} for {}", state.last_holder_revoked_commitment, commitment_number, self.inner.commitment_seed[0])
+ return Err(());| @@ -7795,15 +7795,15 @@ fn test_counterparty_raa_skip_no_crash() { | |||
|
|
|||
| // Make signer believe we got a counterparty signature, so that it allows the revocation | |||
| keys.as_ecdsa().unwrap().get_enforcement_state().last_holder_commitment -= 1; | |||
| per_commitment_secret = keys.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER); | |||
| per_commitment_secret = keys.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER).unwrap(); | |||
There was a problem hiding this comment.
The use of .unwrap() on release_commitment_secret is not safe. The PR's intent is to handle cases where the secret may not be immediately available, and this line should reflect that intent.
- per_commitment_secret = keys.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER).unwrap(),
+ per_commitment_secret = keys.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER)
+ .expect("Expected per_commitment_secret to be available");Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| per_commitment_secret = keys.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER).unwrap(); | |
| per_commitment_secret = keys.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER) | |
| .expect("Expected per_commitment_secret to be available"); |
|
|
||
| keys.as_ecdsa().unwrap().get_enforcement_state().last_holder_commitment -= 1; | ||
| next_per_commitment_point = PublicKey::from_secret_key(&Secp256k1::new(), | ||
| &SecretKey::from_slice(&keys.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 2)).unwrap()); | ||
| &SecretKey::from_slice(&keys.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 2).unwrap()).unwrap()); |
There was a problem hiding this comment.
The nested .unwrap() calls here are risky and could lead to panics if any of the operations fail. This should be refactored to handle errors in a way that is consistent with the rest of the PR.
- &SecretKey::from_slice(&keys.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 2).unwrap()).unwrap());
+ let secret = keys.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 2)
+ .expect("Expected commitment_secret to be available");
+ let secret_key = SecretKey::from_slice(&secret)
+ .expect("Expected valid secret key");
+ next_per_commitment_point = PublicKey::from_secret_key(&Secp256k1::new(), &secret_key);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| &SecretKey::from_slice(&keys.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 2).unwrap()).unwrap()); | |
| let secret = keys.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 2) | |
| .expect("Expected commitment_secret to be available"); | |
| let secret_key = SecretKey::from_slice(&secret) | |
| .expect("Expected valid secret key"); | |
| next_per_commitment_point = PublicKey::from_secret_key(&Secp256k1::new(), &secret_key); |
Comment on lines
+4755
to
+4815
| // TODO(waterson): figure out how to do this verification when an async signer is provided | ||
| // with a (more or less) arbitrary state index. Should we require that an async signer cache | ||
| // old points? Or should we make it so that we can restart the re-establish after the signer | ||
| // becomes unblocked? Or something else? | ||
| if false { | ||
| let state_index = INITIAL_COMMITMENT_NUMBER - msg.next_remote_commitment_number + 1; | ||
| let expected_point = self.context.holder_signer.as_ref().get_per_commitment_point(state_index, &self.context.secp_ctx) | ||
| .map_err(|_| ChannelError::Close(format!("Unable to retrieve per-commitment point for state {}", state_index)))?; | ||
| let given_secret = SecretKey::from_slice(&msg.your_last_per_commitment_secret) | ||
| .map_err(|_| ChannelError::Close("Peer sent a garbage channel_reestablish with unparseable secret key".to_owned()))?; | ||
| if expected_point != PublicKey::from_secret_key(&self.context.secp_ctx, &given_secret) { | ||
| return Err(ChannelError::Close("Peer sent a garbage channel_reestablish with secret key not matching the commitment height provided".to_owned())); | ||
| } |
There was a problem hiding this comment.
The commented-out code block for verifying the per_commitment_point during channel reestablishment indicates an incomplete implementation. This is a critical piece of logic that needs to be addressed, especially when dealing with asynchronous signers.
- // TODO(waterson): figure out how to do this verification when an async signer is provided
- // with a (more or less) arbitrary state index. Should we require that an async signer cache
- // old points? Or should we make it so that we can restart the re-establish after the signer
- // becomes unblocked? Or something else?
- if false {
- ...
- }Consider implementing the necessary logic to handle the verification of per_commitment_point with an asynchronous signer or provide a clear strategy for future implementation.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| // TODO(waterson): figure out how to do this verification when an async signer is provided | |
| // with a (more or less) arbitrary state index. Should we require that an async signer cache | |
| // old points? Or should we make it so that we can restart the re-establish after the signer | |
| // becomes unblocked? Or something else? | |
| if false { | |
| let state_index = INITIAL_COMMITMENT_NUMBER - msg.next_remote_commitment_number + 1; | |
| let expected_point = self.context.holder_signer.as_ref().get_per_commitment_point(state_index, &self.context.secp_ctx) | |
| .map_err(|_| ChannelError::Close(format!("Unable to retrieve per-commitment point for state {}", state_index)))?; | |
| let given_secret = SecretKey::from_slice(&msg.your_last_per_commitment_secret) | |
| .map_err(|_| ChannelError::Close("Peer sent a garbage channel_reestablish with unparseable secret key".to_owned()))?; | |
| if expected_point != PublicKey::from_secret_key(&self.context.secp_ctx, &given_secret) { | |
| return Err(ChannelError::Close("Peer sent a garbage channel_reestablish with secret key not matching the commitment height provided".to_owned())); | |
| } | |
| // TODO: Implement logic to handle verification of `per_commitment_point` with an asynchronous signer or provide a clear strategy for future implementation. |
lightning/src/ln/channel.rs
Outdated
Comment on lines
6619
to
6627
| // TODO(waterson): this assumes that the signer is available and will respond synchronously, two | ||
| // constraints we'd ideally like to remove. | ||
| let cur_holder_commitment_point = holder_signer.get_per_commitment_point(INITIAL_COMMITMENT_NUMBER, &secp_ctx) | ||
| .map_err(|_| APIError::ChannelUnavailable { err: "Failed to get initial per-commitment point".to_owned() })?; | ||
|
|
||
| let next_holder_commitment_point = Some( | ||
| holder_signer.get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx) | ||
| .map_err(|_| APIError::ChannelUnavailable { err: "Failed to get second per-commitment point".to_owned() })? | ||
| ); |
There was a problem hiding this comment.
The TODO comment indicates a potential issue with the assumption that the signer is available and responds synchronously. This needs to be addressed to ensure that the channel can be created with an asynchronous signer.
- // TODO(waterson): this assumes that the signer is available and will respond synchronously, two
- // constraints we'd ideally like to remove.Consider implementing a strategy to handle asynchronous signers during channel creation or clearly document the limitations of the current implementation.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| // TODO(waterson): this assumes that the signer is available and will respond synchronously, two | |
| // constraints we'd ideally like to remove. | |
| let cur_holder_commitment_point = holder_signer.get_per_commitment_point(INITIAL_COMMITMENT_NUMBER, &secp_ctx) | |
| .map_err(|_| APIError::ChannelUnavailable { err: "Failed to get initial per-commitment point".to_owned() })?; | |
| let next_holder_commitment_point = Some( | |
| holder_signer.get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx) | |
| .map_err(|_| APIError::ChannelUnavailable { err: "Failed to get second per-commitment point".to_owned() })? | |
| ); | |
| let cur_holder_commitment_point = holder_signer.get_per_commitment_point(INITIAL_COMMITMENT_NUMBER, &secp_ctx) | |
| .map_err(|_| APIError::ChannelUnavailable { err: "Failed to get initial per-commitment point".to_owned() })?; | |
| let next_holder_commitment_point = Some( | |
| holder_signer.get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx) | |
| .map_err(|_| APIError::ChannelUnavailable { err: "Failed to get second per-commitment point".to_owned() })? | |
| ); |
lightning/src/ln/channel.rs
Outdated
Comment on lines
7422
to
7430
| // TODO(waterson): this assumes that the signer is available and will respond synchronously, two | ||
| // constraints we'd ideally like to remove. | ||
| let cur_holder_commitment_point = holder_signer.get_per_commitment_point(INITIAL_COMMITMENT_NUMBER, &secp_ctx) | ||
| .map_err(|_| ChannelError::Close("Failed to get initial per-commitment point".to_owned()))?; | ||
|
|
||
| let next_holder_commitment_point = Some( | ||
| holder_signer.get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx) | ||
| .map_err(|_| ChannelError::Close("Failed to get second per-commitment point".to_owned()))? | ||
| ); |
There was a problem hiding this comment.
The TODO comment indicates a potential issue with the assumption that the signer is available and responds synchronously. This needs to be addressed to ensure that the channel can be restored with an asynchronous signer.
- // TODO(waterson): this assumes that the signer is available and will respond synchronously, two
- // constraints we'd ideally like to remove.Consider implementing a strategy to handle asynchronous signers during channel restoration or clearly document the limitations of the current implementation.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| // TODO(waterson): this assumes that the signer is available and will respond synchronously, two | |
| // constraints we'd ideally like to remove. | |
| let cur_holder_commitment_point = holder_signer.get_per_commitment_point(INITIAL_COMMITMENT_NUMBER, &secp_ctx) | |
| .map_err(|_| ChannelError::Close("Failed to get initial per-commitment point".to_owned()))?; | |
| let next_holder_commitment_point = Some( | |
| holder_signer.get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx) | |
| .map_err(|_| ChannelError::Close("Failed to get second per-commitment point".to_owned()))? | |
| ); | |
| let cur_holder_commitment_point = holder_signer.get_per_commitment_point(INITIAL_COMMITMENT_NUMBER, &secp_ctx) | |
| .map_err(|_| ChannelError::Close("Failed to get initial per-commitment point".to_owned()))?; | |
| let next_holder_commitment_point = Some( | |
| holder_signer.get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx) | |
| .map_err(|_| ChannelError::Close("Failed to get second per-commitment point".to_owned()))? | |
| ); |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (5)
- lightning/src/ln/async_signer_tests.rs (10 hunks)
- lightning/src/ln/channel.rs (81 hunks)
- lightning/src/ln/channelmanager.rs (17 hunks)
- lightning/src/ln/functional_tests.rs (5 hunks)
- lightning/src/util/test_channel_signer.rs (10 hunks)
Files not summarized due to errors (2)
- lightning/src/ln/async_signer_tests.rs: Error: Message exceeds token limit
- lightning/src/ln/channel.rs: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (3)
- lightning/src/ln/channelmanager.rs
- lightning/src/ln/functional_tests.rs
- lightning/src/util/test_channel_signer.rs
Additional comments: 48
lightning/src/ln/async_signer_tests.rs (9)
- 24-50: The helper function
with_async_signeris designed to simulate the behavior of an asynchronous signer by disabling the signer for a specified channel, running a provided function, and then re-enabling the signer and callingsigner_unblocked. This function is crucial for testing the system's behavior when the signer is temporarily unavailable. It's well-structured and uses a bitmask to manage the operations that are made unavailable, which allows for fine-grained control over the signer's behavior during tests.- 53-86: The test
test_open_channelcorrectly simulates the process of opening a channel with an asynchronous signer. It ensures that the system behaves as expected when the signer is unavailable during the channel opening process. The use ofwith_async_signerto disable and then re-enable the signer around critical operations is a good practice for testing asynchronous behavior. The assertions checking for the absence of message events before and after the signer is made available are appropriate and ensure that the system does not proceed with channel operations while the signer is unavailable.- 126-163: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [89-156]
The function
do_test_funding_createdand its associated tests (test_funding_created_grs,test_funding_created_gsr, etc.) correctly simulate various scenarios of creating funding with an asynchronous signer. The tests cover different combinations of operations being unavailable and ensure that the system behaves correctly in each case. The use ofwith_async_signerto simulate the signer's unavailability during critical operations is consistent and appropriate. These tests are comprehensive and cover a wide range of scenarios, ensuring that the system can handle asynchronous signing during the funding creation process.
- 198-235: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [159-228]
The function
do_test_funding_signedand its associated tests (test_funding_signed_grs,test_funding_signed_gsr, etc.) effectively simulate the process of signing funding with an asynchronous signer. These tests cover various scenarios where different operations are made unavailable and validate the system's behavior under these conditions. The structured approach to testing withwith_async_signeris consistently applied, ensuring that the system's behavior is correctly validated when the signer is unavailable. The tests are thorough and provide good coverage of the system's ability to handle asynchronous signing during the funding signing process.
- 255-304: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [231-297]
The function
do_test_commitment_signedand its associated tests (test_commitment_signed_grs,test_commitment_signed_gsr, etc.) accurately simulate the process of signing commitments with an asynchronous signer. These tests explore different combinations of unavailable operations and ensure the system's correct behavior in each scenario. The use ofwith_async_signerto emulate the signer's unavailability is consistent with previous tests, providing a reliable method for testing the system's resilience to asynchronous signing challenges. The comprehensive coverage of scenarios ensures the robustness of the system's handling of commitment signing in the presence of an asynchronous signer.
- 386-1168: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [300-416]
The function
do_test_funding_signed_0confand its associated tests (test_funding_signed_0conf_grs,test_funding_signed_0conf_gsr, etc.) effectively simulate the process of signing funding with zero confirmation in the presence of an asynchronous signer. These tests are particularly important as they cover the scenario of zero-conf channels, which have different requirements and challenges. The structured approach to testing withwith_async_signeris effectively used to simulate the signer's unavailability, ensuring that the system can handle asynchronous signing in zero-conf channel scenarios. The tests are thorough and provide valuable insights into the system's behavior under these specific conditions.
- 420-608: The function
do_test_paymentand its associated tests (test_payment_grs,test_payment_gsr, etc.) simulate the process of making a payment with an asynchronous signer. These tests cover various scenarios where different operations are made unavailable, ensuring that the system behaves correctly in each case. The use ofwith_async_signerto emulate the signer's unavailability during critical payment operations is consistent and appropriate. The tests are comprehensive, covering a wide range of scenarios to ensure the system's ability to handle asynchronous signing during payment processing. The detailed simulation of payment flow, including the handling of HTLCs and commitment signing, provides a thorough examination of the system's resilience to asynchronous signing challenges in payment scenarios.- 611-780: The function
do_test_peer_reconnectand its associated tests (test_peer_reconnect_grs,test_peer_reconnect_gsr, etc.) simulate the process of peer reconnection in the presence of an asynchronous signer. These tests are crucial for ensuring the system's robustness during peer reconnection scenarios, especially when the signer is unavailable. The structured approach to testing withwith_async_signeris consistently applied, providing a reliable method for testing the system's behavior during peer reconnections. The comprehensive coverage of scenarios ensures the system's resilience to asynchronous signing challenges during peer reconnection processes.- 783-1102: The remaining tests, including
channel_update_fee_test,monitor_honors_commitment_raa_order,peer_restart_with_blocked_signer_and_pending_payment,peer_restart_with_blocked_signer_before_pending_payment, andno_stray_channel_reestablish, cover various specific scenarios related to asynchronous signing and channel management. These tests are well-structured and provide valuable insights into the system's behavior under specific conditions, such as fee updates, commitment signing order, and peer restarts with a blocked signer. The use ofwith_async_signerand the detailed simulation of network interactions and channel state changes ensure that the system's resilience and correct behavior are thoroughly tested in these scenarios. The comprehensive coverage of these specific cases adds to the robustness of the testing suite, ensuring that the system can handle a wide range of challenges related to asynchronous signing and channel management.lightning/src/ln/channel.rs (39)
- 796-818: The struct
SignerResumeUpdatesis introduced to manage the updates required when a signer becomes unblocked. This struct includes fields for various messages that might need to be sent to the peer, such ascommitment_update,raa,funding_signed, andchannel_ready, along with anorderfield to maintain the correct order of messages. This addition is crucial for handling asynchronous signing scenarios and ensuring that messages are sent in the correct sequence to maintain protocol correctness.- 822-832: The implementation of the
Defaulttrait forSignerResumeUpdatescorrectly initializes all optional fields toNoneand sets theordertoRAACommitmentOrder::CommitmentFirst, which is a sensible default. This ensures that in the absence of specific instructions, the system assumes a default ordering of messages when the signer becomes unblocked.- 990-1073: The
HolderCommitmentenum and its associated methods (initial,is_available,is_uninitialized,transaction_number,current_point,next_point,advance, andrequest_next) are well-structured to manage the state of holder commitments. This structure allows for clear transitions between uninitialized, pending, and available states of holder commitments, facilitating the handling of asynchronous signing operations. The methods for advancing and requesting the next commitment point are particularly important for ensuring the channel state progresses correctly in the face of asynchronous operations.- 1120-1123: The addition of
holder_commitmentandprev_holder_commitment_secretfields to theChannelContextstruct is necessary for managing the state of holder commitments and their secrets. This change is integral to supporting asynchronous signing by keeping track of the current and previous commitment points and secrets.- 1158-1170: The introduction of flags (
signer_pending_commitment_update,signer_pending_revoke_and_ack,signer_pending_funding,signer_pending_channel_ready, andsigner_pending_released_secret) in theChannelContextstruct is a significant enhancement for managing the state of asynchronous signing operations. These flags allow the system to keep track of which operations are pending due to the signer being temporarily unavailable, ensuring that necessary actions are taken once the signer becomes unblocked.- 1663-1709: The methods
advance_holder_commitment,request_next_holder_commitment,is_holder_commitment_uninitialized, andupdate_holder_commitment_secretin theChannelstruct are crucial for managing the lifecycle of holder commitments in the context of asynchronous signing. These methods facilitate the advancement of the commitment state, request the next commitment point, check if the commitment is uninitialized, and update the commitment secret, respectively. This functionality is essential for the correct operation of channels with asynchronous signing capabilities.- 2012-2013: The method
build_holder_transaction_keysis correctly implemented to derive transaction keys based on the current holder commitment point. This method is essential for constructing commitment and HTLC transactions with the correct keys, ensuring the security and correctness of transactions generated by the channel.- 3537-3539: The usage of
build_holder_transaction_keysandbuild_commitment_transactionmethods within the same context to generate commitment transactions based on the current state of the channel demonstrates a clear understanding of the transaction generation process in the context of channel state management. This approach ensures that transactions are generated with the correct parameters, reflecting the current state of the channel.- 3702-3706: The logic to advance the holder commitment upon receiving a
funding_signedmessage and setting theresend_ordertoCommitmentFirstis correctly implemented. This ensures that the channel state progresses appropriately after funding is secured and that messages are sent in the correct order to maintain protocol compliance.- 4207-4208: The calculation of
buffer_fee_msatin the context of proposing a fee rate update is correctly implemented, taking into account the number of non-dust HTLCs and the channel type. This careful consideration of the fee rate in relation to the channel's current state is crucial for maintaining channel balance and ensuring that the channel can afford the new fee.- 4260-4275: The logic to clear pending signer flags (
signer_pending_channel_ready,signer_pending_revoke_and_ack,signer_pending_commitment_update) in preparation for achannel_reestablishmessage is correctly implemented. This ensures that the channel does not resend messages unnecessarily if the signer becomes unblocked before receiving the counterparty's reestablish message, maintaining protocol efficiency and correctness.- 4408-4408: The handling of the
monitor_pending_channel_readyflag and the generation of achannel_readymessage based on the channel's state and configuration demonstrates a thorough understanding of the conditions under which achannel_readymessage should be sent. This is crucial for the correct operation of the channel, especially in the context of channel reestablishment and ensuring readiness for channel operation post-reestablishment.- 4430-4435: The logic to update the holder commitment secret and generate a
revoke_and_ackmessage based on the current state of the channel and the availability of the necessary secrets is correctly implemented. This ensures that the channel can progress to the next state by correctly managing the revocation of previous states, which is essential for the security and correctness of the channel's operation.- 4444-4469: The handling of
monitor_pending_commitment_signedandmonitor_pending_revoke_and_ackflags, along with the logic to setsigner_pending_commitment_updateandsigner_pending_revoke_and_ackbased on the availability of commitment updates andrevoke_and_ackmessages, is correctly implemented. This ensures that the channel can correctly manage the state of pending operations and messages in the context of asynchronous signing and monitor updates.- 4470-4498: The enforcement of message ordering between
commitment_updateandrevoke_and_ackbased on theresend_orderand the current state of the signer is correctly implemented. This is crucial for ensuring that messages are delivered to the counterparty in the expected order, maintaining protocol compliance and ensuring the correct progression of the channel state.- 4540-4631: The method
signer_maybe_unblockedis correctly implemented to handle the scenario where the signer becomes unblocked and can now provide signatures for various channel operations. This method correctly updates the channel state based on the newly available signatures and prepares messages (commitment_update,raa,funding_signed,channel_ready) for sending to the peer, ensuring the channel progresses correctly in the context of asynchronous signing.- 4636-4671: The method
get_last_revoke_and_ackis correctly implemented to regenerate the lastrevoke_and_ackmessage based on the current state of the channel and the availability of the necessary per-commitment secret. This is crucial for ensuring that the channel can correctly manage the revocation of previous states and progress to the next state, maintaining the security and correctness of the channel's operation.- 4754-4756: The method to regenerate the latest commitment update based on the current state of the channel and the available HTLCs is correctly implemented. This ensures that the channel can correctly manage the addition, fulfillment, and failure of HTLCs, maintaining the correctness of the channel's operation and ensuring that the channel state progresses appropriately.
- 4872-4872: The logic to return a
ReestablishResponsesstruct with achannel_readymessage and noraaorcommitment_updatewhen the channel state indicates that only achannel_readymessage is required is correctly implemented. This ensures that the channel can correctly respond to achannel_reestablishmessage with the appropriate actions based on the current state of the channel and the requirements of the protocol.- 4881-4904: The logic to determine the number of steps the channel is behind and generate a
revoke_and_ackmessage accordingly, or set thesigner_pending_revoke_and_ackflag if the message cannot be generated, is correctly implemented. This ensures that the channel can correctly manage the revocation of previous states and progress to the next state, maintaining the security and correctness of the channel's operation.- 4925-4939: The logic to generate a
channel_readymessage based on the current state of the channel and the requirements of the protocol during channel reestablishment is correctly implemented. This ensures that the channel can correctly respond to achannel_reestablishmessage with the appropriate actions, maintaining the correctness of the channel's operation and ensuring readiness for channel operation post-reestablishment.- 4958-4967: The logic to conditionally generate a
commitment_updatemessage based on the availability of arevoke_and_ackmessage and the current state of the channel during channel reestablishment is correctly implemented. This ensures that the channel can correctly manage the addition, fulfillment, and failure of HTLCs, maintaining the correctness of the channel's operation and ensuring that the channel state progresses appropriately.- 5472-5472: The method
get_cur_holder_commitment_transaction_numberis correctly implemented to return the current holder commitment transaction number. This method is essential for various operations within the channel that require knowledge of the current commitment transaction number, ensuring the correctness of channel operations.- 5567-5567: The logic to check if the channel is in a specific state based on the holder and counterparty commitment transaction numbers is correctly implemented. This check is crucial for determining the readiness of the channel for certain operations, ensuring the correctness of channel operations based on the current state of the channel.
- 5670-5709: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [5621-5698]
The method
check_get_channel_readyand the conditions under which achannel_readymessage is generated or not are correctly implemented. This method ensures that achannel_readymessage is generated only when appropriate, based on the channel's state and configuration, maintaining protocol compliance and ensuring the correct operation of the channel.
- 5638-5645: The logic to determine whether a
channel_readymessage can be produced based on the number of confirmations and the channel's state is correctly implemented. This ensures that achannel_readymessage is generated only when the channel is in a state that allows for such a message to be sent, maintaining protocol compliance and ensuring the correct operation of the channel.- 5673-5697: The conditions under which a
channel_readymessage is not produced, such as when the signer is pending funding or a monitor update is in progress, are correctly implemented. This ensures that achannel_readymessage is generated only when all prerequisites are met, maintaining protocol compliance and ensuring the correct operation of the channel.- 5773-5773: The logic to generate a
channel_readymessage immediately upon transaction confirmation, if necessary, is correctly implemented. This ensures that achannel_readymessage is sent to the peer as soon as the channel is ready for operation, maintaining protocol compliance and ensuring the readiness of the channel for operation.- 5839-5839: The logic to generate a
channel_readymessage upon a new block confirmation, if necessary, is correctly implemented. This ensures that achannel_readymessage is sent to the peer as soon as the channel is ready for operation based on the new block confirmation, maintaining protocol compliance and ensuring the readiness of the channel for operation.- 6142-6142: The calculation of
next_local_commitment_numberandnext_remote_commitment_numberfor thechannel_reestablishmessage is correctly implemented. This ensures that thechannel_reestablishmessage contains the correct commitment numbers, reflecting the current state of the channel and facilitating the correct reestablishment of the channel state with the peer.- 6328-6328: The logic to set the
resend_ordertoRevokeAndACKFirstand adjust the channel state accordingly is correctly implemented. This ensures that the channel state is correctly updated to reflect the new ordering requirement, maintaining protocol compliance and ensuring the correct progression of the channel state.- 6593-6593: The struct
OutboundV1Channelis introduced to represent an outbound channel using V1 channel establishment. This struct includes fields for managing the channel context and the unfunded channel context, along with a flagsigner_pending_open_channelto track the state of the signer. This addition is crucial for handling outbound channels and managing the state of asynchronous signing operations.- 6597-6604: The method
newfor creating a new instance ofOutboundV1Channelis correctly implemented, taking into account various parameters such as fee estimator, entropy source, signer provider, counterparty node ID, channel value, push amount, user ID, config, current chain height, outbound SCID alias, and temporary channel ID. This method ensures that an outbound channel is correctly initialized with all necessary parameters, facilitating the establishment and management of the channel.- 6666-6673: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [6669-6695]
The initialization of
OutboundV1Channelwith aChannelContextstruct that includes theholder_signer,shutdown_scriptpubkey,destination_script,holder_commitment, and other relevant fields is correctly implemented. This ensures that the outbound channel is correctly set up with all necessary information for its operation, including the management of holder commitments and the handling of shutdown and destination scripts.
- 6805-6806: The initialization of the
unfunded_contextfield inOutboundV1Channelwithunfunded_channel_age_ticksset to 0 and thesigner_pending_open_channelflag set to false is correctly implemented. This ensures that the outbound channel is correctly initialized with the unfunded channel context and the state of the signer, facilitating the management of the channel's lifecycle.- 6862-6862: The check to ensure that the channel commitment transaction numbers have not advanced prior to the
funding_createdmessage is correctly implemented. This ensures that the channel state is consistent and that the channel has not prematurely advanced its state, maintaining the integrity of the channel's operation.- 6929-6929: The method
maybe_handle_error_without_closefor handling errors without closing the channel is correctly implemented. This method allows for the handling of certain error conditions in a way that does not necessitate the immediate closure of the channel, providing flexibility in error handling and the potential for error recovery.- 6964-6964: The method
get_open_channelfor generating anOpenChannelmessage based on the current state of theOutboundV1Channelis correctly implemented. This method ensures that anOpenChannelmessage is generated with the correct parameters, reflecting the current state of the channel and facilitating the establishment of the channel with the counterparty.- 6971-6971: The method
get_open_channelcorrectly checks if the channel is outbound and if the channel has not moved forward before generating anOpenChannelmessage. This ensures that anOpenChannelmessage is generated only when appropriate, maintaining the integrity of the channel's operation and ensuring that the channel establishment process is correctly managed.
waterson
force-pushed
the
waterson/cache-commitment-point
branch
2 times, most recently
from
45675ed
to
2a474cc
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (10)
- fuzz/src/chanmon_consistency.rs (14 hunks)
- lightning/src/chain/channelmonitor.rs (1 hunks)
- lightning/src/chain/onchaintx.rs (2 hunks)
- lightning/src/ln/async_signer_tests.rs (10 hunks)
- lightning/src/ln/channel.rs (83 hunks)
- lightning/src/ln/channelmanager.rs (17 hunks)
- lightning/src/ln/functional_test_utils.rs (9 hunks)
- lightning/src/ln/functional_tests.rs (5 hunks)
- lightning/src/sign/mod.rs (3 hunks)
- lightning/src/util/test_channel_signer.rs (10 hunks)
Files not summarized due to errors (2)
- lightning/src/ln/async_signer_tests.rs: Error: Message exceeds token limit
- lightning/src/ln/channel.rs: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (7)
- lightning/src/chain/channelmonitor.rs
- lightning/src/chain/onchaintx.rs
- lightning/src/ln/channelmanager.rs
- lightning/src/ln/functional_test_utils.rs
- lightning/src/ln/functional_tests.rs
- lightning/src/sign/mod.rs
- lightning/src/util/test_channel_signer.rs
Additional comments: 50
lightning/src/ln/async_signer_tests.rs (13)
- 24-50: The function
with_async_signeris a critical piece of infrastructure for these tests, simulating the enabling and disabling of a signer's operations for a channel. It's well-structured to support the tests' needs, but it's essential to ensure that the re-computation of the channel ID (lines 37-42) is necessary and correctly implemented, considering the potential for channel ID changes during the test setup phase.- 52-100: The test
test_open_channeleffectively simulates the process of opening a channel with an asynchronous signer. It's crucial to verify that the assertions made (e.g., expecting no message events at certain points) align with the expected behavior in these asynchronous scenarios. Additionally, ensuring that the test covers all relevant aspects of the channel opening process in the context of asynchronous signing is important.- 140-177: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [102-170]
The series of tests from
test_funding_created_grstotest_funding_created_sgrare designed to simulate the funding creation process under various conditions of signer availability. It's important to ensure that these tests comprehensively cover the scenarios they aim to test and that the use ofwith_async_signerin these contexts accurately reflects the intended asynchronous behavior.
- 212-249: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [173-242]
Similar to the previous comment, the tests from
test_funding_signed_grstotest_funding_signed_sgrfocus on the funding signed phase with an asynchronous signer. Verifying the completeness of these tests and the correct simulation of asynchronous behavior is crucial for ensuring the robustness of the implementation under test.
- 269-318: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [245-312]
The tests from
test_commitment_signed_grstotest_commitment_signed_sgraim to verify the behavior of commitment signing with an asynchronous signer. It's essential to confirm that these tests accurately simulate the asynchronous signing process and cover all relevant scenarios to ensure the implementation's resilience.
- 400-1182: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [314-430]
The tests from
test_funding_signed_0conf_grstotest_funding_signed_0conf_sgrfocus on the zero-confirmation funding signed process with an asynchronous signer. Ensuring that these tests accurately reflect the asynchronous nature of the signing process and cover the necessary scenarios for zero-confirmation channels is important for validating the implementation's behavior.
- 433-622: The tests from
test_payment_grstotest_payment_sgrsimulate payment processing with an asynchronous signer. It's crucial to verify that these tests accurately represent the payment process in the context of asynchronous signing and that they cover all necessary scenarios to ensure the robustness of the payment mechanism.- 625-794: The tests from
test_peer_reconnect_grstotest_peer_reconnect_sgrsimulate peer reconnection scenarios with an asynchronous signer. Ensuring that these tests accurately simulate the reconnection process and the asynchronous signing behavior is important for validating the implementation's resilience in handling peer reconnections.- 797-859: The test
channel_update_fee_testaims to verify the behavior of fee updates in a channel with an asynchronous signer. It's important to ensure that this test accurately simulates the fee update process and the asynchronous signing behavior, particularly in scenarios where the signer's availability might impact the ability to process fee updates.- 862-900: The test
monitor_honors_commitment_raa_orderfocuses on the order of commitment and revocation messages in the presence of an asynchronous signer. Verifying that this test accurately represents the expected message order and that it effectively tests the system's behavior in maintaining this order is crucial for ensuring the correctness of the implementation.- 903-999: The test
peer_restart_with_blocked_signer_and_pending_paymentsimulates a scenario where a peer restarts with a blocked signer and a pending payment. Ensuring that this test accurately represents the complexities of handling pending payments and signer availability across peer restarts is important for validating the system's resilience and correctness in such scenarios.- 1002-1115: The test
peer_restart_with_blocked_signer_before_pending_paymentaims to simulate the behavior of the system when a peer restarts with a blocked signer before a pending payment is processed. Verifying that this test accurately captures the nuances of signer availability and payment processing in the context of peer restarts is crucial for ensuring the robustness of the implementation.- 1118-1181: The test
no_stray_channel_reestablishis designed to ensure that no unnecessarychannel_reestablishmessages are sent under specific conditions, including signer availability and peer restarts. It's important to verify that this test accurately represents the intended scenarios and effectively validates the absence of straychannel_reestablishmessages.fuzz/src/chanmon_consistency.rs (5)
- 52-52: The import of
opsfromlightning::util::test_channel_signeris added to support the new asynchronous signing feature. This change aligns with the PR's objective to enhance theChannelSignerinterface for asynchronous operations.- 76-76: The addition of
ASYNC_OPSconstant is directly related to the asynchronous signing feature, enabling tests to specify which operations should simulate asynchronous behavior. This constant is well-defined and correctly uses the bitwise OR operator to combine multiple operation flags.- 833-833: The handling of various LN messages (
update_add_htlc,update_fulfill_htlc,update_fail_htlc,update_fail_malformed_htlc,update_fee,commitment_signed,revoke_and_ack,channel_reestablish) within the fuzz test is crucial for simulating real-world scenarios and testing the robustness of channel state consistency. These operations are correctly placed within the test logic to simulate different network conditions and peer interactions. It's important to ensure that these message handlers are invoked with the correct parameters and in realistic sequences to accurately assess the system's behavior.Also applies to: 848-848, 852-852, 856-856, 860-860, 877-877, 886-886, 894-894
- 1294-1403: The handling of asynchronous signing operations (
set_signer_unavailable,set_signer_available,signer_unblocked) is a significant addition to this test file, reflecting the PR's main objective. These operations are correctly used to simulate the signer's availability changes and trigger the necessary state transitions in the channel logic. It's crucial that these methods are called with the correct operation flags and that thesigner_unblockedmethod is invoked to resume channel operations after making the signer available. This ensures the test accurately reflects the intended behavior of asynchronous signing in the system.- 1537-1569: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1498-1606]
The final section of the test, which ensures no channel is in a stuck state and that the system can recover from various error conditions, is critical for assessing the robustness of the channel monitor logic, especially in the context of asynchronous signing. The steps taken to restore monitors, reconnect peers, restore signers, and flush pending messages are essential for a comprehensive test. This section effectively simulates recovery from failure states and ensures that channels can resume normal operation, which is crucial for the reliability of the Lightning Network. The use of assertions to verify the ability to send payments after recovery is a good practice to confirm the system's functionality.
lightning/src/ln/channel.rs (32)
- 796-818: The
SignerResumeUpdatesstruct is introduced to manage updates when the signer becomes unblocked. This struct includes fields for various messages that might need to be sent to the peer, such ascommitment_update,raa,funding_signed, andchannel_ready, along with anorderfield to maintain the correct message sending order. This addition is crucial for handling asynchronous signing scenarios effectively. However, ensure that the logic that utilizes this struct properly checks for the presence of these optional fields before attempting to send messages to the peer to avoid unwrappingNonevalues.- 995-1000: The
HolderCommitmentenum is introduced to manage the state of holder commitments. It includes variants forUninitialized,PendingNext, andAvailable, each carrying thetransaction_numberand relevant public keys as needed. This design allows for a clear representation of the holder commitment's state, facilitating easier management of commitment points and transitions between states. Ensure that all code paths that modify the holder commitment state handle these transitions correctly and update thetransaction_numberappropriately to maintain consistency.- 1125-1128: The
holder_commitmentfield is added to track the current state of the holder's commitment transaction, andprev_holder_commitment_secretis introduced to store the secret corresponding to the previous state. These additions are essential for managing commitment transactions and secrets, especially in the context of asynchronous signing. Ensure that the logic for updating these fields correctly handles all possible states and transitions, particularly in error scenarios or when dealing with asynchronous operations that may not complete immediately.- 1163-1175: The introduction of flags such as
signer_pending_commitment_update,signer_pending_revoke_and_ack,signer_pending_funding, andsigner_pending_channel_readyis crucial for managing the state of pending operations when dealing with an asynchronous signer. These flags help in tracking which operations are pending and need to be retried once the signer becomes unblocked. It's important to ensure that these flags are correctly set and cleared at appropriate times to avoid missing or duplicating operations.- 1668-1674: The methods
advance_holder_commitmentandrequest_next_holder_commitmentare introduced to manage the advancement of the holder's commitment state. These methods are critical for updating the commitment point and requesting the next commitment point from the signer, especially in asynchronous signing scenarios. Ensure that these methods are called at appropriate times to keep the commitment state up-to-date and handle any errors or delays in obtaining the next commitment point.- 2013-2014: The method
build_holder_transaction_keysis introduced to generate the transaction keys for the holder's commitment transaction. This method is essential for constructing commitment transactions and ensuring that the correct keys are used based on the current commitment point. Ensure that this method is called with the correct parameters and that the generated keys are used consistently across the codebase to maintain transaction integrity.- 3543-3545: The usage of
build_holder_transaction_keysandbuild_commitment_transactionwithin the same context suggests a coherent approach to generating commitment transactions based on the current state of the holder's commitment. This pattern is crucial for ensuring that commitment transactions are built with the correct parameters and keys. Ensure that the transaction generation logic is robust and handles all necessary edge cases, especially in scenarios involving asynchronous signing or state transitions.- 3708-3712: The call to
advance_holder_commitmentfollowed by a log statement about setting the resend order toCommitmentFirstindicates a clear sequence of operations related to managing the holder's commitment state and preparing for message resending. This sequence is important for ensuring that the channel state is correctly updated and that messages are resent in the correct order after a reconnection. Ensure that the logic surrounding these operations is carefully reviewed to prevent any inconsistencies or errors in message handling.- 4207-4208: The calculation of
buffer_fee_msatusingbuild_holder_transaction_keysandbuild_commitment_transactiondemonstrates a detailed approach to managing commitment transactions and fees. This calculation is crucial for ensuring that the channel has sufficient balance to cover the fees for commitment transactions. Ensure that the fee calculation logic accurately reflects the current channel state and fee rates to prevent any issues with transaction broadcasting or channel closure due to insufficient fees.- 4260-4275: The logic for clearing pending flags such as
signer_pending_channel_ready,signer_pending_revoke_and_ack, andsigner_pending_commitment_updateis crucial for managing the state of operations awaiting the signer's response. This clearing process is important to ensure that operations are not duplicated and that the channel state is correctly updated once the signer becomes unblocked. Ensure that these flags are managed carefully throughout the codebase to maintain consistency and prevent any operational errors.- 4408-4408: The conditional logic for generating a
channel_readymessage based on the channel's state and the presence of an announcement signature indicates a nuanced approach to managing channel state transitions and message generation. This logic is important for ensuring that the correct messages are sent to the peer at the right time, especially in scenarios involving channel reestablishment or updates. Ensure that the conditions for generating these messages are thoroughly reviewed to prevent any missed messages or incorrect state transitions.- 4430-4435: The logic for updating the holder commitment secret and generating a
revoke_and_ackmessage based on the channel's state and the signer's readiness is critical for maintaining the integrity of the commitment transaction process. This logic ensures that secrets are released and acknowledgment messages are sent at appropriate times. Ensure that the error handling and conditional logic surrounding these operations are robust to prevent any issues with commitment transaction validity or channel security.- 4444-4458: The handling of
monitor_pending_commitment_signedandmonitor_pending_revoke_and_ackflags, along with the conditional logic for settingsigner_pending_commitment_updateandsigner_pending_revoke_and_ack, demonstrates a comprehensive approach to managing channel state and message resending. This handling is crucial for ensuring that messages are resent in the correct order and that the channel state is accurately reflected. Ensure that the logic for managing these flags and conditions is carefully reviewed to maintain channel consistency and prevent any message ordering issues.- 4470-4492: The enforcement of message ordering between
commitment_updateandrevoke-and-ackbased on theresend_orderand the signer's readiness is an important aspect of managing channel state and message flow. This enforcement ensures that messages are delivered to the peer in the expected order, which is critical for maintaining protocol compliance and channel integrity. Ensure that the logic for handling message ordering is robust and accounts for all possible states and transitions to prevent any issues with message delivery or channel operations.- 4540-4617: The
signer_maybe_unblockedmethod's implementation, which generatesSignerResumeUpdatesbased on the signer's readiness and the current channel state, is crucial for handling asynchronous signing scenarios. This method ensures that pending operations are retried and that the necessary messages are generated and sent to the peer once the signer becomes unblocked. Ensure that this method's logic is thoroughly reviewed to cover all scenarios where the signer may become unblocked and to prevent any missed operations or inconsistencies in message sending.- 4636-4671: The
get_last_revoke_and_ackmethod's logic for regenerating the lastrevoke-and-ackmessage based on the current state of the holder's commitment and the availability of the per-commitment secret is critical for ensuring the integrity of the commitment transaction process. This method allows for the correct regeneration and sending of acknowledgment messages, which is essential for maintaining channel security and protocol compliance. Ensure that the conditions and error handling in this method are robust to prevent any issues with message generation or commitment transaction validity.- 4754-4756: The regeneration of the latest commitment update in the
get_last_commitment_update_for_sendmethod demonstrates a detailed approach to managing commitment transactions and HTLCs. This regeneration is crucial for ensuring that the latest state is accurately reflected in the commitment transaction, especially in scenarios involving asynchronous signing or state transitions. Ensure that the logic for regenerating commitment updates is carefully reviewed to prevent any inconsistencies or errors in transaction generation.- 4872-4872: The logic for generating a
channel_readymessage in theReestablishResponsesstruct based on the channel's state and the presence of an announcement signature is an important aspect of managing channel state transitions and message generation. This logic ensures that the correct messages are sent to the peer at the right time, especially in scenarios involving channel reestablishment or updates. Ensure that the conditions for generating these messages are thoroughly reviewed to prevent any missed messages or incorrect state transitions.- 4881-4904: The conditional logic for generating a
revoke_and_ackmessage based on the channel's state and the signer's readiness is critical for maintaining the integrity of the commitment transaction process. This logic ensures that secrets are released and acknowledgment messages are sent at appropriate times. Ensure that the error handling and conditional logic surrounding these operations are robust to prevent any issues with commitment transaction validity or channel security.- 4925-4928: The calculation of
channel_readybased on the channel's state and the confirmation height indicates a nuanced approach to managing channel state transitions and message generation. This calculation is important for ensuring that the correct messages are sent to the peer at the right time, especially in scenarios involving channel reestablishment or updates. Ensure that the logic for calculating and generatingchannel_readymessages is carefully reviewed to prevent any missed messages or incorrect state transitions.- 4932-4939: The handling of
raaandchannel_readymessages in theReestablishResponsesstruct, along with the conditional logic for settingsigner_pending_revoke_and_ack, demonstrates a comprehensive approach to managing channel state and message resending. This handling is crucial for ensuring that messages are resent in the correct order and that the channel state is accurately reflected. Ensure that the logic for managing these messages and conditions is carefully reviewed to maintain channel consistency and prevent any message ordering issues.- 4958-4967: The logic for generating a
commitment_updatemessage based on the channel's state and the signer's readiness is critical for maintaining the integrity of the commitment transaction process. This logic ensures that the latest state is accurately reflected in the commitment transaction, especially in scenarios involving asynchronous signing or state transitions. Ensure that the conditions and error handling in this logic are robust to prevent any inconsistencies or errors in transaction generation.- 5482-5482: The method
get_cur_holder_commitment_transaction_numberis introduced to provide easy access to the current holder commitment transaction number. This method is essential for various operations that require knowledge of the current commitment state, such as generating messages or performing state checks. Ensure that this method is used consistently across the codebase to maintain clarity and reduce the risk of errors due to direct field access.- 5577-5577: The logic for determining if a channel is in a specific state based on the commitment transaction numbers is crucial for managing channel state transitions and operations. This logic ensures that operations are performed based on the accurate state of the channel, especially in scenarios involving asynchronous signing or state transitions. Ensure that the conditions for determining channel states are thoroughly reviewed to prevent any missed operations or incorrect state transitions.
- 5631-5638: The
check_get_channel_readymethod's implementation, which generates achannel_readymessage based on the channel's state and confirmation height, is crucial for handling channel state transitions and message generation. This method ensures that the correct messages are sent to the peer at the right time, especially in scenarios involving channel reestablishment or updates. Ensure that this method's logic is thoroughly reviewed to cover all scenarios where achannel_readymessage may be needed and to prevent any missed messages or inconsistencies in message sending.- 5648-5654: The conditional logic for generating a
channel_readymessage based on the channel's state and the number of confirmations is critical for maintaining the integrity of the channel state and ensuring that messages are sent at appropriate times. This logic allows for the correct generation and sending ofchannel_readymessages, which is essential for maintaining channel security and protocol compliance. Ensure that the conditions and error handling in this logic are robust to prevent any issues with message generation or channel state transitions.- 5683-5707: The logic for determining whether a
channel_readymessage should be generated and sent based on various conditions, such as the signer's readiness and the channel's state, is crucial for managing channel operations and state transitions. This logic ensures thatchannel_readymessages are generated and sent at the correct times, especially in scenarios involving asynchronous signing or channel reestablishment. Ensure that the conditions for generating and sendingchannel_readymessages are carefully reviewed to prevent any missed messages or incorrect state transitions.- 5783-5783: The conditional logic for generating a
channel_readymessage immediately after a transaction confirmation, instead of waiting for abest_block_updatedcall, demonstrates a proactive approach to managing channel state and message generation. This logic is important for ensuring thatchannel_readymessages are sent at the correct time, especially in scenarios involving 0-conf channels or rapid state transitions. Ensure that the conditions for generating these messages are thoroughly reviewed to prevent any missed messages or incorrect state transitions.- 5849-5849: The logic for generating a
channel_readymessage based on the channel's state and the current block height is crucial for managing channel state transitions and message generation. This logic ensures that the correct messages are sent to the peer at the right time, especially in scenarios involving channel reestablishment or updates. Ensure that the logic for generatingchannel_readymessages is carefully reviewed to prevent any missed messages or incorrect state transitions.- 6152-6152: The calculation of
next_local_commitment_numberandnext_remote_commitment_numberfor thechannel_reestablishmessage is crucial for ensuring that the message accurately reflects the current state of the channel's commitment transactions. This calculation is important for maintaining protocol compliance and ensuring that both parties have a consistent view of the channel state. Ensure that the logic for calculating these numbers is robust and accounts for all possible states and transitions to prevent any issues with channel reestablishment or state synchronization.- 6338-6338: The setting of
resend_ordertoRevokeAndACKFirstand the subsequent logic for handling commitment transactions and HTLCs demonstrates a clear sequence of operations related to managing the holder's commitment state and preparing for message resending. This sequence is important for ensuring that the channel state is correctly updated and that messages are resent in the correct order after a reconnection. Ensure that the logic surrounding these operations is carefully reviewed to prevent any inconsistencies or errors in message handling.- 6603-6603: The
OutboundV1Channelstruct is introduced to represent an outbound channel using V1 channel establishment. This struct includes fields for managing the channel context, unfunded context, and a flag for tracking if an open channel message is pending due to the signer being blocked. This addition is crucial for handling outbound channel operations, especially in scenarios involving asynchronous signing. Ensure that the logic that utilizes this struct properly manages the channel state and handles the asynchronous signing scenarios effectively to avoid any operational issues.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- lightning/src/ln/channelmanager.rs (17 hunks)
- lightning/src/ln/functional_test_utils.rs (9 hunks)
- lightning/src/ln/functional_tests.rs (5 hunks)
- lightning/src/sign/mod.rs (3 hunks)
Files skipped from review as they are similar to previous changes (3)
- lightning/src/ln/channelmanager.rs
- lightning/src/ln/functional_tests.rs
- lightning/src/sign/mod.rs
This is a do-over of lightningdevkit#2653, wherein we support asynchronous signing for 'normal' channel operation. This involves allowing the following `ChannelSigner` methods to return an `Err` result, indicating that the requested value is not available: - get_per_commitment_point - release_commitment_secret - sign_counterparty_commitment When the value does become available, channel operation can be resumed by invoking `signer_unblocked`. Note that this adds the current and next per-commitment point to the state that is persisted by the channel monitor.
Creates and manages an explicit `HolderCommitment` type to deal with managing the current and next commitment points.
waterson
force-pushed
the
waterson/cache-commitment-point
branch
from
886b7e3
to
8db91d7
Compare
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2849 +/- ##
==========================================
- Coverage 89.18% 88.99% -0.20%
==========================================
Files 116 116
Lines 93098 93477 +379
Branches 93098 93477 +379
==========================================
+ Hits 83034 83189 +155
- Misses 7538 7733 +195
- Partials 2526 2555 +29 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (10)
- fuzz/src/chanmon_consistency.rs (14 hunks)
- lightning/src/chain/channelmonitor.rs (1 hunks)
- lightning/src/chain/onchaintx.rs (2 hunks)
- lightning/src/ln/async_signer_tests.rs (10 hunks)
- lightning/src/ln/channel.rs (83 hunks)
- lightning/src/ln/channelmanager.rs (17 hunks)
- lightning/src/ln/functional_test_utils.rs (9 hunks)
- lightning/src/ln/functional_tests.rs (5 hunks)
- lightning/src/sign/mod.rs (3 hunks)
- lightning/src/util/test_channel_signer.rs (10 hunks)
Files not summarized due to errors (2)
- lightning/src/ln/async_signer_tests.rs: Error: Message exceeds token limit
- lightning/src/ln/channel.rs: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (8)
- fuzz/src/chanmon_consistency.rs
- lightning/src/chain/channelmonitor.rs
- lightning/src/chain/onchaintx.rs
- lightning/src/ln/channelmanager.rs
- lightning/src/ln/functional_test_utils.rs
- lightning/src/ln/functional_tests.rs
- lightning/src/sign/mod.rs
- lightning/src/util/test_channel_signer.rs
Additional comments: 30
lightning/src/ln/channel.rs (30)
- 796-818: The
SignerResumeUpdatesstruct is introduced to manage updates when the signer becomes unblocked. This struct includes fields for various messages that might need to be sent to the peer, such ascommitment_update,raa,funding_signed, andchannel_ready, along with anorderfield to maintain the correct message sending order. This addition is crucial for handling asynchronous signing scenarios effectively. However, it's important to ensure that the logic for utilizing this struct elsewhere in the codebase correctly handles these updates, especially considering the order in which messages are sent.- 995-1000: The
HolderCommitmentenum is introduced to manage the state of the holder's commitment transactions. It includes states forUninitialized,PendingNext, andAvailable, each carrying thetransaction_numberand relevant public keys as needed. This change is essential for tracking the progression of commitment transactions, especially in the context of asynchronous signing. It's important to verify that transitions between these states are handled correctly throughout the channel's lifecycle to ensure the integrity of commitment transactions.- 1125-1128: The
holder_commitmentandprev_holder_commitment_secretfields are added to track the current state of the holder's commitment transaction and the secret for the previous state, respectively. This addition is crucial for managing commitment transactions and secrets, especially in scenarios involving asynchronous signing. It's important to ensure that these fields are updated correctly in response to relevant channel events and that their values are used appropriately in commitment transaction generation and verification processes.- 1163-1175: The introduction of flags such as
signer_pending_commitment_update,signer_pending_revoke_and_ack,signer_pending_funding, andsigner_pending_channel_readyis crucial for managing the state of pending operations when the signer is asynchronous. These flags help in tracking which operations are pending and ensure that the channel can resume normal operations once the signer becomes unblocked. It's important to verify that these flags are set and cleared at appropriate times in the channel's lifecycle to prevent any inconsistencies or missed operations.- 1668-1680: The methods
advance_holder_commitment,request_next_holder_commitment,is_holder_commitment_uninitialized, andupdate_holder_commitment_secretare introduced to manage the holder's commitment state and secrets, especially in the context of asynchronous signing. These methods are crucial for advancing the state of the holder's commitment, requesting the next state, checking if the commitment is uninitialized, and updating the commitment secret. It's important to ensure that these methods are used correctly throughout the channel's lifecycle to maintain the integrity and consistency of commitment transactions and secrets.- 2013-2014: The method
build_holder_transaction_keysis introduced to generate the transaction keys for the holder's commitment transaction. This method is crucial for ensuring that the correct keys are used in the generation of commitment transactions, especially in scenarios involving asynchronous signing. It's important to verify that this method is used correctly in the commitment transaction generation process and that the generated keys are consistent with the expected state of the channel.- 3543-3545: The usage of
build_holder_transaction_keysandbuild_commitment_transactionwithin the context of generating a commitment transaction indicates a structured approach to handling commitment transactions. This pattern ensures that the correct keys are used and that the commitment transaction is built based on the current state of the channel. It's important to verify that the generated commitment transaction aligns with the channel's state and that any errors in this process are handled appropriately.- 3708-3712: The call to
advance_holder_commitmentfollowed by the adjustment ofresend_ordertoCommitmentFirstdemonstrates a clear sequence of operations to advance the holder's commitment state and ensure the correct ordering of messages. This sequence is crucial for maintaining the integrity of the channel's state, especially in asynchronous signing scenarios. It's important to verify that the advancement of the holder's commitment and the setting of the resend order are done in a manner that preserves the consistency and correctness of the channel's state.- 4207-4208: The generation of a commitment transaction within the context of proposing a fee rate update demonstrates the channel's ability to handle dynamic fee rates. This process involves building the holder's transaction keys, generating the commitment transaction, and calculating the buffer fee msat. It's important to ensure that the fee rate update process is robust, correctly calculates fees, and maintains the integrity of the commitment transaction. Additionally, verifying that the holder's balance after the fee rate update is sufficient is crucial for preventing channel closure due to insufficient funds.
- 4260-4275: Clearing pending flags such as
signer_pending_channel_ready,signer_pending_revoke_and_ack, andsigner_pending_commitment_updatein anticipation of achannel_reestablishmessage demonstrates proactive management of the channel's state. This approach ensures that operations are not duplicated and that the channel can resume normal operations efficiently once the counterparty's reestablish message is received. It's important to verify that these flags are managed correctly to prevent any inconsistencies or missed operations in the channel's lifecycle.- 4408-4408: The conditional generation of a
channel_readymessage based on the channel's outbound status and the absence of a minimum depth requirement for the funding transaction demonstrates a nuanced approach to managing channel readiness. This logic is crucial for ensuring that the channel is ready to process payments as soon as the funding transaction is confirmed, especially in scenarios where immediate readiness is required. It's important to verify that this logic correctly reflects the channel's readiness state and that the generatedchannel_readymessage is sent appropriately.- 4430-4435: The conditional generation of a
revoke_and_ackmessage based on the monitor's pending status and the availability of the holder's commitment secret demonstrates careful management of message sending in the channel's lifecycle. This approach ensures that messages are generated and sent based on the current state of the channel and the signer, especially in asynchronous signing scenarios. It's important to verify that the generation and sending ofrevoke_and_ackmessages are handled correctly to maintain the integrity and consistency of the channel's state.- 4444-4458: The logic for handling the pending status of a
commitment_signedmessage and the conditional clearing of thesigner_pending_commitment_updateflag demonstrates a detailed approach to managing the channel's state. This logic is crucial for ensuring that commitment updates are handled correctly, especially in scenarios involving asynchronous signing. It's important to verify that the conditions for clearing the pending status are accurate and that the channel's state is updated appropriately to reflect the resolution of pending operations.- 4540-4630: The
signer_maybe_unblockedmethod is designed to handle the scenario where the signer becomes unblocked, allowing the channel to proceed with operations that were previously pending due to the signer's unavailability. This method checks the availability of the holder's commitment and attempts to update it if necessary. It also handles the generation of various messages (funding_signed,channel_ready,commitment_update,raa) based on the current state of the channel and the pending flags. This approach is crucial for ensuring that the channel can resume normal operations efficiently once the signer is available. It's important to verify that this method correctly updates the channel's state and generates the necessary messages based on the pending flags and the current state of the channel.- 4636-4671: The
get_last_revoke_and_ackmethod is designed to generate arevoke_and_ackmessage based on the current state of the holder's commitment and the availability of the previous commitment secret. This method is crucial for ensuring that the correctrevoke_and_ackmessage is generated and sent to the counterparty, especially in scenarios involving asynchronous signing. It's important to verify that this method correctly handles the generation of therevoke_and_ackmessage based on the availability of the commitment point and secret, and that any errors in this process are handled appropriately to maintain the integrity of the channel's state.- 4754-4756: The method for regenerating the latest commitment update demonstrates the channel's ability to handle state updates and generate the necessary messages for communication with the counterparty. This process involves calculating the commitment transaction fee, updating HTLCs, and generating a
commitment_signedmessage. It's important to ensure that the regeneration of the commitment update is done accurately based on the current state of the channel and that the generated message aligns with the expected state of the channel.- 4872-4872: The generation of a
channel_readymessage in theReestablishResponsesstruct based on the channel's state during reconnection demonstrates a structured approach to managing channel readiness. This logic ensures that the channel signals its readiness to the counterparty at the appropriate time, especially in scenarios where immediate readiness is required. It's important to verify that the generation of thechannel_readymessage is done accurately based on the channel's state and that the message is sent appropriately to maintain the integrity of the channel's operations.- 4925-4925: The calculation of
channel_readybased on the commitment transaction numbers during channel reestablishment demonstrates a detailed approach to managing channel readiness. This logic is crucial for ensuring that the channel signals its readiness to the counterparty at the correct time, especially in scenarios involving asynchronous signing. It's important to verify that the calculation of thechannel_readymessage is done accurately based on the commitment transaction numbers and that the message is sent appropriately to maintain the integrity of the channel's operations.- 4958-4963: The conditional generation of a
commitment_updatemessage based on the availability of arevoke_and_ackmessage or the steps behind in the commitment transaction sequence demonstrates careful management of message sending in the channel's lifecycle. This approach ensures that messages are generated and sent based on the current state of the channel and the signer, especially in asynchronous signing scenarios. It's important to verify that the generation and sending ofcommitment_updatemessages are handled correctly to maintain the integrity and consistency of the channel's state.- 5482-5482: The method
get_cur_holder_commitment_transaction_numberis introduced to retrieve the current transaction number for the holder's commitment. This method is crucial for ensuring that the correct transaction number is used in various operations within the channel, especially in scenarios involving asynchronous signing. It's important to verify that this method is used correctly throughout the channel's lifecycle to maintain the integrity and consistency of commitment transactions.- 5577-5577: The condition checking if both the holder's and counterparty's commitment transaction numbers are at their initial values to determine if the channel is a 0-conf channel demonstrates a nuanced approach to managing channel readiness. This logic is crucial for ensuring that the channel is ready to process payments as soon as the funding transaction is confirmed, especially in scenarios where immediate readiness is required. It's important to verify that this logic correctly reflects the channel's readiness state and that the channel behaves appropriately in 0-conf scenarios.
- 5631-5638: The
check_get_channel_readymethod is designed to determine if achannel_readymessage should be generated based on the channel's confirmation height and minimum depth requirement. This method is crucial for ensuring that the channel signals its readiness to the counterparty at the appropriate time, especially in scenarios where immediate readiness is required. It's important to verify that this method correctly assesses the channel's readiness state based on the confirmation height and minimum depth requirement and that the generatedchannel_readymessage is sent appropriately.- 5648-5654: The logic for not generating a
channel_readymessage if the funding transaction confirmations are less than the minimum depth requirement demonstrates careful management of channel readiness. This approach ensures that the channel signals its readiness to the counterparty only when it is truly ready, especially in scenarios involving asynchronous signing or delayed transaction confirmations. It's important to verify that this logic accurately reflects the channel's readiness state based on the funding transaction confirmations and that the channel behaves appropriately in these scenarios.- 5683-5687: The condition for not generating a
channel_readymessage if a commitment update is not needed demonstrates a detailed approach to managing channel readiness. This logic is crucial for ensuring that the channel signals its readiness to the counterparty only when necessary, especially in scenarios where the channel's state does not require an immediate update. It's important to verify that this condition accurately reflects the channel's readiness state and that the channel behaves appropriately in scenarios where a commitment update is not needed.- 5783-5783: The conditional generation of a
channel_readymessage based on the channel's 0-conf status and the immediate need for channel readiness demonstrates a nuanced approach to managing channel operations. This logic is crucial for ensuring that the channel is ready to process payments as soon as the funding transaction is confirmed, especially in scenarios where immediate readiness is required. It's important to verify that this logic correctly reflects the channel's readiness state in 0-conf scenarios and that the generatedchannel_readymessage is sent appropriately.- 5849-5849: The generation of a
channel_readymessage based on the channel's confirmation height and the need for channel readiness demonstrates a structured approach to managing channel operations. This logic ensures that the channel signals its readiness to the counterparty at the appropriate time, especially in scenarios involving asynchronous signing or delayed transaction confirmations. It's important to verify that the generation of thechannel_readymessage is done accurately based on the channel's state and that the message is sent appropriately to maintain the integrity of the channel's operations.- 6152-6152: The calculation of
next_local_commitment_numberfor thechannel_reestablishmessage based on the holder's and counterparty's commitment transaction numbers demonstrates a detailed approach to managing channel state synchronization. This calculation is crucial for ensuring that thechannel_reestablishmessage accurately reflects the current state of the channel, especially in scenarios involving asynchronous signing or state discrepancies. It's important to verify that this calculation is done accurately and that thechannel_reestablishmessage is generated correctly based on the channel's state.- 6338-6338: The adjustment of
resend_ordertoRevokeAndACKFirstfollowing the generation of a commitment transaction demonstrates a clear sequence of operations to ensure the correct ordering of messages. This sequence is crucial for maintaining the integrity of the channel's state, especially in asynchronous signing scenarios. It's important to verify that the adjustment of the resend order is done in a manner that preserves the consistency and correctness of the channel's state.- 6603-6603: The
OutboundV1Channelstruct is introduced to represent an outbound channel using V1 channel establishment. This struct includes fields for managing the channel's context, unfunded context, and a flag for tracking if anopen_channelmessage is pending due to the signer being blocked. This addition is crucial for handling outbound channel operations, especially in scenarios involving asynchronous signing. It's important to ensure that the logic for utilizing this struct elsewhere in the codebase correctly handles these operations and tracks the state of the outbound channel effectively.- 6607-6610: The
newmethod forOutboundV1Channelis designed to create a new outbound channel with the specified parameters. This method is crucial for initializing the channel's state, including the selection of channel features, calculation of channel values, and generation of channel keys. It's important to verify that this method correctly initializes the channel based on the provided parameters and that any errors in this process are handled appropriately to ensure the successful creation of the outbound channel.
Comment on lines
+24
to
+50
| /// Helper to run operations with a simulated asynchronous signer. | ||
| /// | ||
| /// Disables the signer for the specified channel and then runs `do_fn`, then re-enables the signer | ||
| /// and calls `signer_unblocked`. | ||
| #[cfg(test)] | ||
| pub fn with_async_signer<'a, DoFn, T>(node: &Node, peer_id: &PublicKey, channel_id: &ChannelId, masks: &Vec<u32>, do_fn: &'a DoFn) -> T | ||
| where DoFn: Fn() -> T | ||
| { | ||
| let mask = masks.iter().fold(0, |acc, m| (acc | m)); | ||
| eprintln!("disabling {}", ops::string_from(mask)); | ||
| node.set_channel_signer_ops_available(peer_id, channel_id, mask, false); | ||
| let res = do_fn(); | ||
|
|
||
| // Recompute the channel ID just in case the original ID was temporary. | ||
| let new_channel_id = { | ||
| let channels = node.node.list_channels(); | ||
| assert_eq!(channels.len(), 1, "expected one channel, not {}", channels.len()); | ||
| channels[0].channel_id | ||
| }; | ||
|
|
||
| for mask in masks { | ||
| eprintln!("enabling {} and calling signer_unblocked", ops::string_from(*mask)); | ||
| node.set_channel_signer_ops_available(peer_id, &new_channel_id, *mask, true); | ||
| node.node.signer_unblocked(Some((*peer_id, new_channel_id))); | ||
| } | ||
| res | ||
| } |
There was a problem hiding this comment.
The function with_async_signer is designed to simulate the behavior of an asynchronous signer by disabling the signer for specified operations, running a provided closure, and then re-enabling the signer and calling signer_unblocked. This approach effectively tests the system's ability to handle asynchronous signing operations. However, there's a potential improvement in error handling and documentation:
- Consider adding error handling for the operations within the closure
do_fn. If these operations fail, the function currently proceeds to re-enable the signer and callsigner_unblockedwithout addressing the failure. This might not accurately reflect real-world usage where a failure would likely halt further operations or require specific error handling. - Enhance documentation to clearly describe the expected behavior and usage of this function, including the importance of correctly managing the signer's availability to simulate asynchronous operations accurately.
/// Improved documentation and error handling for `with_async_signer`.
///
/// # Arguments
/// * `node` - The node on which to perform the operations.
/// * `peer_id` - The public key of the peer involved in the channel.
/// * `channel_id` - The ID of the channel for which the signer operations are being simulated.
/// * `masks` - A list of operation masks to disable and then re-enable.
/// * `do_fn` - The closure to execute with the signer disabled. This closure should handle its own errors.
///
/// # Errors
/// This function now returns a `Result<T, E>` to properly handle errors from `do_fn`.
pub fn with_async_signer<'a, DoFn, T, E>(node: &Node, peer_id: &PublicKey, channel_id: &ChannelId, masks: &Vec<u32>, do_fn: &'a DoFn) -> Result<T, E>
where DoFn: Fn() -> Result<T, E>
{
// Existing implementation with error handling for `do_fn`.
}
Comment on lines
10
to
106
| //! Tests for asynchronous signing. These tests verify that the channel state machine behaves | ||
| //! properly with a signer implementation that asynchronously derives signatures. | ||
|
|
||
| use bitcoin::secp256k1::PublicKey; | ||
| use crate::events::{Event, MessageSendEvent, MessageSendEventsProvider}; | ||
| use crate::ln::ChannelId; | ||
| use crate::ln::functional_test_utils::*; | ||
| use crate::ln::msgs; | ||
| use crate::ln::msgs::ChannelMessageHandler; | ||
| use crate::ln::channelmanager::{PaymentId, RecipientOnionFields}; | ||
| use crate::ln::channelmanager::{PaymentId, RAACommitmentOrder, RecipientOnionFields}; | ||
| use crate::util::ser::Writeable; | ||
| use crate::util::test_channel_signer::{EnforcementState, ops}; | ||
| use crate::util::test_utils; | ||
|
|
||
| /// Helper to run operations with a simulated asynchronous signer. | ||
| /// | ||
| /// Disables the signer for the specified channel and then runs `do_fn`, then re-enables the signer | ||
| /// and calls `signer_unblocked`. | ||
| #[cfg(test)] | ||
| pub fn with_async_signer<'a, DoFn, T>(node: &Node, peer_id: &PublicKey, channel_id: &ChannelId, masks: &Vec<u32>, do_fn: &'a DoFn) -> T | ||
| where DoFn: Fn() -> T | ||
| { | ||
| let mask = masks.iter().fold(0, |acc, m| (acc | m)); | ||
| eprintln!("disabling {}", ops::string_from(mask)); | ||
| node.set_channel_signer_ops_available(peer_id, channel_id, mask, false); | ||
| let res = do_fn(); | ||
|
|
||
| // Recompute the channel ID just in case the original ID was temporary. | ||
| let new_channel_id = { | ||
| let channels = node.node.list_channels(); | ||
| assert_eq!(channels.len(), 1, "expected one channel, not {}", channels.len()); | ||
| channels[0].channel_id | ||
| }; | ||
|
|
||
| for mask in masks { | ||
| eprintln!("enabling {} and calling signer_unblocked", ops::string_from(*mask)); | ||
| node.set_channel_signer_ops_available(peer_id, &new_channel_id, *mask, true); | ||
| node.node.signer_unblocked(Some((*peer_id, new_channel_id))); | ||
| } | ||
| res | ||
| } | ||
|
|
||
| #[cfg(feature = "std")] | ||
| #[test] | ||
| fn test_async_commitment_signature_for_funding_created() { | ||
| fn test_open_channel() { | ||
| // Simulate acquiring the signature for `funding_created` asynchronously. | ||
| let chanmon_cfgs = create_chanmon_cfgs(2); | ||
| let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); | ||
| let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); | ||
| let nodes = create_network(2, &node_cfgs, &node_chanmgrs); | ||
|
|
||
| // Open an outbound channel simulating an async signer. | ||
| let channel_id_0 = EnforcementState::with_default_unavailable( | ||
| ops::GET_PER_COMMITMENT_POINT, | ||
| || nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100000, 10001, 42, None, None) | ||
| ).expect("Failed to create channel"); | ||
|
|
||
| { | ||
| let msgs = nodes[0].node.get_and_clear_pending_msg_events(); | ||
| assert!(msgs.is_empty(), "Expected no message events; got {:?}", msgs); | ||
| } | ||
|
|
||
| nodes[0].set_channel_signer_ops_available(&nodes[1].node.get_our_node_id(), &channel_id_0, ops::GET_PER_COMMITMENT_POINT, true); | ||
| nodes[0].node.signer_unblocked(None); | ||
|
|
||
| // nodes[0] --- open_channel --> nodes[1] | ||
| let mut open_chan_msg = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id()); | ||
|
|
||
| // Handle an inbound channel simulating an async signer. | ||
| EnforcementState::with_default_unavailable( | ||
| ops::GET_PER_COMMITMENT_POINT, | ||
| || nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), &open_chan_msg) | ||
| ); | ||
|
|
||
| { | ||
| let msgs = nodes[1].node.get_and_clear_pending_msg_events(); | ||
| assert!(msgs.is_empty(), "Expected no message events; got {:?}", msgs); | ||
| } | ||
|
|
||
| let channel_id_1 = { | ||
| let channels = nodes[1].node.list_channels(); | ||
| assert_eq!(channels.len(), 1, "expected one channel, not {}", channels.len()); | ||
| channels[0].channel_id | ||
| }; | ||
|
|
||
| nodes[1].set_channel_signer_ops_available(&nodes[0].node.get_our_node_id(), &channel_id_1, ops::GET_PER_COMMITMENT_POINT, true); | ||
| nodes[1].node.signer_unblocked(None); | ||
|
|
||
| // nodes[0] <-- accept_channel --- nodes[1] | ||
| get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, nodes[0].node.get_our_node_id()); | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| fn do_test_funding_created(masks: &Vec<u32>) { | ||
| // Simulate acquiring the signature for `funding_created` asynchronously. | ||
| let chanmon_cfgs = create_chanmon_cfgs(2); | ||
| let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [52-242]
The tests for funding creation (test_funding_created_grs, test_funding_created_gsr, etc.) effectively simulate various scenarios of asynchronous signing during the funding creation process. These tests are crucial for ensuring that the channel state machine correctly handles asynchronous signing operations during one of the most critical phases of channel lifecycle management. To further improve these tests:
- Consider adding more detailed comments explaining the significance of each scenario being tested, especially the different combinations of operations being simulated as unavailable. This will help future maintainers and developers understand the rationale behind each test case and the specific conditions it aims to validate.
- Ensure that edge cases and error conditions are adequately tested. For example, scenarios where the signer becomes available after an extended period or where the signer fails to become available should be considered.
// Example of enhanced documentation for a specific test case.
/// Tests the funding creation process with the signer's operations for getting the per-commitment point,
/// releasing the commitment secret, and signing the counterparty's commitment being made unavailable in
/// a specific order. This test simulates the scenario where these operations are asynchronously handled
/// and ensures that the channel state machine can correctly proceed with the funding creation process
/// under these conditions.
#[test]
fn test_funding_created_grs() {
do_test_funding_created(&vec![ops::GET_PER_COMMITMENT_POINT, ops::RELEASE_COMMITMENT_SECRET, ops::SIGN_COUNTERPARTY_COMMITMENT]);
}
Comment on lines
400
to
1182
| }; | ||
|
|
||
| { | ||
| nodes[0].node.handle_channel_reestablish(&nodes[1].node.get_our_node_id(), &reestablish_2[0]); | ||
| match handle_chan_reestablish_msgs!(nodes[0], nodes[1]) { | ||
| (None, None, None, _) => (), | ||
| (channel_ready, revoke_and_ack, commitment_update, order) => { | ||
| panic!("got channel_ready={:?} revoke_and_ack={:?} commitment_update={:?} order={:?}", | ||
| channel_ready, revoke_and_ack, commitment_update, order); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &raa); | ||
| nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &cu.commitment_signed); | ||
| check_added_monitors!(nodes[0], 2); | ||
|
|
||
| // At this point Alice should provide Bob with the revoke_and_ack. | ||
| let raa = { | ||
| let events = nodes[0].node.get_and_clear_pending_msg_events(); | ||
| assert_eq!(events.len(), 1, "Expected 1 event, got {}: {:?}", events.len(), events); | ||
| match &events[0] { | ||
| MessageSendEvent::SendRevokeAndACK { msg: raa, .. } => raa.clone(), | ||
| ev => panic!("Expected SendRevokeAndACK, got {:?}", ev) | ||
| } | ||
| }; | ||
|
|
||
| nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &raa); | ||
| check_added_monitors!(nodes[1], 2); | ||
|
|
||
| expect_pending_htlcs_forwardable!(nodes[1]); | ||
| expect_payment_claimable!(nodes[1], payment_hash, payment_secret, 1_000_000); | ||
| } | ||
|
|
||
| #[test] | ||
| fn no_stray_channel_reestablish() { | ||
| // Original fuzz trace. | ||
| // a0 Disable A’s signer. | ||
| // 2c Disconnect A and B, then restart A. | ||
| // 0e Reconnect A and B. | ||
| // 2d Disconnect A and B (and C), then restart B. | ||
| // a1 Unblock A’s signer get_per_commitment_point | ||
| // ff Reset. | ||
|
|
||
| let chanmon_cfgs = create_chanmon_cfgs(2); | ||
| let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); | ||
| let alice_persister; | ||
| let bob_persister; | ||
| let alice_new_chain_monitor; | ||
| let bob_new_chain_monitor; | ||
|
|
||
| let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); | ||
| let alice_deserialized; | ||
| let bob_deserialized; | ||
|
|
||
| let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs); | ||
| let (_, _, channel_id, _) = create_announced_chan_between_nodes(&nodes, 0, 1); | ||
|
|
||
| // Turn off Alice's signer. | ||
| eprintln!("disabling alice's signer"); | ||
| nodes[0].set_channel_signer_ops_available( | ||
| &nodes[1].node.get_our_node_id(), &channel_id, | ||
| ops::GET_PER_COMMITMENT_POINT | ops::RELEASE_COMMITMENT_SECRET | ops::SIGN_COUNTERPARTY_COMMITMENT, | ||
| false); | ||
|
|
||
| // Disconnect Bob and restart Alice | ||
| eprintln!("disconnecting bob"); | ||
| nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id()); | ||
|
|
||
| eprintln!("restarting alice"); | ||
| { | ||
| let alice_serialized = nodes[0].node.encode(); | ||
| let alice_monitor_serialized = get_monitor!(nodes[0], channel_id).encode(); | ||
| reload_node!(nodes[0], *nodes[0].node.get_current_default_configuration(), &alice_serialized, &[&alice_monitor_serialized], alice_persister, alice_new_chain_monitor, alice_deserialized); | ||
| } | ||
|
|
||
| // Reconnect Alice and Bob. | ||
| eprintln!("reconnecting alice and bob"); | ||
| nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { | ||
| features: nodes[1].node.init_features(), networks: None, remote_network_address: None | ||
| }, false).unwrap(); | ||
|
|
||
| // Disconnect Alice and restart Bob | ||
| eprintln!("disconnecting alice"); | ||
| nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id()); | ||
|
|
||
| eprintln!("restarting bob"); | ||
| { | ||
| let bob_serialized = nodes[1].node.encode(); | ||
| let bob_monitor_serialized = get_monitor!(nodes[1], channel_id).encode(); | ||
| reload_node!(nodes[1], *nodes[1].node.get_current_default_configuration(), &bob_serialized, &[&bob_monitor_serialized], bob_persister, bob_new_chain_monitor, bob_deserialized); | ||
| } | ||
|
|
||
| eprintln!("unblocking alice's signer for get_per_commitment_point"); | ||
| nodes[0].set_channel_signer_ops_available(&nodes[1].node.get_our_node_id(), &channel_id, ops::GET_PER_COMMITMENT_POINT, true); | ||
| nodes[0].node.signer_unblocked(None); | ||
|
|
||
| let events = nodes[0].node.get_and_clear_pending_msg_events(); | ||
| assert!(events.is_empty(), "Expected no events from Alice, got {:?}", events); | ||
| } |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [52-1116]
The entire suite of tests in async_signer_tests.rs provides comprehensive coverage of various scenarios involving asynchronous signing in channel operations. To further refine these tests:
- Enhance documentation for each test function to explain the specific scenario being tested, the expected behavior, and the significance of the test in the context of asynchronous signing. This will aid in understanding and maintaining the test suite.
- Review and ensure that error handling is appropriately implemented, especially in scenarios where operations might fail due to the signer being unavailable. Consider how these failures are handled and whether the tests accurately reflect the expected behavior of the system under such conditions.
- Evaluate the coverage of edge cases and error conditions across all tests. Consider adding tests for scenarios that may not be currently covered, such as the signer failing to become available after being disabled or the system encountering unexpected errors during asynchronous operations.
// General approach for enhancing documentation and error handling in tests.
/// Tests the behavior of [specific operation] with an asynchronous signer.
///
/// This test simulates the scenario where [describe scenario], expecting the system to [expected behavior].
/// It is crucial for ensuring that [rationale for the test].
///
/// # Errors
/// This test verifies that the system can gracefully handle errors related to asynchronous signing by [describe error handling strategy].
fn example_test_function() {
// Test implementation with improved error handling and edge case coverage.
}
Comment on lines
+4801
to
+4815
| let our_commitment_transaction = INITIAL_COMMITMENT_NUMBER - self.context.holder_commitment.transaction_number() - 1; | ||
| if msg.next_remote_commitment_number > 0 { | ||
| let expected_point = self.context.holder_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - msg.next_remote_commitment_number + 1, &self.context.secp_ctx); | ||
| let given_secret = SecretKey::from_slice(&msg.your_last_per_commitment_secret) | ||
| .map_err(|_| ChannelError::Close("Peer sent a garbage channel_reestablish with unparseable secret key".to_owned()))?; | ||
| if expected_point != PublicKey::from_secret_key(&self.context.secp_ctx, &given_secret) { | ||
| return Err(ChannelError::Close("Peer sent a garbage channel_reestablish with secret key not matching the commitment height provided".to_owned())); | ||
| // TODO(waterson): figure out how to do this verification when an async signer is provided | ||
| // with a (more or less) arbitrary state index. Should we require that an async signer cache | ||
| // old points? Or should we make it so that we can restart the re-establish after the signer | ||
| // becomes unblocked? Or something else? | ||
| if false { | ||
| let state_index = INITIAL_COMMITMENT_NUMBER - msg.next_remote_commitment_number + 1; | ||
| let expected_point = self.context.holder_signer.as_ref().get_per_commitment_point(state_index, &self.context.secp_ctx) | ||
| .map_err(|_| ChannelError::Close(format!("Unable to retrieve per-commitment point for state {}", state_index)))?; | ||
| let given_secret = SecretKey::from_slice(&msg.your_last_per_commitment_secret) | ||
| .map_err(|_| ChannelError::Close("Peer sent a garbage channel_reestablish with unparseable secret key".to_owned()))?; | ||
| if expected_point != PublicKey::from_secret_key(&self.context.secp_ctx, &given_secret) { | ||
| return Err(ChannelError::Close("Peer sent a garbage channel_reestablish with secret key not matching the commitment height provided".to_owned())); | ||
| } |
There was a problem hiding this comment.
The commented-out code block related to verifying the per_commitment_point during channel reestablishment in the context of an asynchronous signer highlights an incomplete implementation that needs to be addressed. This is a critical piece of logic for ensuring the security and correctness of channel operations, especially in scenarios involving asynchronous signers. It's important to implement the necessary logic to handle this verification or provide a clear strategy for future implementation.
- // TODO(waterson): figure out how to do this verification when an async signer is provided
- // with a (more or less) arbitrary state index. Should we require that an async signer cache
- // old points? Or should we make it so that we can restart the re-establish after the signer
- // becomes unblocked? Or something else?
- if false {
- ...
- }Consider implementing the necessary logic to handle the verification of per_commitment_point with an asynchronous signer or provide a clear strategy for future implementation.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| let our_commitment_transaction = INITIAL_COMMITMENT_NUMBER - self.context.holder_commitment.transaction_number() - 1; | |
| if msg.next_remote_commitment_number > 0 { | |
| let expected_point = self.context.holder_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - msg.next_remote_commitment_number + 1, &self.context.secp_ctx); | |
| let given_secret = SecretKey::from_slice(&msg.your_last_per_commitment_secret) | |
| .map_err(|_| ChannelError::Close("Peer sent a garbage channel_reestablish with unparseable secret key".to_owned()))?; | |
| if expected_point != PublicKey::from_secret_key(&self.context.secp_ctx, &given_secret) { | |
| return Err(ChannelError::Close("Peer sent a garbage channel_reestablish with secret key not matching the commitment height provided".to_owned())); | |
| // TODO(waterson): figure out how to do this verification when an async signer is provided | |
| // with a (more or less) arbitrary state index. Should we require that an async signer cache | |
| // old points? Or should we make it so that we can restart the re-establish after the signer | |
| // becomes unblocked? Or something else? | |
| if false { | |
| let state_index = INITIAL_COMMITMENT_NUMBER - msg.next_remote_commitment_number + 1; | |
| let expected_point = self.context.holder_signer.as_ref().get_per_commitment_point(state_index, &self.context.secp_ctx) | |
| .map_err(|_| ChannelError::Close(format!("Unable to retrieve per-commitment point for state {}", state_index)))?; | |
| let given_secret = SecretKey::from_slice(&msg.your_last_per_commitment_secret) | |
| .map_err(|_| ChannelError::Close("Peer sent a garbage channel_reestablish with unparseable secret key".to_owned()))?; | |
| if expected_point != PublicKey::from_secret_key(&self.context.secp_ctx, &given_secret) { | |
| return Err(ChannelError::Close("Peer sent a garbage channel_reestablish with secret key not matching the commitment height provided".to_owned())); | |
| } | |
| let our_commitment_transaction = INITIAL_COMMITMENT_NUMBER - self.context.holder_commitment.transaction_number() - 1; | |
| if msg.next_remote_commitment_number > 0 { |
TheBlueMatt
reviewed
Jan 30, 2024
| @@ -5912,6 +5919,11 @@ where | |||
| emit_channel_ready_event!(pending_events, channel); | |||
| } | |||
|
|
|||
| log_debug!(self.logger, "Outgoing message queue is{}", if pending_msg_events.is_empty() { " empty" } else { "..." }); | |||
There was a problem hiding this comment.
nit: if you plan to leave this upstream (not sure if its really necessary?) it should definitely be TRACE.
| @@ -6464,6 +6493,7 @@ where | |||
| hash_map::Entry::Occupied(mut chan_phase_entry) => { | |||
| if let ChannelPhase::Funded(chan) = chan_phase_entry.get_mut() { | |||
| let logger = WithChannelContext::from(&self.logger, &chan.context); | |||
| log_debug!(logger, "<== channel_ready"); | |||
There was a problem hiding this comment.
I assume this and the below logs are just here for testing? Mind removing them?
| @@ -4228,7 +4427,12 @@ impl<SP: Deref> Channel<SP> where | |||
| } | |||
|
|
|||
| let raa = if self.context.monitor_pending_revoke_and_ack { | |||
| Some(self.get_last_revoke_and_ack()) | |||
| self.context.update_holder_commitment_secret(logger); | |||
| self.get_last_revoke_and_ack(logger).or_else(|| { | |||
There was a problem hiding this comment.
The cases where we have to remember to call update_holder_commitment_secret are not well documented and pretty awkward. I think what's required is update_holder_commitment_secret has to be called any time we're about to call get_last_revoke_and_ack, but if we'd previously called it then we only need to do so if signer_pending_released_secret is set.
If we require the signer always remember the last value, then I think we can drop update_holder_commitment_secret, signer_pending_released_secret, and prev_holder_commitment_secret entirely, inlining the signer call into get_last_revoke_and_ack and just leaning on signer_pending_revoke_and_ack. If we don't want to require that, I'd still really like to see this logic encapsulted somewhere inside signer_pending_revoke_and_ack so its easier to keep track of.
| .map_err(|_| ChannelError::Close("Peer sent a garbage channel_reestablish with unparseable secret key".to_owned()))?; | ||
| if expected_point != PublicKey::from_secret_key(&self.context.secp_ctx, &given_secret) { | ||
| return Err(ChannelError::Close("Peer sent a garbage channel_reestablish with secret key not matching the commitment height provided".to_owned())); | ||
| // TODO(waterson): figure out how to do this verification when an async signer is provided |
There was a problem hiding this comment.
Yea, this is super tricky. We have to be really careful in this case because we absolutely cannot end up in any cases where we can ChannelError::Close until we've validated the secret provided against the per-commitment-point we have locally, and Close errors are kinda everywhere...
What I think we can do is request the point, then generate some kind of error that causes a peer disconnect (and no more message processing) without handling the message. Then, the peer will just get stuck in a reconnect loop until our signer comes back and we can decide whether to close or panic. Should be a separate PR, though.
|
One more thing we need to add to docs here before we ship is note that the signing |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a do-over of #2653, wherein we support asynchronous signing for 'normal' channel operation. This involves allowing the following
ChannelSignermethods to return anErrresult, indicating that the requested value is not available:When the value does become available, channel operation can be resumed by invoking
signer_unblocked.Note that this adds the current and next per-commitment point to the state that is persisted by the channel monitor.