Support async get_per_commitment_point, release_commitment_secret #2653
Conversation
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2653 +/- ##
==========================================
- Coverage 88.49% 88.32% -0.18%
==========================================
Files 114 114
Lines 91935 92397 +462
Branches 91935 92397 +462
==========================================
+ Hits 81359 81610 +251
- Misses 8097 8257 +160
- Partials 2479 2530 +51 ☔ View full report in Codecov by Sentry. |
waterson
force-pushed
the
waterson/fallible-signer
branch
from
f95d24d
to
7dd9fda
Compare
waterson
force-pushed
the
waterson/fallible-signer
branch
from
7dd9fda
to
68c4b31
Compare
| // 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? |
There was a problem hiding this comment.
We don't need to cache old points because the secrets are obtained deterministically as long as the signer tracks the commitment_seed, which it must, otherwise you can't revoke any of your prior states to move the state machine forward.
I think in this case we can just cache the ChannelReestablish so we can re-process it later when signer_maybe_unblocked gets called. Some nodes may disconnect you if you take too long to respond to their ChannelReestablish (LDK waits up to 2 mins), but they'll just send it again (ideally with the same next_remote_commitment_number) upon reconnecting.
waterson
force-pushed
the
waterson/fallible-signer
branch
from
68c4b31
to
8c31c7f
Compare
waterson
force-pushed
the
waterson/fallible-signer
branch
from
dd1c436
to
ec3baf9
Compare
|
Put up a simple fuzz test coverage for this in the chanmon_consistency test at TheBlueMatt@aaadeec it seems to be finding some issues... |
lightning/src/sign/mod.rs
Outdated
| fn release_commitment_secret(&self, idx: u64) -> [u8; 32] { | ||
| chan_utils::build_commitment_secret(&self.commitment_seed, idx) | ||
| fn release_commitment_secret(&self, idx: u64) -> Result<[u8; 32], ()> { | ||
| Ok(chan_utils::build_commitment_secret(&self.commitment_seed, idx)) | ||
| } | ||
|
|
||
| fn validate_holder_commitment(&self, _holder_tx: &HolderCommitmentTransaction, _preimages: Vec<PaymentPreimage>) -> Result<(), ()> { |
There was a problem hiding this comment.
I think some functions should probably not be considered async, like this one. We aren't waiting for a response back from the signer, we're just giving it some data. There's no reason for that to be async.
There was a problem hiding this comment.
Are we talking about release_commitment_secret here? That actually is requesting the commitment secret for the previous state, right?
There was a problem hiding this comment.
validate_holder_commitment (and validate_counterparty_revocation). They're kinda confusing here cause we already have a defined use for the Err case - "that thing is invalid, close the channel!", we can't repurpose it for "working on this in the background" without a enum to capture states. For now I'd suggest we just leave them as non-async. We may need to figure it out later, though, cause the validation failing does mean we shouldn't apply the new holder commitment at all, and instead when we broadcast go use the previous one.
waterson
force-pushed
the
waterson/fallible-signer
branch
2 times, most recently
from
a82e126
to
5d7d5de
Compare
lightning/src/ln/channel.rs
Outdated
| } | ||
|
|
||
| // Make sure that we honor any ordering requirements between the commitment update and revoke-and-ack. | ||
| let (commitment_update, raa) = match &self.context.resend_order { |
There was a problem hiding this comment.
Is there a reason we care here? I dont think get_last_commitment_update_for_send or get_last_revoke_and_ack care about the order they're called in. We can just do the checking for if we should include them in an order match.
There was a problem hiding this comment.
So... no we don't care about the order they're called in but we must ensure that the following hold:
- If order is
CommitmentFirstand therevoke_and_ackis available but thecommitment_updateis not, we must not send therevoke_and_ack. - If order is
RevokeAndACKFirstand thecommitment_updateis available but therevoke_and_ackis not, we must not send thecommitment_update.
If we collapse this particular fragment and instead generate the CU and RAA without regard for order, some of the async signer tests will fail; in particular, those that unblock the signer in a way that causes one of the above to be violated. For example, if the order is CommitmentFirst, we can cause the signer to reveal the RAA first without revealing the CU.
This, combined with the fact that we clear the signer_pending_* bit, means that it is difficult to handle this in the ChannelManager, which admittedly feels like it might be a more appropriate place to do so.
But maybe there's a cleaner way to do this?
|
The fuzzer should probably at least have two or three set bytes, rather than only one, so that we can control setting disabling RAA + CS separately to test the ordering. |
|
Any updates here? Anything I can help with re: getting the fuzzer some time? |
waterson
force-pushed
the
waterson/fallible-signer
branch
5 times, most recently
from
6eabd1d
to
20851ff
Compare
|
Let me know when you're ready for a round of review here/when you're pretty confident in it. |
|
Needs rebase, sorry about that. |
|
Slipping to 0.0.120. |
waterson
force-pushed
the
waterson/fallible-signer
branch
3 times, most recently
from
40c918d
to
71e2ddf
Compare
waterson
force-pushed
the
waterson/fallible-signer
branch
from
71e2ddf
to
8830162
Compare
This PR adds state to the context to allow a `ChannelSigner` implementation to respond asynchronously to the `get_per_commitment_point`, `release_commitment_secret`, and `sign_counterparty_commitment` methods, which are the main signer methods that are called during channel setup and normal operation.
waterson
force-pushed
the
waterson/fallible-signer
branch
from
8830162
to
2c7ca7f
Compare
| /// signer wasn't ready, then this will be set to `true`. | ||
| signer_pending_released_secret: bool, | ||
| /// If we need to send a `channel_reestablish` then this will be set to `true`. | ||
| signer_pending_channel_reestablish: bool, |
There was a problem hiding this comment.
Hmm, I'm kinda sad just how much we had to make interruptible here. Digging deep into the channel utilities like build_commitment_transaction is a super invasive change, not to mention changing the definition of connected between the state flags and the is_connected function is gonna cause some confusion later.
I do wonder if we added an extra buffered commitment point if we could work around this. Rather than only storing the current commitment point for use in the next commitment transaction/channel_reestablish, store both the current one and the next one. We'd then block revoke_and_ack sending until we've received the new-next commitment point (and maybe make release_commitment_secret return a tuple rather than calling it and get_per_commitment_point). From there, we always know we can call commitment_signed (at least in the same contexts that we could previously). I may be totally missing something here.
There was a problem hiding this comment.
Let me know if I reopened a discussion we'd already had. I know we had dual-point-caching originally, did I suggest we drop that?
There was a problem hiding this comment.
I feel like there was a reason that we wanted to be able to get the next per-commitment point without releasing the previous commitment secret, but I'm not remembering the details.
Agreed that this has gotten messy. Let me see if I can drive it to correctness and then maybe we can work through what it might take to refactor and simplify.
waterson
force-pushed
the
waterson/fallible-signer
branch
4 times, most recently
from
bdf6847
to
2bc710a
Compare
Forget pending signature requests that will be recomputed when we receive a `channel_reestablish`. This ensures that we don't e.g. send a `revoke_and_ack` and `commitment_update` twice: once when signing is unblocked, and again when receiving the counterparty's reestablish message. It also ensures that we don't send a stray `channel_reestablish` if we are recconected, and then disconnected again before making any progress.
When we get a `channel_reestablish` that requires a `channel_ready`, don't respond with the `commitment_signed` or `revoke_and_ack` until we have the `channel_ready` available to send.
After we've verified that payments will succeed, flush the messages so that the next test starts with all the message queues empty.
waterson
force-pushed
the
waterson/fallible-signer
branch
from
2bc710a
to
7b77771
Compare
WalkthroughThe updates across the Rust codebase focus on enhancing asynchronous operations, particularly around signing. Changes include better handling of channel state and peer connections, direct assignment for commitment points, and more robust error handling. Logging improvements provide clearer tracking of message queues and channel events, while modifications to the signer interface and test utilities reflect a shift towards more explicit error management. Changes
Poem
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: 32
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 (81 hunks)
- lightning/src/ln/channelmanager.rs (18 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
Files skipped from review due to trivial changes (1)
- lightning/src/chain/channelmonitor.rs
Additional comments: 77
lightning/src/util/test_channel_signer.rs (10)
- 73-90: The introduction of a bitmask to enable or disable specific operations is a good approach for granular control over the
TestChannelSignerbehavior. This change allows for more precise testing scenarios.- 157-164: The
set_ops_availablemethod correctly updates theunavailable_signer_opsbitmask to reflect the availability of operations. The use of bitwise operations here is appropriate and efficient.- 166-167: The
is_signer_availablemethod is a straightforward check against theunavailable_signer_opsbitmask. It's a clean and efficient way to determine if a signer operation is available.- 172-175: The
get_per_commitment_pointmethod has been correctly updated to return aResulttype, allowing for error handling in line with the asynchronous nature of the operation.- 179-182: The
release_commitment_secretmethod now returns aResulttype, which is consistent with the other changes made to support asynchronous operations. The assertions within the method ensure that the commitment index is within expected bounds.- 193-195: The
validate_holder_commitmentmethod's update to return aResulttype is in line with the new error handling pattern. The assertions check for the correct commitment number sequence.- 204-204: The
validate_counterparty_revocationmethod has been updated to return aResulttype, aligning with the other methods and allowing for proper error handling.- 227-227: The
sign_counterparty_commitmentmethod correctly checks for signer availability and updates the state with the new commitment number. The assertions enforce the expected commitment number sequence.- 246-246: The
sign_holder_commitmentmethod includes a check for signer availability and correctly enforces the policy regarding the revocation of commitments. The panic condition is guarded by a flag, allowing for flexibility in testing scenarios.- 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 a bitmask for tracking the availability of signer operations. This change is consistent with the overall approach of using bitmasks for finer control over the signer's behavior.lightning/src/chain/onchaintx.rs (2)
- 181-181: The addition of
per_commitment_pointtoExternalHTLCClaimstruct is consistent with the summary.- 1181-1185: The setting of
per_commitment_pointingenerate_external_htlc_claimis consistent with the summary and the addition of the field to the struct.fuzz/src/chanmon_consistency.rs (19)
- 52-52: The import of
opsfromlightning::util::test_channel_signeris new and is used to defineASYNC_OPSconstant.- 76-76: The
ASYNC_OPSconstant is added to combine multiple operations flags. This is a good use of bitflags for managing combinations of options.- 833-833: The comment inside the macro
process_msg_events!is helpful for understanding the purpose of the code block, which is to deliverupdate_add_htlcmessages to nodes.- 848-848: Similar to the previous comment, this one explains the delivery of
update_fulfill_htlcmessages.- 852-852: This comment explains the delivery of
update_fail_htlcmessages.- 856-856: This comment explains the delivery of
update_fail_malformed_htlcmessages.- 860-860: This comment explains the delivery of
update_feemessages.- 877-877: This comment explains the delivery of
commitment_signedmessages.- 886-886: This comment explains the delivery of
revoke_and_ackmessages.- 894-894: This comment explains the delivery of
channel_reestablishmessages.- 917-917: The panic message here is a catch-all for unhandled message events, which is a good practice to ensure that all possible message types are accounted for during testing.
- 1294-1320: The
#[cfg(async_signing)]conditional compilation attribute is used to ensure that the code related to asynchronous signing is only compiled when theasync_signingfeature is enabled. This is a good practice for feature-gated code paths.- 1322-1376: Similar to the previous comment, these blocks are also gated by the
async_signingfeature and handle setting the signer availability for different operations. It's important to ensure that the feature is properly tested when enabled.- 1378-1404: Again, these blocks are gated by the
async_signingfeature and handle setting the signer availability for different operations for thekeys_manager_c. Consistency in handling signer availability across different key managers is crucial.- 1410-1410: The comment here indicates the start of a block that ensures no channel is in a stuck state. This is a critical part of the test to ensure that channels can recover from various states.
- 1452-1473: The block of code under
#[cfg(async_signing)]ensures that all signers are set to available, which is necessary for the final part of the test where the channels are restored to normal operation.- 1488-1521: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1474-1518]
This loop runs the event queues to quiescence, which is a necessary step to ensure that all pending events are processed and the state is settled before the test concludes.
- 1491-1492: The comment indicates successful restoration of channels to normal operation, which is the expected outcome of the test.
- 1504-1518: The loop here is a continuation of the previous event processing loop, ensuring that all messages are flushed and the state is fully settled.
lightning/src/ln/async_signer_tests.rs (17)
- 13-22: The imports and module documentation are standard and appropriate.
- 24-50: The
with_async_signerfunction is a test helper that simulates an asynchronous signer by disabling the signer for a specified channel, running a provided function, and then re-enabling the signer and callingsigner_unblocked. The logic seems correct, but it's important to ensure that thedo_fndoes not depend on the signer being available, as it is explicitly disabled before callingdo_fn.- 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 tests
do_test_funding_createdand its variations (test_funding_created_grs,test_funding_created_gsr, etc.) are designed to simulate acquiring the signature forfunding_createdasynchronously. The tests are well-structured and cover different orderings of operations. However, it's crucial that the tests are comprehensive enough to cover all edge cases, especially given the asynchronous nature of the operations involved.
- 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 tests
do_test_funding_signedand its variations follow a similar pattern to thedo_test_funding_createdtests, simulating the asynchronous acquisition of the signature forfunding_signed. The structure and coverage appear to be consistent with the previous set of tests.
- 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 tests
do_test_commitment_signedand its variations are designed to simulate the asynchronous signing ofcommitment_signed. The tests are consistent with the previous patterns and seem to cover the necessary scenarios.
- 349-1605: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [265-379]
The tests
do_test_funding_signed_0confand its variations simulate the asynchronous signing forfunding_signedin a zero-conf channel. It's important to ensure that the zero-conf channel behavior is correctly simulated and that the tests accurately reflect the expected behavior in such scenarios.
- 383-571: The tests
do_test_paymentand its variations simulate a one-hop payment with an asynchronous signer at each step. The tests should ensure that the payment lifecycle is correctly handled, even with the signer being asynchronous.- 574-743: The tests
do_test_peer_reconnectand its variations simulate peer reconnection with an asynchronous signer. It's crucial that these tests accurately simulate the reconnection process and handle the asynchronous nature of the signer correctly, ensuring that the channel state is consistent post-reconnection.- 746-850: The
channel_update_fee_testfunction tests the channel's ability to handle a fee update when the signer is unavailable. It's important to ensure that the channel can still operate correctly under these conditions and that the test covers all relevant scenarios.- 852-950: The
reconnect_while_awaiting_commitment_pointtest simulates a scenario where a peer reconnects while the signer is awaiting a commitment point. This test is important for ensuring that the channel state machine can handle such scenarios without losing state or causing channel failures.- 953-1029: The
peer_restart_with_blocked_signertest checks the behavior of the system when a peer restarts with a blocked signer. It's important to ensure that the system can handle restarts gracefully and that the signer's state is correctly managed across restarts.- 1032-1244: The
peer_restart_with_blocked_signer_and_pending_paymenttest simulates a peer restart with a blocked signer and a pending payment. This test is crucial for ensuring that pending payments are not lost or invalidated due to signer issues during a peer restart.- 1132-1244: The
peer_restart_with_blocked_signer_before_pending_paymenttest ensures that the system behaves correctly when a peer restarts with a blocked signer before a pending payment is processed. This test is important for ensuring that the payment lifecycle is not disrupted by signer issues.- 1248-1311: The
no_stray_channel_reestablishtest seems to be a fuzz test to ensure that no unexpectedchannel_reestablishmessages are sent under certain conditions. It's important that fuzz tests like this one are comprehensive and cover a wide range of scenarios to catch potential issues.- 1315-1415: The
dont_elide_channely_ready_from_state_1test ensures that achannel_readymessage is not omitted when it should be sent. This test is important for ensuring that the channel state machine behaves correctly and follows the protocol.- 1418-1513: The
dont_lose_commitment_updatetest ensures that acommitment_updateis not lost in the event of a signer being disabled and a peer restart. This test is crucial for ensuring the reliability of the channel state machine in the face of signer issues.- 1516-1603: The
dont_lose_commitment_update_reduxtest is similar to the previousdont_lose_commitment_updatetest and ensures that commitment updates are not lost under certain conditions. It's important that this test and the previous one are not redundant and that they cover different scenarios.lightning/src/sign/mod.rs (4)
- 577-580: The
get_per_commitment_pointmethod now returns aResult<PublicKey, ()>instead ofPublicKey. This change allows for asynchronous operations by enabling the method to signal a failure to produce a commitment point, which the caller must handle appropriately.- 591-591: The
release_commitment_secretmethod now returns aResult<[u8; 32], ()>instead of[u8; 32]. Similar toget_per_commitment_point, this change supports asynchronous operations by allowing the method to signal a failure to produce a commitment secret.- 1083-1085: The implementation of
get_per_commitment_pointinInMemorySignercorrectly returns aResult<PublicKey, ()>. It unwraps the secret key from the slice, which is guaranteed to be correct due to the fixed size ofcommitment_seed, and then derives the public key.- 1088-1089: The implementation of
release_commitment_secretinInMemorySignercorrectly returns aResult<[u8; 32], ()>. It uses thebuild_commitment_secretfunction to derive the secret, which should not fail given the deterministic nature of the derivation process.lightning/src/ln/functional_test_utils.rs (4)
- 35-36: The import of
opsfromtest_channel_signeris added.- 451-451: The function
set_channel_signer_ops_availablehas been renamed and its parameters have been modified to include amaskparameter.- 467-470: The logging statement within
set_channel_signer_ops_availablehas been updated to include themaskparameter.- 472-481: A new function
forget_signer_materialhas been added to theNodestruct.lightning/src/ln/functional_tests.rs (5)
- 723-723: Using
unwrap()directly in tests is acceptable for unwrappingResulttypes when the test should panic if an error occurs. However, consider if there is a need to provide a more descriptive message usingexpect()for better debugging.- 1445-1446: Similar to the previous comment,
unwrap()is used here. Ensure that the use ofunwrap()aligns with the intent of the test. If the test should not proceed in the case of an error,unwrap()is suitable. Otherwise, consider handling the error more gracefully.- 1458-1458: This change is consistent with the previous ones, adding
unwrap()to handle theResulttype. Ensure that this is the desired behavior for the test.- 7798-7798: The use of
unwrap()here is consistent with the intent to panic if an error occurs. This is typical in test scenarios where a failure to retrieve a secret would indicate a critical error.- 7802-7802: Here,
expect()is used with a descriptive message, which is good practice for debugging. It provides clarity on why the test might panic at this point.lightning/src/ln/channelmanager.rs (16)
- 641-641: The addition of
Debugto theRAACommitmentOrderenum is a good practice for enums that may be logged or otherwise output during debugging.- 2541-2541: Making the
channelvariable mutable is necessary for the subsequent operations that modify its state. Ensure that the mutability ofchanneldoes not introduce any unintended side effects.- 2556-2560: The handling of
opt_msgbeingNonewith logging and setting a flag is a good approach to manage asynchronous operations. Ensure that the flagsigner_pending_open_channelis appropriately cleared elsewhere in the code.Verification successful
The provided context confirms that the flag
signer_pending_open_channelis cleared after the signer is no longer pending, as indicated by the log message and the conditional logic in the code. This aligns with the original review comment's concern, verifying that the flag is appropriately managed.* 2574-2579: The conditional logic for sending the `OpenChannel` message is correct. Ensure that the message queuing and sending mechanism is robust and handles failure scenarios appropriately. * 5914-5919: Adding logging statements to track the outgoing message queue is useful for debugging. Ensure that the logging level is appropriate and that it does not lead to log flooding in production. * 6071-6081: The handling of `accept_inbound_channel` with conditional logic and logging is consistent with the previous hunk. Again, verify that the flag `signer_pending_accept_channel` is managed correctly throughout the code lifecycle. * 6233-6243: This hunk is similar to the previous one regarding `accept_inbound_channel`. The same verification applies here. * 6305-6310: The error handling in `funding_created` with logging and re-insertion of the channel in case of a transient error is a good practice. Ensure that the error handling is consistent and that all transient errors are handled similarly. * 6698-6698: The logging for `update_add_htlc` is consistent with the previous logging comments. Ensure that the log message is appropriately detailed and consistent. * 6724-6724: The logging for `update_fulfill_htlc` is consistent with the previous logging comments. Ensure that the log message is appropriately detailed and consistent. * 6823-6823: The logging for `commitment_signed` is consistent with the previous logging comments. Ensure that the log message is appropriately detailed and consistent. * 6998-6998: The logging for `revoke_and_ack` is consistent with the previous logging comments. Ensure that the log message is appropriately detailed and consistent. * 7149-7149: The logging for `channel_reestablish` is consistent with the previous logging comments. Ensure that the log message is appropriately detailed and consistent. * 7381-7423: The logic for queuing messages based on the signer being unblocked and the message ordering is complex. Ensure that the message ordering is preserved and that the logic is thoroughly tested, especially since message ordering is critical in the channel state machine.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the flag `signer_pending_open_channel` is cleared after the signer is no longer pending. rg --type rust "signer_pending_open_channel = false"Length of output: 128
Script:
#!/bin/bash # Output the context around where `signer_pending_open_channel` is set to false to verify it's in the correct context. rg --type rust -C 5 "signer_pending_open_channel = false" lightning/src/ln/channel.rsLength of output: 537
Verification successful
The verification process has identified assertions in the test suite that check the
RAACommitmentOrder, indicating that the message ordering logic is indeed being tested. While this does not guarantee the thoroughness of the tests, it is a strong indicator that the message ordering is part of the test suite and is being verified.* 9070-9076: The logic for queuing `channel_reestablish` messages is consistent with the previous comments on message queuing. Ensure that the message queuing is robust and tested. * 9154-9160: The handling of `maybe_handle_error_without_close` with conditional message queuing is appropriate. Ensure that the error handling logic is consistent and that all similar scenarios are handled in the same way.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the message ordering logic is preserved and thoroughly tested. rg --type rust "RAACommitmentOrder"Length of output: 4763
Script:
#!/bin/bash # Search for assertions or tests related to RAACommitmentOrder in the test files. rg --type rust "assert.*RAACommitmentOrder" -A 2 -B 2Length of output: 2480
Verification successful
The verification process has confirmed that the handling of
maybe_handle_error_without_closewith conditional message queuing inchannelmanager.rsis consistent within the codebase, as this is the only occurrence of its usage. No other similar scenarios were found that handle the output of this method differently.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify consistent error handling logic for similar scenarios. rg --type rust "maybe_handle_error_without_close"Length of output: 275
Script:
#!/bin/bash # Find all occurrences of `maybe_handle_error_without_close` being called and display the surrounding code for context. rg --type rust --context 5 "maybe_handle_error_without_close"Length of output: 1896
|
|
||
| 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 double unwrap() is potentially problematic. If the first unwrap() fails, it will panic, and the second unwrap() will never be reached. If the intent is to panic on the first error, this is fine, but it's a bit unusual to see chained unwrap() calls. Consider if this is the best approach or if a single expect() with a descriptive message would be more appropriate.
- &SecretKey::from_slice(&keys.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 2).unwrap()).unwrap());
+ &SecretKey::from_slice(&keys.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 2).expect("unable to release commitment secret and convert to SecretKey"));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.
| &SecretKey::from_slice(&keys.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 2).unwrap()).unwrap()); | |
| &SecretKey::from_slice(&keys.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 2).expect("unable to release commitment secret and convert to SecretKey")); |
| #[cfg(test)] | ||
| pub fn string_from(mask: u32) -> String { | ||
| if mask == 0 { | ||
| return "nothing".to_owned(); | ||
| } | ||
| if mask == !(0 as u32) { | ||
| return "everything".to_owned(); | ||
| } | ||
|
|
||
| vec![ | ||
| if (mask & GET_PER_COMMITMENT_POINT) != 0 { Some("get_per_commitment_point") } else { None }, | ||
| if (mask & RELEASE_COMMITMENT_SECRET) != 0 { Some("release_commitment_secret") } else { None }, | ||
| if (mask & VALIDATE_HOLDER_COMMITMENT) != 0 { Some("validate_holder_commitment") } else { None }, | ||
| if (mask & SIGN_COUNTERPARTY_COMMITMENT) != 0 { Some("sign_counterparty_commitment") } else { None }, | ||
| if (mask & VALIDATE_COUNTERPARTY_REVOCATION) != 0 { Some("validate_counterparty_revocation") } else { None }, | ||
| if (mask & SIGN_HOLDER_COMMITMENT_AND_HTLCS) != 0 { Some("sign_holder_commitment_and_htlcs") } else { None }, | ||
| if (mask & SIGN_JUSTICE_REVOKED_OUTPUT) != 0 { Some("sign_justice_revoked_output") } else { None }, | ||
| if (mask & SIGN_JUSTICE_REVOKED_HTLC) != 0 { Some("sign_justice_revoked_htlc") } else { None }, | ||
| if (mask & SIGN_HOLDER_HTLC_TRANSACTION) != 0 { Some("sign_holder_htlc_transaction") } else { None }, | ||
| if (mask & SIGN_COUNTERPARTY_HTLC_TRANSATION) != 0 { Some("sign_counterparty_htlc_transation") } else { None }, | ||
| if (mask & SIGN_CLOSING_TRANSACTION) != 0 { Some("sign_closing_transaction") } else { None }, | ||
| if (mask & SIGN_HOLDER_ANCHOR_INPUT) != 0 { Some("sign_holder_anchor_input") } else { None }, | ||
| if (mask & SIGN_CHANNEL_ANNOUNCMENT_WITH_FUNDING_KEY) != 0 { Some("sign_channel_announcment_with_funding_key") } else { None }, | ||
| ].iter().flatten().map(|s| s.to_string()).collect::<Vec<_>>().join(", ") | ||
| } |
There was a problem hiding this comment.
The string_from function provides a clear mapping from the bitmask to human-readable strings, which is useful for debugging and logging purposes. However, there's a typo in the string "sign_counterparty_htlc_transation" which should be "sign_counterparty_htlc_transaction".
- if (mask & SIGN_COUNTERPARTY_HTLC_TRANSATION) != 0 { Some("sign_counterparty_htlc_transation") } else { None },
+ if (mask & SIGN_COUNTERPARTY_HTLC_TRANSATION) != 0 { Some("sign_counterparty_htlc_transaction") } else { None },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.
| #[cfg(test)] | |
| pub fn string_from(mask: u32) -> String { | |
| if mask == 0 { | |
| return "nothing".to_owned(); | |
| } | |
| if mask == !(0 as u32) { | |
| return "everything".to_owned(); | |
| } | |
| vec![ | |
| if (mask & GET_PER_COMMITMENT_POINT) != 0 { Some("get_per_commitment_point") } else { None }, | |
| if (mask & RELEASE_COMMITMENT_SECRET) != 0 { Some("release_commitment_secret") } else { None }, | |
| if (mask & VALIDATE_HOLDER_COMMITMENT) != 0 { Some("validate_holder_commitment") } else { None }, | |
| if (mask & SIGN_COUNTERPARTY_COMMITMENT) != 0 { Some("sign_counterparty_commitment") } else { None }, | |
| if (mask & VALIDATE_COUNTERPARTY_REVOCATION) != 0 { Some("validate_counterparty_revocation") } else { None }, | |
| if (mask & SIGN_HOLDER_COMMITMENT_AND_HTLCS) != 0 { Some("sign_holder_commitment_and_htlcs") } else { None }, | |
| if (mask & SIGN_JUSTICE_REVOKED_OUTPUT) != 0 { Some("sign_justice_revoked_output") } else { None }, | |
| if (mask & SIGN_JUSTICE_REVOKED_HTLC) != 0 { Some("sign_justice_revoked_htlc") } else { None }, | |
| if (mask & SIGN_HOLDER_HTLC_TRANSACTION) != 0 { Some("sign_holder_htlc_transaction") } else { None }, | |
| if (mask & SIGN_COUNTERPARTY_HTLC_TRANSATION) != 0 { Some("sign_counterparty_htlc_transation") } else { None }, | |
| if (mask & SIGN_CLOSING_TRANSACTION) != 0 { Some("sign_closing_transaction") } else { None }, | |
| if (mask & SIGN_HOLDER_ANCHOR_INPUT) != 0 { Some("sign_holder_anchor_input") } else { None }, | |
| if (mask & SIGN_CHANNEL_ANNOUNCMENT_WITH_FUNDING_KEY) != 0 { Some("sign_channel_announcment_with_funding_key") } else { None }, | |
| ].iter().flatten().map(|s| s.to_string()).collect::<Vec<_>>().join(", ") | |
| } | |
| #[cfg(test)] | |
| pub fn string_from(mask: u32) -> String { | |
| if mask == 0 { | |
| return "nothing".to_owned(); | |
| } | |
| if mask == !(0 as u32) { | |
| return "everything".to_owned(); | |
| } | |
| vec![ | |
| if (mask & GET_PER_COMMITMENT_POINT) != 0 { Some("get_per_commitment_point") } else { None }, | |
| if (mask & RELEASE_COMMITMENT_SECRET) != 0 { Some("release_commitment_secret") } else { None }, | |
| if (mask & VALIDATE_HOLDER_COMMITMENT) != 0 { Some("validate_holder_commitment") } else { None }, | |
| if (mask & SIGN_COUNTERPARTY_COMMITMENT) != 0 { Some("sign_counterparty_commitment") } else { None }, | |
| if (mask & VALIDATE_COUNTERPARTY_REVOCATION) != 0 { Some("validate_counterparty_revocation") } else { None }, | |
| if (mask & SIGN_HOLDER_COMMITMENT_AND_HTLCS) != 0 { Some("sign_holder_commitment_and_htlcs") } else { None }, | |
| if (mask & SIGN_JUSTICE_REVOKED_OUTPUT) != 0 { Some("sign_justice_revoked_output") } else { None }, | |
| if (mask & SIGN_JUSTICE_REVOKED_HTLC) != 0 { Some("sign_justice_revoked_htlc") } else { None }, | |
| if (mask & SIGN_HOLDER_HTLC_TRANSACTION) != 0 { Some("sign_holder_htlc_transaction") } else { None }, | |
| if (mask & SIGN_COUNTERPARTY_HTLC_TRANSATION) != 0 { Some("sign_counterparty_htlc_transaction") } else { None }, | |
| if (mask & SIGN_CLOSING_TRANSACTION) != 0 { Some("sign_closing_transaction") } else { None }, | |
| if (mask & SIGN_HOLDER_ANCHOR_INPUT) != 0 { Some("sign_holder_anchor_input") } else { None }, | |
| if (mask & SIGN_CHANNEL_ANNOUNCMENT_WITH_FUNDING_KEY) != 0 { Some("sign_channel_announcment_with_funding_key") } else { None }, | |
| ].iter().flatten().map(|s| s.to_string()).collect::<Vec<_>>().join(", ") | |
| } |
| @@ -6423,6 +6457,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.
Tip
Codebase Verification
The log messages throughout the codebase include various contextual pieces of information such as node IDs, channel IDs, transaction IDs, and specific actions being taken or events occurring. They often follow a structure where the context (like an ID or a state) is provided followed by a description of the event or action.
The log message in question, log_debug!(logger, "<== channel_ready");, is minimal and does not include specific identifiers or detailed context. It indicates that a channel_ready event has occurred but does not specify which channel or any other details that might be relevant for debugging purposes.
Comparing this to other log messages, it seems less informative. For example, other log messages include additional context such as "Handling SendAcceptChannel event in peer_handler for node {} for channel {}", which includes both the node and channel identifiers.
Based on the patterns observed in the codebase, the log message in question could be improved by adding more context, such as the channel ID or node ID, to align with the level of detail seen in other log messages.
Given this analysis, the review comment is justified as the log message could indeed provide more context to be consistent with other similar log messages in the codebase.
Analysis chain
Logging the receipt of channel_ready is useful for debugging. Ensure that the log message provides enough context to be useful and that it is consistent with other similar log messages.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistency and context in log messages.
rg --type rust "log_debug!"
Length of output: 23578
| channel.context.update_holder_per_commitment_point(&args.logger); | ||
| // channel.context.update_holder_commitment_secret(&args.logger); |
There was a problem hiding this comment.
The commented-out line // channel.context.update_holder_commitment_secret(&args.logger); suggests incomplete implementation or a temporary change. Clarify the intention behind this commented code and ensure it is addressed before merging.
Remove commented-out code or implement the necessary logic if it's part of the intended changes.
| @@ -727,7 +727,7 @@ struct HTLCStats { | |||
|
|
|||
| /// An enum gathering stats on commitment transaction, either local or remote. | |||
| struct CommitmentStats<'a> { | |||
| tx: CommitmentTransaction, // the transaction info | |||
| tx: Option<CommitmentTransaction>, // the transaction info | |||
There was a problem hiding this comment.
CommitmentStats struct is missing documentation for its fields.
struct CommitmentStats<'a> {
+ /// The transaction info, if available.
tx: Option<CommitmentTransaction>,
+ /// The feerate per kiloweight used to build the transaction.
feerate_per_kw: u32,
+ /// The total fee, in satoshis, included in the transaction.
total_fee_sat: u64,
+ /// The number of HTLC outputs (excluding dust HTLCs) included in the transaction.
num_nondust_htlcs: usize,
}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.
| tx: Option<CommitmentTransaction>, // the transaction info | |
| struct CommitmentStats<'a> { | |
| /// The transaction info, if available. | |
| tx: Option<CommitmentTransaction>, | |
| /// The feerate per kiloweight used to build the transaction. | |
| feerate_per_kw: u32, | |
| /// The total fee, in satoshis, included in the transaction. | |
| total_fee_sat: u64, | |
| /// The number of HTLC outputs (excluding dust HTLCs) included in the transaction. | |
| num_nondust_htlcs: usize, | |
| } |
| let channel_ready = if funding_signed.is_some() { | ||
| self.check_get_channel_ready(0) | ||
|
|
||
| // Don't yield up a `channel_ready` message if we're still pending funding. | ||
| let channel_ready = if self.context.signer_pending_channel_ready && !self.context.signer_pending_funding { | ||
| log_trace!(logger, "Attempting to generate pending channel ready..."); | ||
| self.get_channel_ready().map(|msg| { | ||
| log_trace!(logger, "Generated channel_ready; clearing signer_pending_channel_ready"); | ||
| self.context.signer_pending_channel_ready = false; | ||
| msg | ||
| }) | ||
| } else { None }; | ||
|
|
||
| log_trace!(logger, "Signer unblocked with {} commitment_update, {} funding_signed and {} channel_ready", | ||
| // If we're pending a channel_ready to the counterparty, then we can't be sending commitment | ||
| // updates. If we're not, then make sure that we honor any ordering requirements between the | ||
| // commitment update and revoke-and-ack. | ||
| let (commitment_update, raa) = if false && self.context.signer_pending_channel_ready { | ||
| (None, None) | ||
| } else { | ||
| match &self.context.resend_order { | ||
| RAACommitmentOrder::CommitmentFirst => { | ||
| let cu = if self.context.signer_pending_commitment_update { | ||
| log_trace!(logger, "Attempting to generate pending commitment update..."); | ||
| self.get_last_commitment_update_for_send(logger).map(|cu| { | ||
| log_trace!(logger, "Generated commitment update; clearing signer_pending_commitment_update"); | ||
| self.context.signer_pending_commitment_update = false; | ||
| cu | ||
| }).ok() | ||
| } else { None }; | ||
|
|
||
| let raa = if self.context.signer_pending_revoke_and_ack && !self.context.signer_pending_commitment_update { | ||
| log_trace!(logger, "Attempting to generate pending RAA..."); | ||
| self.get_last_revoke_and_ack(logger).map(|raa| { | ||
| log_trace!(logger, "Generated RAA; clearing signer_pending_revoke_and_ack"); | ||
| self.context.signer_pending_revoke_and_ack = false; | ||
| raa | ||
| }) | ||
| } else { None }; | ||
|
|
||
| (cu, raa) | ||
| } | ||
|
|
||
| RAACommitmentOrder::RevokeAndACKFirst => { | ||
| let raa = if self.context.signer_pending_revoke_and_ack { | ||
| log_trace!(logger, "Attempting to generate pending RAA..."); | ||
| self.get_last_revoke_and_ack(logger).map(|raa| { | ||
| log_trace!(logger, "Generated RAA; clearing signer_pending_revoke_and_ack"); | ||
| self.context.signer_pending_revoke_and_ack = false; | ||
| raa | ||
| }) | ||
| } else { None }; | ||
|
|
||
| let cu = if self.context.signer_pending_commitment_update && !self.context.signer_pending_revoke_and_ack { | ||
| log_trace!(logger, "Attempting to generate pending commitment update..."); | ||
| self.get_last_commitment_update_for_send(logger).map(|cu| { | ||
| log_trace!(logger, "Generated commitment update; clearing signer_pending_commitment_update"); | ||
| self.context.signer_pending_commitment_update = false; | ||
| cu | ||
| }).ok() | ||
| } else { None }; | ||
|
|
||
| (cu, raa) | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| let order = self.context.resend_order.clone(); | ||
|
|
||
| log_debug!(logger, "Signing unblocked in channel {} at sequence {} resulted in {} channel reestablish, {} commitment update, {} RAA{}, {} funding signed, {} channel ready", | ||
| &self.context.channel_id(), self.context.cur_holder_commitment_transaction_number, | ||
| if channel_reestablish.is_some() { "a" } else { "no" }, | ||
| if commitment_update.is_some() { "a" } else { "no" }, | ||
| if raa.is_some() { "an" } else { "no" }, | ||
| if commitment_update.is_some() && raa.is_some() { | ||
| if order == RAACommitmentOrder::CommitmentFirst { " (commitment first)" } else { " (RAA first)" } | ||
| } else { "" }, | ||
| if funding_signed.is_some() { "a" } else { "no" }, | ||
| if channel_ready.is_some() { "a" } else { "no" }); | ||
|
|
||
| SignerResumeUpdates { | ||
| commitment_update, | ||
| raa, | ||
| order, | ||
| funding_signed, | ||
| channel_ready, | ||
| channel_reestablish, | ||
| } | ||
| } | ||
|
|
||
| fn get_last_revoke_and_ack(&self) -> msgs::RevokeAndACK { | ||
| let next_per_commitment_point = self.context.holder_signer.as_ref().get_per_commitment_point(self.context.cur_holder_commitment_transaction_number, &self.context.secp_ctx); | ||
| let per_commitment_secret = self.context.holder_signer.as_ref().release_commitment_secret(self.context.cur_holder_commitment_transaction_number + 2); | ||
| msgs::RevokeAndACK { | ||
| channel_id: self.context.channel_id, | ||
| per_commitment_secret, | ||
| next_per_commitment_point, | ||
| #[cfg(taproot)] | ||
| next_local_nonce: None, | ||
| fn get_last_revoke_and_ack<L: Deref>(&self, logger: &L) -> Option<msgs::RevokeAndACK> where L::Target: Logger { |
There was a problem hiding this comment.
The method signer_maybe_unblocked is missing documentation. It's unclear what the purpose of this method is and how it should be used.
+ /// Called when the signer may have become unblocked, to attempt to generate any pending messages.
#[cfg(async_signing)]
pub fn signer_maybe_unblocked<L: Deref>(&mut self, logger: &L) -> SignerResumeUpdates where L::Target: Logger {
// ... existing code ...
}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.
| log_trace!(logger, "Signing unblocked in channel {} at sequence {}", | |
| &self.context.channel_id(), self.context.cur_holder_commitment_transaction_number); | |
| if self.context.signer_pending_commitment_point { | |
| log_trace!(logger, "Attempting to update holder per-commitment point..."); | |
| self.context.update_holder_per_commitment_point(logger); | |
| } | |
| if self.context.signer_pending_released_secret { | |
| log_trace!(logger, "Attempting to update holder commitment secret..."); | |
| self.context.update_holder_commitment_secret(logger); | |
| } | |
| let channel_reestablish = if self.context.signer_pending_channel_reestablish { | |
| log_trace!(logger, "Attempting to generate channel_reestablish..."); | |
| self.get_channel_reestablish(logger).map(|msg| { | |
| log_trace!(logger, "Generated channel_reestablish; clearing signer_pending_channel_reestablish"); | |
| self.context.signer_pending_channel_reestablish = false; | |
| msg | |
| }) | |
| } else { None }; | |
| if self.context.channel_state.is_peer_disconnected() && channel_reestablish.is_none() { | |
| log_trace!(logger, "Peer is disconnected; no unblocked messages to send."); | |
| return SignerResumeUpdates::default() | |
| } | |
| let funding_signed = if self.context.signer_pending_funding && !self.context.is_outbound() { | |
| log_trace!(logger, "Attempting to generate pending funding signed..."); | |
| self.context.get_funding_signed_msg(logger).1 | |
| } else { None }; | |
| let channel_ready = if funding_signed.is_some() { | |
| self.check_get_channel_ready(0) | |
| // Don't yield up a `channel_ready` message if we're still pending funding. | |
| let channel_ready = if self.context.signer_pending_channel_ready && !self.context.signer_pending_funding { | |
| log_trace!(logger, "Attempting to generate pending channel ready..."); | |
| self.get_channel_ready().map(|msg| { | |
| log_trace!(logger, "Generated channel_ready; clearing signer_pending_channel_ready"); | |
| self.context.signer_pending_channel_ready = false; | |
| msg | |
| }) | |
| } else { None }; | |
| log_trace!(logger, "Signer unblocked with {} commitment_update, {} funding_signed and {} channel_ready", | |
| // If we're pending a channel_ready to the counterparty, then we can't be sending commitment | |
| // updates. If we're not, then make sure that we honor any ordering requirements between the | |
| // commitment update and revoke-and-ack. | |
| let (commitment_update, raa) = if false && self.context.signer_pending_channel_ready { | |
| (None, None) | |
| } else { | |
| match &self.context.resend_order { | |
| RAACommitmentOrder::CommitmentFirst => { | |
| let cu = if self.context.signer_pending_commitment_update { | |
| log_trace!(logger, "Attempting to generate pending commitment update..."); | |
| self.get_last_commitment_update_for_send(logger).map(|cu| { | |
| log_trace!(logger, "Generated commitment update; clearing signer_pending_commitment_update"); | |
| self.context.signer_pending_commitment_update = false; | |
| cu | |
| }).ok() | |
| } else { None }; | |
| let raa = if self.context.signer_pending_revoke_and_ack && !self.context.signer_pending_commitment_update { | |
| log_trace!(logger, "Attempting to generate pending RAA..."); | |
| self.get_last_revoke_and_ack(logger).map(|raa| { | |
| log_trace!(logger, "Generated RAA; clearing signer_pending_revoke_and_ack"); | |
| self.context.signer_pending_revoke_and_ack = false; | |
| raa | |
| }) | |
| } else { None }; | |
| (cu, raa) | |
| } | |
| RAACommitmentOrder::RevokeAndACKFirst => { | |
| let raa = if self.context.signer_pending_revoke_and_ack { | |
| log_trace!(logger, "Attempting to generate pending RAA..."); | |
| self.get_last_revoke_and_ack(logger).map(|raa| { | |
| log_trace!(logger, "Generated RAA; clearing signer_pending_revoke_and_ack"); | |
| self.context.signer_pending_revoke_and_ack = false; | |
| raa | |
| }) | |
| } else { None }; | |
| let cu = if self.context.signer_pending_commitment_update && !self.context.signer_pending_revoke_and_ack { | |
| log_trace!(logger, "Attempting to generate pending commitment update..."); | |
| self.get_last_commitment_update_for_send(logger).map(|cu| { | |
| log_trace!(logger, "Generated commitment update; clearing signer_pending_commitment_update"); | |
| self.context.signer_pending_commitment_update = false; | |
| cu | |
| }).ok() | |
| } else { None }; | |
| (cu, raa) | |
| } | |
| } | |
| }; | |
| let order = self.context.resend_order.clone(); | |
| log_debug!(logger, "Signing unblocked in channel {} at sequence {} resulted in {} channel reestablish, {} commitment update, {} RAA{}, {} funding signed, {} channel ready", | |
| &self.context.channel_id(), self.context.cur_holder_commitment_transaction_number, | |
| if channel_reestablish.is_some() { "a" } else { "no" }, | |
| if commitment_update.is_some() { "a" } else { "no" }, | |
| if raa.is_some() { "an" } else { "no" }, | |
| if commitment_update.is_some() && raa.is_some() { | |
| if order == RAACommitmentOrder::CommitmentFirst { " (commitment first)" } else { " (RAA first)" } | |
| } else { "" }, | |
| if funding_signed.is_some() { "a" } else { "no" }, | |
| if channel_ready.is_some() { "a" } else { "no" }); | |
| SignerResumeUpdates { | |
| commitment_update, | |
| raa, | |
| order, | |
| funding_signed, | |
| channel_ready, | |
| channel_reestablish, | |
| } | |
| } | |
| fn get_last_revoke_and_ack(&self) -> msgs::RevokeAndACK { | |
| let next_per_commitment_point = self.context.holder_signer.as_ref().get_per_commitment_point(self.context.cur_holder_commitment_transaction_number, &self.context.secp_ctx); | |
| let per_commitment_secret = self.context.holder_signer.as_ref().release_commitment_secret(self.context.cur_holder_commitment_transaction_number + 2); | |
| msgs::RevokeAndACK { | |
| channel_id: self.context.channel_id, | |
| per_commitment_secret, | |
| next_per_commitment_point, | |
| #[cfg(taproot)] | |
| next_local_nonce: None, | |
| fn get_last_revoke_and_ack<L: Deref>(&self, logger: &L) -> Option<msgs::RevokeAndACK> where L::Target: Logger { | |
| /// Called when the signer may have become unblocked, to attempt to generate any pending messages. | |
| #[cfg(async_signing)] | |
| pub fn signer_maybe_unblocked<L: Deref>(&mut self, logger: &L) -> SignerResumeUpdates where L::Target: Logger { | |
| // ... existing code ... | |
| } |
| @@ -4398,6 +4750,9 @@ impl<SP: Deref> Channel<SP> where | |||
| return Err(()); | |||
| } | |||
| }; | |||
| log_debug!(logger, "Regenerated latest commitment update in channel {} at {} with{} {} update_adds, {} update_fulfills, {} update_fails, and {} update_fail_malformeds", | |||
There was a problem hiding this comment.
The method get_last_commitment_update_for_send is missing documentation. It's unclear what the purpose of this method is and how it should be used.
+ /// Regenerates the latest commitment update for immediate sending to our peer.
fn get_last_commitment_update_for_send<L: Deref>(&mut self, logger: &L) -> Result<msgs::CommitmentUpdate, ()> where L::Target: Logger {
// ... existing code ...
}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.
| log_debug!(logger, "Regenerated latest commitment update in channel {} at {} with{} {} update_adds, {} update_fulfills, {} update_fails, and {} update_fail_malformeds", | |
| /// Regenerates the latest commitment update for immediate sending to our peer. | |
| fn get_last_commitment_update_for_send<L: Deref>(&mut self, logger: &L) -> Result<msgs::CommitmentUpdate, ()> where L::Target: Logger { | |
| // ... existing code ... | |
| } | |
| log_debug!(logger, "Regenerated latest commitment update in channel {} at {} with{} {} update_adds, {} update_fulfills, {} update_fails, and {} update_fail_malformeds", |
| // 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 TODO comment regarding the verification of the commitment point when an async signer is provided is not addressed. This should be tracked in an issue or resolved before merging.
+ // TODO: Figure out how to verify the commitment point with an async signer.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.
| // 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: Figure out how to verify the commitment point with an async signer. | |
| // 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 channel_ready = self.get_channel_ready(); | ||
| if channel_ready.is_none() { | ||
| log_trace!(logger, "Could not generate channel_ready during channel_reestablish; setting signer_pending_channel_ready"); | ||
| self.context.signer_pending_channel_ready = true; | ||
| } | ||
|
|
||
| return Ok(ReestablishResponses { | ||
| channel_ready: Some(msgs::ChannelReady { | ||
| channel_id: self.context.channel_id(), | ||
| next_per_commitment_point, | ||
| short_channel_id_alias: Some(self.context.outbound_scid_alias), | ||
| }), | ||
| channel_ready, | ||
| raa: None, commitment_update: None, | ||
| order: RAACommitmentOrder::CommitmentFirst, | ||
| shutdown_msg, announcement_sigs, | ||
| }); | ||
| } | ||
|
|
||
| let required_revoke = if msg.next_remote_commitment_number == our_commitment_transaction { | ||
| // If they think we're behind by one state, then we owe them an RAA. We may or may not have that | ||
| // RAA handy depending on the status of the remote signer and the monitor. | ||
| let steps_behind = (INITIAL_COMMITMENT_NUMBER - self.context.cur_holder_commitment_transaction_number) - (msg.next_remote_commitment_number + 1); | ||
| let mut raa = if steps_behind == 0 { | ||
| // Remote isn't waiting on any RevokeAndACK from us! | ||
| // Note that if we need to repeat our ChannelReady we'll do that in the next if block. | ||
| None | ||
| } else if msg.next_remote_commitment_number + 1 == our_commitment_transaction { | ||
| } else if steps_behind == 1 { // msg.next_remote_commitment_number + 1 == (INITIAL_COMMITMENT_NUMBER - 1) - self.context.cur_holder_commitment_transaction_number { | ||
| if self.context.channel_state.is_monitor_update_in_progress() { | ||
| self.context.monitor_pending_revoke_and_ack = true; | ||
| None | ||
| } else { | ||
| Some(self.get_last_revoke_and_ack()) | ||
| self.context.update_holder_commitment_secret(logger); | ||
| self.get_last_revoke_and_ack(logger).map(|raa| { | ||
| if self.context.signer_pending_revoke_and_ack { | ||
| log_trace!(logger, "Generated RAA for channel_reestablish; clearing signer_pending_revoke_and_ack"); | ||
| self.context.signer_pending_revoke_and_ack = false; | ||
| } | ||
| raa | ||
| }).or_else(|| { | ||
| if !self.context.signer_pending_revoke_and_ack { | ||
| log_trace!(logger, "Unable to generate RAA for channel_reestablish; setting signer_pending_revoke_and_ack"); | ||
| self.context.signer_pending_revoke_and_ack = true; | ||
| } | ||
| None | ||
| }) |
There was a problem hiding this comment.
The logic for generating channel_ready during channel_reestablish is not documented. It's unclear why signer_pending_channel_ready is set in this context.
+ // If we cannot generate `channel_ready` during `channel_reestablish`, set `signer_pending_channel_ready`.
let channel_ready = self.get_channel_ready();
if channel_ready.is_none() {
log_trace!(logger, "Could not generate channel_ready during channel_reestablish; setting signer_pending_channel_ready");
self.context.signer_pending_channel_ready = true;
}
// ... similar code for `raa` ...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.
| let channel_ready = self.get_channel_ready(); | |
| if channel_ready.is_none() { | |
| log_trace!(logger, "Could not generate channel_ready during channel_reestablish; setting signer_pending_channel_ready"); | |
| self.context.signer_pending_channel_ready = true; | |
| } | |
| return Ok(ReestablishResponses { | |
| channel_ready: Some(msgs::ChannelReady { | |
| channel_id: self.context.channel_id(), | |
| next_per_commitment_point, | |
| short_channel_id_alias: Some(self.context.outbound_scid_alias), | |
| }), | |
| channel_ready, | |
| raa: None, commitment_update: None, | |
| order: RAACommitmentOrder::CommitmentFirst, | |
| shutdown_msg, announcement_sigs, | |
| }); | |
| } | |
| let required_revoke = if msg.next_remote_commitment_number == our_commitment_transaction { | |
| // If they think we're behind by one state, then we owe them an RAA. We may or may not have that | |
| // RAA handy depending on the status of the remote signer and the monitor. | |
| let steps_behind = (INITIAL_COMMITMENT_NUMBER - self.context.cur_holder_commitment_transaction_number) - (msg.next_remote_commitment_number + 1); | |
| let mut raa = if steps_behind == 0 { | |
| // Remote isn't waiting on any RevokeAndACK from us! | |
| // Note that if we need to repeat our ChannelReady we'll do that in the next if block. | |
| None | |
| } else if msg.next_remote_commitment_number + 1 == our_commitment_transaction { | |
| } else if steps_behind == 1 { // msg.next_remote_commitment_number + 1 == (INITIAL_COMMITMENT_NUMBER - 1) - self.context.cur_holder_commitment_transaction_number { | |
| if self.context.channel_state.is_monitor_update_in_progress() { | |
| self.context.monitor_pending_revoke_and_ack = true; | |
| None | |
| } else { | |
| Some(self.get_last_revoke_and_ack()) | |
| self.context.update_holder_commitment_secret(logger); | |
| self.get_last_revoke_and_ack(logger).map(|raa| { | |
| if self.context.signer_pending_revoke_and_ack { | |
| log_trace!(logger, "Generated RAA for channel_reestablish; clearing signer_pending_revoke_and_ack"); | |
| self.context.signer_pending_revoke_and_ack = false; | |
| } | |
| raa | |
| }).or_else(|| { | |
| if !self.context.signer_pending_revoke_and_ack { | |
| log_trace!(logger, "Unable to generate RAA for channel_reestablish; setting signer_pending_revoke_and_ack"); | |
| self.context.signer_pending_revoke_and_ack = true; | |
| } | |
| None | |
| }) | |
| // If we cannot generate `channel_ready` during `channel_reestablish`, set `signer_pending_channel_ready`. | |
| let channel_ready = self.get_channel_ready(); | |
| if channel_ready.is_none() { | |
| log_trace!(logger, "Could not generate channel_ready during channel_reestablish; setting signer_pending_channel_ready"); | |
| self.context.signer_pending_channel_ready = true; | |
| } | |
| return Ok(ReestablishResponses { | |
| channel_ready, | |
| raa: None, commitment_update: None, | |
| order: RAACommitmentOrder::CommitmentFirst, | |
| shutdown_msg, announcement_sigs, | |
| }); | |
| } | |
| // If they think we're behind by one state, then we owe them an RAA. We may or may not have that | |
| // RAA handy depending on the status of the remote signer and the monitor. | |
| let steps_behind = (INITIAL_COMMITMENT_NUMBER - self.context.cur_holder_commitment_transaction_number) - (msg.next_remote_commitment_number + 1); | |
| let mut raa = if steps_behind == 0 { | |
| // Remote isn't waiting on any RevokeAndACK from us! | |
| // Note that if we need to repeat our ChannelReady we'll do that in the next if block. | |
| None | |
| } else if steps_behind == 1 { // msg.next_remote_commitment_number + 1 == (INITIAL_COMMITMENT_NUMBER - 1) - self.context.cur_holder_commitment_transaction_number { | |
| if self.context.channel_state.is_monitor_update_in_progress() { | |
| self.context.monitor_pending_revoke_and_ack = true; | |
| None | |
| } else { | |
| self.context.update_holder_commitment_secret(logger); | |
| self.get_last_revoke_and_ack(logger).map(|raa| { | |
| if self.context.signer_pending_revoke_and_ack { | |
| log_trace!(logger, "Generated RAA for channel_reestablish; clearing signer_pending_revoke_and_ack"); | |
| self.context.signer_pending_revoke_and_ack = false; | |
| } | |
| raa | |
| }).or_else(|| { | |
| if !self.context.signer_pending_revoke_and_ack { | |
| log_trace!(logger, "Unable to generate RAA for channel_reestablish; setting signer_pending_revoke_and_ack"); | |
| self.context.signer_pending_revoke_and_ack = true; | |
| } | |
| None | |
| }) |
| let mut channel_ready = None; | ||
| if msg.next_local_commitment_number == 1 && INITIAL_COMMITMENT_NUMBER - self.context.cur_holder_commitment_transaction_number == 1 { | ||
| // We should never have to worry about MonitorUpdateInProgress resending ChannelReady | ||
| let next_per_commitment_point = self.context.holder_signer.as_ref().get_per_commitment_point(self.context.cur_holder_commitment_transaction_number, &self.context.secp_ctx); | ||
| Some(msgs::ChannelReady { | ||
| channel_id: self.context.channel_id(), | ||
| next_per_commitment_point, | ||
| short_channel_id_alias: Some(self.context.outbound_scid_alias), | ||
| }) | ||
| } else { None }; | ||
| log_debug!(logger, "Reconnecting channel at state 1, (re?)sending channel_ready"); | ||
| channel_ready = self.get_channel_ready(); | ||
| if channel_ready.is_none() { | ||
| if !self.context.signer_pending_channel_ready { | ||
| log_trace!(logger, "Unable to generate channel_ready for channel_reestablish; setting signer_pending_channel_ready"); | ||
| self.context.signer_pending_channel_ready = true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if msg.next_local_commitment_number == next_counterparty_commitment_number { | ||
| if required_revoke.is_some() { | ||
| if raa.is_some() || self.context.signer_pending_revoke_and_ack { | ||
| log_debug!(logger, "Reconnected channel {} with only lost outbound RAA", &self.context.channel_id()); | ||
| } else { | ||
| log_debug!(logger, "Reconnected channel {} with no loss", &self.context.channel_id()); | ||
| } | ||
|
|
||
| // If we need to provide a channel_ready, but cannot create it yet (e.g. because we do not | ||
| // have the next commitment point), then we must not send a revoke-and-ack: suppress it if | ||
| // necessary. | ||
| if self.context.signer_pending_channel_ready && raa.is_some() { | ||
| log_trace!(logger, "Suppressing RAA because signer_pending_channel_ready; marking signer_pending_revoke_and_ack"); | ||
| self.context.signer_pending_revoke_and_ack = true; | ||
| raa = None; | ||
| } |
There was a problem hiding this comment.
The logic for handling channel_ready and raa during channel_reestablish is repeated here, which suggests potential refactoring to avoid duplication. Additionally, the logic for suppressing messages is not documented.
+ // If we need to provide `channel_ready` but cannot create it yet, suppress `raa` if necessary.
if self.context.signer_pending_channel_ready && raa.is_some() {
log_trace!(logger, "Suppressing RAA because signer_pending_channel_ready; marking signer_pending_revoke_and_ack");
self.context.signer_pending_revoke_and_ack = true;
raa = None;
}
// ... similar code for `commitment_update` ...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.
| let mut channel_ready = None; | |
| if msg.next_local_commitment_number == 1 && INITIAL_COMMITMENT_NUMBER - self.context.cur_holder_commitment_transaction_number == 1 { | |
| // We should never have to worry about MonitorUpdateInProgress resending ChannelReady | |
| let next_per_commitment_point = self.context.holder_signer.as_ref().get_per_commitment_point(self.context.cur_holder_commitment_transaction_number, &self.context.secp_ctx); | |
| Some(msgs::ChannelReady { | |
| channel_id: self.context.channel_id(), | |
| next_per_commitment_point, | |
| short_channel_id_alias: Some(self.context.outbound_scid_alias), | |
| }) | |
| } else { None }; | |
| log_debug!(logger, "Reconnecting channel at state 1, (re?)sending channel_ready"); | |
| channel_ready = self.get_channel_ready(); | |
| if channel_ready.is_none() { | |
| if !self.context.signer_pending_channel_ready { | |
| log_trace!(logger, "Unable to generate channel_ready for channel_reestablish; setting signer_pending_channel_ready"); | |
| self.context.signer_pending_channel_ready = true; | |
| } | |
| } | |
| } | |
| if msg.next_local_commitment_number == next_counterparty_commitment_number { | |
| if required_revoke.is_some() { | |
| if raa.is_some() || self.context.signer_pending_revoke_and_ack { | |
| log_debug!(logger, "Reconnected channel {} with only lost outbound RAA", &self.context.channel_id()); | |
| } else { | |
| log_debug!(logger, "Reconnected channel {} with no loss", &self.context.channel_id()); | |
| } | |
| // If we need to provide a channel_ready, but cannot create it yet (e.g. because we do not | |
| // have the next commitment point), then we must not send a revoke-and-ack: suppress it if | |
| // necessary. | |
| if self.context.signer_pending_channel_ready && raa.is_some() { | |
| log_trace!(logger, "Suppressing RAA because signer_pending_channel_ready; marking signer_pending_revoke_and_ack"); | |
| self.context.signer_pending_revoke_and_ack = true; | |
| raa = None; | |
| } | |
| let mut channel_ready = None; | |
| if msg.next_local_commitment_number == 1 && INITIAL_COMMITMENT_NUMBER - self.context.cur_holder_commitment_transaction_number == 1 { | |
| // We should never have to worry about MonitorUpdateInProgress resending ChannelReady | |
| log_debug!(logger, "Reconnecting channel at state 1, (re?)sending channel_ready"); | |
| channel_ready = self.get_channel_ready(); | |
| if channel_ready.is_none() { | |
| if !self.context.signer_pending_channel_ready { | |
| log_trace!(logger, "Unable to generate channel_ready for channel_reestablish; setting signer_pending_channel_ready"); | |
| self.context.signer_pending_channel_ready = true; | |
| } | |
| } | |
| } | |
| if msg.next_local_commitment_number == next_counterparty_commitment_number { | |
| if raa.is_some() || self.context.signer_pending_revoke_and_ack { | |
| log_debug!(logger, "Reconnected channel {} with only lost outbound RAA", &self.context.channel_id()); | |
| } else { | |
| log_debug!(logger, "Reconnected channel {} with no loss", &self.context.channel_id()); | |
| } | |
| // If we need to provide a channel_ready, but cannot create it yet (e.g. because we do not | |
| // have the next commitment point), then we must not send a revoke-and-ack: suppress it if | |
| // necessary. | |
| // If we need to provide `channel_ready` but cannot create it yet, suppress `raa` if necessary. | |
| if self.context.signer_pending_channel_ready && raa.is_some() { | |
| log_trace!(logger, "Suppressing RAA because signer_pending_channel_ready; marking signer_pending_revoke_and_ack"); | |
| self.context.signer_pending_revoke_and_ack = true; | |
| raa = None; | |
| } |
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.
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.
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.
|
I'm going to close this in favor of #2849. |
Apologies in advance for the hair-ball. Mostly just wanted to get a 50,000 foot overview to see if things were headed in the right direction. If things look okay, I can take a step back and chop this into more modestly-sized PRs.
In general, this PR adds state to the context to allow a
ChannelSignerimplementation to respond asynchronously to theget_per_commitment_point,release_commitment_secret, andsign_counterparty_commitmentmethods, which are the main signer methods that are called during channel setup and normal operation.These changes seem to work as advertised during normal channel operation (creation and payment exchange), and do not obviously fail during channel re-establishment or across node restart. That said, there are a lot more test scenarios to evaluate here.
The details are below.
Adds the RevokeAndAck and RAACommitemnt ordering to the
SignerResumeUpdatesstruct that is returned fromsigner_maybe_unblocked.Adds
signer_maybe_unblockedfor both inbound and outbound unfunded channel states. We need these now becauseget_per_commitment_pointis asynchronous -- and necessary for us to proceed out of the unfunded state into normaloperation. Adds appropriate
SignerResumeUpdatesclasses for both inbound and outbound unfunded states.Maintains
cur_holder_commitment_pointandprev_holder_commitment_secreton the channel context. By making these part of the context state, we can access them at any point we need them without requiring a signer call to regenerate. These are updated appropriately throughout the channel state machine by calling a new context methodupdate_holder_per_commitment.Add several flags to indicate messages that may now be pending the remote signer unblocking:
signer_pending_revoke_and_ack, set when we're waiting to send the revoke-and-ack to the counterparty. This might not just be us waiting on the signer; for example, if the commitment order requires sending the RAA after the commitment update, then even though we might be able to generate the RAA (e.g., because we have the secret), we will not do so while the commitment update is pending (e.g., because we're missing the latest point or do not have a signed counterparty commitment).signer_pending_channel_ready, set when we're waiting to send the channel-ready to the counterparty.signer_pending_commitment_point, set when we're waiting for the signer to return us the commitment point for the current state.signer_pending_released_secret, set when we're waiting for the signer to release the commitment secret for the previous state.This state (current commitment point, previous secret, flags) is persisted in the channel monitor, and restored when the channel is unpickled.
When a monitor update completes, we may still be pending results from the remote signer. If that is the case, we ensure that we correctly maintain the above state flags before the channel state is resumed. For example, if the monitor is pending a commitment signed, but we were not able to retrieve the commitment update from the signer, then we ensure that
signer_pending_commitment_updateis set.When unblocking the signer, we need to ensure that we honor message ordering constraints. For the commitment update and revoke-and-ack, ensure that we can honor the context's current
resend_order. For example, assume we must sendthe RAA before the CU, and could potentially send a CU because we have the commitment point and a signed counterparty commitment transaction. But, we have not yet received the previous state's commitment secret. In this case, we
ensure that no messages are emitted until the commitment secret is released, at which point the signer-unblocked call will emit both messages.
A similar situation exists with the
channel_readymessage and thefunding_signed/funding_createdmessages at channel startup: we make sure that we don't emitchannel_readybefore the funding message.There is at least one unsolved problem here: during channel re-establishment, we need to request an arbitrary commitment point from the signer in order to verify that the counterparty's giving us a legitimate secret. For the time
being, I've simply commented out this check; however, this is not a viable solution. There are a few options to consider here:
We could require that an asynchronous signer cache the previous commitment points, so that any such request must necessarily succeed synchronously.
We could attempt to factor this method as to admit an asynchronous response that would restart the channel re-establish once the commitment point has been provided by the signer and the counterparty's secret can be verified.
The former places some additional burden on a remote signer (albeit minimal) and seems reasonable to me.
As for testing...
For testing asynchronous channel signing, replaces the simple boolean ("everything is on, or everything is off") with flags that let us toggle different signing methods on and off. This lets us (sort of) simulate the signer returning responses in an arbitrary order.
Adds a fully-exploded "send a one-hop payment" test to the async test suite. At each step, simulate the async signer being unavailable, and then unblocking. Vary the order to check all possible orderings of
get_per_commitment_point,release_commitment_secret, andsign_counterparty_commitmentbeing provided by the signer.But there is a lot more to be done here. Many of the odd-ball cases in the PR aren't covered by unit tests and were instead uncovered by running the code in situ with an LND counterparty. So there are a lot more tests to write here.