Allow holder commitment and HTLC signature requests to fail #2816
Conversation
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2816 +/- ##
==========================================
- Coverage 89.13% 89.08% -0.06%
==========================================
Files 115 115
Lines 93544 93617 +73
Branches 93544 93617 +73
==========================================
+ Hits 83380 83395 +15
- Misses 7634 7682 +48
- Partials 2530 2540 +10 ☔ View full report in Codecov by Sentry. |
wpaulino
force-pushed
the
retryable-holder-sigs
branch
from
3b07aca
to
53d53c2
Compare
wpaulino
force-pushed
the
retryable-holder-sigs
branch
from
53d53c2
to
ac4f174
Compare
WalkthroughThe recent updates to the codebase focus on enhancing error handling, introducing more flexible fee rate strategies, and improving the signing process for on-chain transactions. Key changes include modifications to the Changes
Assessment against linked issues
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: 10
Configuration used: CodeRabbit UI
Files selected for processing (7)
- lightning/src/chain/channelmonitor.rs (6 hunks)
- lightning/src/chain/onchaintx.rs (8 hunks)
- lightning/src/chain/package.rs (1 hunks)
- lightning/src/ln/async_signer_tests.rs (11 hunks)
- lightning/src/ln/functional_test_utils.rs (1 hunks)
- lightning/src/util/test_channel_signer.rs (2 hunks)
- lightning/src/util/test_utils.rs (4 hunks)
Additional comments: 27
lightning/src/util/test_channel_signer.rs (1)
- 223-225: The
sign_holder_htlc_transactionmethod now includes a check for signer availability, returning an error if the signer is unavailable. This change aligns with the PR's objective to support asynchronous signing.lightning/src/ln/async_signer_tests.rs (11)
- 45-45: The
set_channel_signer_availablemethod is used to simulate the signer's unavailability. This is a critical part of testing the asynchronous signing feature. Ensure that the rest of the test suite accounts for this new behavior to avoid false negatives in tests.- 59-59: The
set_channel_signer_availablemethod is correctly used to simulate the signer becoming available again, which should trigger the retry mechanism. This is a good test case for the asynchronous signing feature.- 100-100: The
set_channel_signer_availablemethod is used to simulate the signer's unavailability during thefunding_createdhandling. This is consistent with the PR's objectives and should be reflected in all relevant tests.- 113-113: The
set_channel_signer_availablemethod is used to simulate the signer becoming available again, which should trigger the retry mechanism for thefunding_signedmessage. This is a good test case for the asynchronous signing feature.- 154-154: The
set_channel_signer_availablemethod is used to simulate the signer's unavailability during thecommitment_signedhandling. This is consistent with the PR's objectives and should be reflected in all relevant tests.- 161-161: The
set_channel_signer_availablemethod is used to simulate the signer becoming available again, which should trigger the retry mechanism for thecommitment_signedmessage. This is a good test case for the asynchronous signing feature.- 217-217: The
set_channel_signer_availablemethod is used to simulate the signer's unavailability during thefunding_createdhandling for a zero-conf channel. This is consistent with the PR's objectives and should be reflected in all relevant tests.- 232-232: The
set_channel_signer_availablemethod is used to simulate the signer becoming available again, which should trigger the retry mechanism for thefunding_signedmessage in a zero-conf channel. This is a good test case for the asynchronous signing feature.- 301-301: The
set_channel_signer_availablemethod is used to simulate the signer's unavailability during thecommitment_signedhandling after a peer disconnect. This is consistent with the PR's objectives and should be reflected in all relevant tests.- 316-316: The
set_channel_signer_availablemethod is used to simulate the signer becoming available again after a peer reconnect, which should trigger the retry mechanism for thecommitment_signedmessage. This is a good test case for the asynchronous signing feature.- 330-433: The new test function
do_test_async_holder_signaturesis added to test the asynchronous signing of holder signatures. It simulates the signer's unavailability and subsequent availability, ensuring that the system can handle asynchronous operations. This is a comprehensive test that covers the new functionality introduced in the PR.lightning/src/chain/onchaintx.rs (3)
- 643-644: The debug logging for the pre-signed commitment transaction is correct and provides useful information.
- 1126-1129: The method
get_maybe_signed_holder_txattempts to sign the holder commitment transaction and falls back to returning an unsigned transaction if signing fails. This change aligns with the PR's objective to handle cases where signatures are not immediately available.- 1163-1173: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1138-1170]
The method
get_maybe_signed_htlc_txhas been updated to handle potentially unsigned HTLC transactions. It correctly attempts to sign the transaction and adds the necessary witness data if successful. If signing fails, it does not add any witness data, leaving the transaction unsigned.lightning/src/util/test_utils.rs (8)
- 1127-1127: The addition of the
unavailable_signersfield toTestKeysInterfaceis intended to simulate the behavior of unavailable signers in tests.- 1186-1190: The logic within
derive_channel_signerchecks if thechannel_keys_idis in theunavailable_signersset and sets the signer's availability accordingly. This is a good use of the new field to control the signer's behavior in tests.- 1242-1242: The
derive_channel_keysmethod is a wrapper aroundderive_channel_signerthat simplifies its usage by not requiring the caller to pass a reference to thechannel_keys_id. This is a convenience method that maintains the existing functionality.- 1242-1242: The
derive_channel_keysmethod is a thin wrapper that does not add any new logic. It's a simple pass-through toderive_channel_signer. This is acceptable as it's meant to simplify the interface for the caller.- 1228-1228: The
unavailable_signersfield is initialized as an emptyHashSet, which is appropriate for its intended use in testing scenarios where signers may be unavailable.- 1127-1127: The
unavailable_signersfield is aMutex<HashSet<[u8; 32]>>, which is thread-safe. This is appropriate since theTestKeysInterfacemay be used in a multi-threaded context during testing.- 1186-1190: The check for signer availability is done by looking up the
channel_keys_idin theunavailable_signersset. This is a simple and effective way to simulate signer unavailability.- 1242-1242: The
derive_channel_keysmethod provides a convenient way to derive channel keys without having to specify thechannel_keys_idexplicitly. This is useful for tests that do not need to control thechannel_keys_id.lightning/src/chain/channelmonitor.rs (4)
- 1399-1400: The trait bounds for
FandLin theload_outputs_to_watchfunction have been refined. This change is appropriate and aligns with Rust's idiomatic use of trait bounds for generic parameters.- 1787-1792: The
do_signer_callfunction is a test utility that allows for the invocation of a closure with the signer as a parameter. This change seems to be in line with the PR's objectives to facilitate testing of asynchronous signing functionality.- 3495-3506: The logic for adding HTLC transactions to the holder transactions list has been updated to check for non-empty witness data. This ensures that only signed transactions are included. The change is consistent with the PR's goal of handling unsigned transactions properly. However, the same recommendation about using a more descriptive error type applies here as well.
- 3531-3536: This hunk is a duplicate of the logic reviewed in hunk 5. It appears to be part of the same function and is likely included due to the way the diff was generated. Since the same logic has been reviewed and approved in hunk 5, no additional comments are necessary for this hunk.
wpaulino
force-pushed
the
retryable-holder-sigs
branch
from
ac4f174
to
8945f62
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (7)
- lightning/src/chain/channelmonitor.rs (5 hunks)
- lightning/src/chain/onchaintx.rs (8 hunks)
- lightning/src/chain/package.rs (1 hunks)
- lightning/src/ln/async_signer_tests.rs (11 hunks)
- lightning/src/ln/functional_test_utils.rs (1 hunks)
- lightning/src/util/test_channel_signer.rs (2 hunks)
- lightning/src/util/test_utils.rs (4 hunks)
Files skipped from review as they are similar to previous changes (6)
- lightning/src/chain/onchaintx.rs
- lightning/src/chain/package.rs
- lightning/src/ln/async_signer_tests.rs
- lightning/src/ln/functional_test_utils.rs
- lightning/src/util/test_channel_signer.rs
- lightning/src/util/test_utils.rs
Additional comments: 4
lightning/src/chain/channelmonitor.rs (4)
- 1399-1400: The trait bounds for
FandLin theload_outputs_to_watchfunction have been refined. This change is straightforward and does not appear to introduce any issues.- 1545-1561: The
get_latest_holder_commitment_txnfunction has been renamed tobroadcast_latest_holder_commitment_txnand now takes additional parameters forbroadcasterandfee_estimator. This change aligns with the PR's objective to enable asynchronous signer functionality. Ensure that all calls to the old function name have been updated to the new name and signature.- 1794-1799: The
do_signer_callfunction is a test utility that allows for the invocation of a closure with theSigner. This change seems to be a utility for testing and does not affect production code.- 3493-3498: The logic within the
if let Some(htlc_tx)block checks if the HTLC transaction is signed before pushing it toholder_transactions. This is a critical change that supports the PR's goal of handling transactions that may be unsigned. Ensure that the logic correctly handles the case where the transaction is not yet signed.Verification successful
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the usage of `get_maybe_signed_htlc_tx` to ensure it's being handled correctly. rg --type rust 'get_maybe_signed_htlc_tx'Length of output: 431
Script:
#!/bin/bash # Examine the implementation of `get_maybe_signed_htlc_tx` in `onchaintx.rs`. rg --type rust --context 10 'fn get_maybe_signed_htlc_tx' lightning/src/chain/onchaintx.rs # Confirm that the check for a non-empty witness is used elsewhere in the codebase as an indicator of a signed transaction. rg --type rust 'input.witness.is_empty()' lightning/Length of output: 1865
Script:
#!/bin/bash # Retrieve the complete implementation of `get_maybe_signed_htlc_tx` from `onchaintx.rs`. rg --type rust --context 30 'fn get_maybe_signed_htlc_tx' lightning/src/chain/onchaintx.rsLength of output: 3053
lightning/src/chain/onchaintx.rs
Outdated
| log_debug!(logger, "Pre-signed {} already has feerate {} sat/kW above required {} sat/kW", | ||
| log_tx!(tx), commitment_tx_feerate_sat_per_1000_weight, | ||
| log_debug!(logger, "Pre-signed commitment {} already has feerate {} sat/kW above required {} sat/kW", | ||
| tx.txid(), commitment_tx_feerate_sat_per_1000_weight, |
There was a problem hiding this comment.
log_tx wants input witnesses and we may not have any if the transaction wasn't signed.
lightning/src/chain/onchaintx.rs
Outdated
| let log_start = if bumped_feerate { "Broadcasting RBF-bumped" } else { "Rebroadcasting" }; | ||
| log_info!(logger, "{} onchain {}", log_start, log_tx!(tx)); | ||
| broadcaster.broadcast_transactions(&[&tx]); | ||
| if tx.input.iter().any(|input| input.witness.is_empty()) { |
There was a problem hiding this comment.
What happens if our fee estimator returns a very slightly different fee every time we call it? If fees are slowly rising, and the estimator is some smoothed thing, we may try to broadcast a new transaction each time with 0.1sat/vB additional fee, which will cause the signer to never succeed, correct?
There was a problem hiding this comment.
I don't see how that causes the signer to never succeed. Whenever rebroadcast_pending_claims is called, we'll only try re-signing the most recently used fee.
There was a problem hiding this comment.
rebroadcast_pending_claims just calls generate_claim for all the relevant requests, which, for is_malleable packages calls compute_package_feerate, which will return the estimator feerate if its greater than the previous package feerate. In such a case, we'll compute_package_output and finalize_malleable_package on a different-feerate tx than previously.
There was a problem hiding this comment.
Discussed this offline and it's been addressed by adding a new ChannelMonitor::signer_unblocked method that will regenerate all claim requests using their most recently used feerate. rebroadcast_pending_claims remains unchanged.
wpaulino
force-pushed
the
retryable-holder-sigs
branch
from
8945f62
to
7340531
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (8)
- lightning/src/chain/channelmonitor.rs (6 hunks)
- lightning/src/chain/onchaintx.rs (8 hunks)
- lightning/src/chain/package.rs (1 hunks)
- lightning/src/ln/async_signer_tests.rs (11 hunks)
- lightning/src/ln/functional_test_utils.rs (1 hunks)
- lightning/src/ln/monitor_tests.rs (2 hunks)
- lightning/src/util/test_channel_signer.rs (2 hunks)
- lightning/src/util/test_utils.rs (4 hunks)
Files skipped from review as they are similar to previous changes (6)
- lightning/src/chain/onchaintx.rs
- lightning/src/chain/package.rs
- lightning/src/ln/async_signer_tests.rs
- lightning/src/ln/functional_test_utils.rs
- lightning/src/util/test_channel_signer.rs
- lightning/src/util/test_utils.rs
Additional comments: 5
lightning/src/chain/channelmonitor.rs (5)
- 1399-1400: The refinement of trait bounds for the
FandLparameters in theload_outputs_to_watchfunction is a good practice for ensuring that the function can only be used with appropriate types. This change enhances type safety and readability.- 1545-1561: The introduction of the
broadcast_latest_holder_commitment_txnmethod with additional parameters forbroadcasterandfee_estimatoris a significant change. It aligns with the PR's objective to enable asynchronous signer functionality. However, the method's documentation should emphasize the conditions under which it is safe to use this function, given its potential risks. Additionally, the use ofLowerBoundedFeeEstimatorandWithChannelMonitorwithin this method suggests a thoughtful approach to fee estimation and logging, enhancing the method's robustness.- 1794-1799: The
do_signer_callmethod, marked for test configurations, provides a flexible way to interact with theSignerfor testing purposes. This utility function is a valuable addition for testing the asynchronous signing functionality, ensuring that the signer can be tested under various conditions without exposing this functionality in the production code.- 2846-2846: The logging within this conditional block serves as a critical warning to operators about the potential risks of broadcasting a holder commitment transaction. This is an essential safety measure, but it also underscores the importance of clear documentation and operator education on handling such scenarios safely.
- 3493-3498: The logic for handling HTLC transactions in this segment is crucial for the asynchronous signing feature. It ensures that only HTLC transactions with non-empty witness data are considered for broadcasting. This check is a necessary part of the transaction validation process, preventing the broadcasting of incomplete transactions. However, it's important to ensure that this logic is thoroughly tested, especially since it directly impacts the safety and reliability of transaction broadcasting.
wpaulino
force-pushed
the
retryable-holder-sigs
branch
from
7340531
to
7496923
Compare
|
Oh, also we need to tweak the BP on mobile so that we call the rebroadcast faster on startup. If we're on mobile we're usually not even running for more than a second or two, so we can't rely on the signer to have signatures on rebroadcast in 30 seconds. We should probably do an exponential rebroadcast starting with 500ms and growing to 30s on startup. |
|
It's already called once on startup, that should suffice? |
wpaulino
force-pushed
the
retryable-holder-sigs
branch
from
7496923
to
6b16a32
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (10)
- lightning/src/chain/channelmonitor.rs (8 hunks)
- lightning/src/chain/onchaintx.rs (15 hunks)
- lightning/src/chain/package.rs (6 hunks)
- lightning/src/ln/async_signer_tests.rs (11 hunks)
- lightning/src/ln/channelmanager.rs (1 hunks)
- lightning/src/ln/functional_test_utils.rs (1 hunks)
- lightning/src/ln/monitor_tests.rs (2 hunks)
- lightning/src/sign/ecdsa.rs (3 hunks)
- lightning/src/util/test_channel_signer.rs (2 hunks)
- lightning/src/util/test_utils.rs (4 hunks)
Files skipped from review due to trivial changes (1)
- lightning/src/ln/functional_test_utils.rs
Files skipped from review as they are similar to previous changes (5)
- lightning/src/chain/onchaintx.rs
- lightning/src/ln/async_signer_tests.rs
- lightning/src/ln/monitor_tests.rs
- lightning/src/util/test_channel_signer.rs
- lightning/src/util/test_utils.rs
Additional comments: 15
lightning/src/sign/ecdsa.rs (3)
- 55-60: The addition of error handling and retry logic in
sign_holder_commitmentis a significant improvement. It allows for asynchronous operation by enabling the signer to indicate unavailability and defer signing until later. This change aligns with the PR's objective to enhance the LDK with asynchronous signer functionality. However, ensure that all downstream consumers of this method properly handle the new error case to prevent unhandled errors.- 118-125: Similar to the
sign_holder_commitmentmethod, the addition of error handling insign_holder_htlc_transactionis crucial for supporting asynchronous signing operations. This change allows the method to signal when the signer is unavailable, requiring a retry later. It's important to verify that the retry mechanism is implemented wherever this method is called to handle the deferred signing process effectively.- 158-163: The changes to
sign_holder_anchor_inputto include error handling for signer unavailability are consistent with the modifications made to other signing methods in this trait. This consistency is beneficial for the overall design, making the asynchronous signing feature more robust. As with the other methods, it's critical to ensure that the caller implements retry logic correctly to manage the asynchronous nature of the signing process.lightning/src/chain/package.rs (4)
- 643-643: The method
get_maybe_signed_holder_txis expected to return anOption<Transaction>, but the return statement is missing theSomewrapper. This issue was previously flagged and remains valid.- 966-966: The method
compute_package_outputnow includes afeerate_strategyparameter. Ensure that all calls to this method throughout the codebase have been updated to include this new parameter.- 977-977: The use of
feerate_bumpwithincompute_package_outputintroduces a new logic for fee estimation. Verify that this logic aligns with the intended behavior and that thefeerate_strategyis correctly applied.Verification successful
The provided script output reveals the implementation details of
compute_package_outputwithinpackage.rs. This function calculates the output value and the new feerate for a transaction package based on the providedpredicted_weight,dust_limit_sats, andfeerate_strategy. The logic includes a conditional check onself.feerate_previousto determine whether to use a new fee estimation logic (feerate_bump) or a standard fee calculation (compute_fee_from_spent_amounts).
When
self.feerate_previousis not 0: It attempts to bump the feerate using thefeerate_bumpfunction, which takes into account thepredicted_weight,input_amounts,self.feerate_previous,feerate_strategy,fee_estimator, andlogger. If successful, it returns the adjusted output value and the new feerate.When
self.feerate_previousis 0: It falls back to a standard fee calculation method,compute_fee_from_spent_amounts, to determine the new fee and feerate.This implementation suggests that the
feerate_strategyis indeed considered in the fee estimation process, specifically in the context of fee bumping whenself.feerate_previousis not zero. The logic appears to align with the intended behavior of dynamically adjusting the fee based on the current feerate, the previous feerate, and the specified strategy for fee estimation.Given this analysis, the use of
feerate_bumpwithincompute_package_outputaligns with the intended behavior of adjusting the transaction fee based on a strategy, and thefeerate_strategyis correctly applied as part of this process.* 993-993: The addition of the `feerate_strategy` parameter in `compute_package_feerate` method is significant. Ensure that all invocations of this method have been updated accordingly.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Manual verification needed to ensure the new fee estimation logic in `compute_package_output` aligns with intended behavior. echo "Review the use of feerate_bump within compute_package_output for correctness and alignment with intended behavior."Length of output: 238
Script:
#!/bin/bash # Search for the definition and usage of compute_package_output to understand the application of feerate_bump and feerate_strategy rg "compute_package_output" -A 30Length of output: 5089
lightning/src/chain/channelmonitor.rs (7)
- 47-47: The imports have been updated to include new traits and structures necessary for the changes in this file. This is a straightforward change and appears correct as it aligns with the PR's objectives to support asynchronous signer functionality.
- 1399-1400: The
load_outputs_to_watchmethod now specifies refined trait bounds for its parameters, ensuring that thefilterargument must implement thechain::Filtertrait and theloggermust implement theLoggertrait. This change improves type safety and clarity in the method's contract.- 1545-1561: The
broadcast_latest_holder_commitment_txnmethod has been introduced, replacing theget_latest_holder_commitment_txnmethod. This change is significant as it directly addresses the PR's objective by enabling the rebroadcasting of transactions when necessary. The method now takes additional parameters for broadcasting and fee estimation, which are essential for handling transactions asynchronously. Ensure that all calls to the old method have been updated to use this new method.Verification successful
The verification process confirms that the
broadcast_latest_holder_commitment_txnmethod has been introduced and is being used as intended, replacing theget_latest_holder_commitment_txnmethod for the main operational purposes. The presence ofunsafe_get_latest_holder_commitment_txnin the codebase is consistent with a common practice for retaining more sensitive or controlled versions of methods for specific use cases, such as testing or internal operations, which does not conflict with the intent of the original review comment. Therefore, the transition appears to have been handled appropriately.* 1748-1767: The `signer_unblocked` method is introduced to trigger rebroadcasts of pending claims after a transaction signature generation failure. This method is part of the enhancements to support asynchronous operations within the signer. It's crucial to ensure that this method is called appropriately in scenarios where the signer becomes available after being previously unavailable. The use of `LowerBoundedFeeEstimator` and `WithChannelMonitor` within this method is consistent with the approach taken in other parts of the PR. * 1815-1818: The `do_signer_call` method is a test utility that allows for direct invocation of signer-related operations. While this method is scoped under `#[cfg(test)]`, indicating it's only available for testing purposes, it's essential to ensure that it's used correctly in tests to simulate asynchronous signing behavior accurately. * 2865-2865: The logging statements have been updated to warn about the availability of a potentially-unsafe holder commitment transaction. This change is part of the broader effort to improve error handling and user guidance when dealing with asynchronous signing scenarios. It's important that these logs are clear and provide actionable advice to the node operator. * 3512-3517: The logic for handling HTLC transactions has been updated to include a check for whether the transaction's inputs have witnesses, indicating that the transaction is fully signed. This change is a direct consequence of the move towards asynchronous signing, where transactions may not be immediately signed. It's crucial to ensure that only fully signed transactions are considered for inclusion in `holder_transactions`.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for old method usage to ensure it's fully replaced. rg --type rust 'get_latest_holder_commitment_txn' # Confirm new method is used correctly everywhere. rg --type rust 'broadcast_latest_holder_commitment_txn'Length of output: 1324
lightning/src/ln/channelmanager.rs (1)
- 3000-3001: The documentation update for
force_close_without_broadcasting_txncorrectly reflects the shift towards usingChannelMonitor::broadcast_latest_holder_commitment_txnfor broadcasting the latest local transaction(s). This change aligns with the PR's objectives to enhance asynchronous operations within the LDK. Ensure that this method (broadcast_latest_holder_commitment_txn) is indeed the recommended approach and is implemented as described.
| @@ -637,10 +637,10 @@ impl PackageSolvingData { | |||
| match self { | |||
| PackageSolvingData::HolderHTLCOutput(ref outp) => { | |||
| debug_assert!(!outp.channel_type_features.supports_anchors_zero_fee_htlc_tx()); | |||
| return onchain_handler.get_fully_signed_htlc_tx(outpoint, &outp.preimage); | |||
| onchain_handler.get_maybe_signed_htlc_tx(outpoint, &outp.preimage) | |||
There was a problem hiding this comment.
The method get_maybe_signed_htlc_tx is called without handling its return type correctly. It should handle the Option<Transaction> return type properly.
- onchain_handler.get_maybe_signed_htlc_tx(outpoint, &outp.preimage)
+ return onchain_handler.get_maybe_signed_htlc_tx(outpoint, &outp.preimage);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.
| onchain_handler.get_maybe_signed_htlc_tx(outpoint, &outp.preimage) | |
| return onchain_handler.get_maybe_signed_htlc_tx(outpoint, &outp.preimage) |
wpaulino
force-pushed
the
retryable-holder-sigs
branch
from
6b16a32
to
18c97b1
Compare
wpaulino
force-pushed
the
retryable-holder-sigs
branch
from
18c97b1
to
4dea907
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (11)
- lightning/src/chain/channelmonitor.rs (8 hunks)
- lightning/src/chain/onchaintx.rs (15 hunks)
- lightning/src/chain/package.rs (6 hunks)
- lightning/src/ln/async_signer_tests.rs (2 hunks)
- lightning/src/ln/channel.rs (1 hunks)
- lightning/src/ln/channelmanager.rs (1 hunks)
- lightning/src/ln/functional_test_utils.rs (1 hunks)
- lightning/src/ln/monitor_tests.rs (2 hunks)
- lightning/src/sign/ecdsa.rs (3 hunks)
- lightning/src/util/test_channel_signer.rs (2 hunks)
- lightning/src/util/test_utils.rs (4 hunks)
Files skipped from review as they are similar to previous changes (9)
- lightning/src/chain/onchaintx.rs
- lightning/src/chain/package.rs
- lightning/src/ln/async_signer_tests.rs
- lightning/src/ln/channelmanager.rs
- lightning/src/ln/functional_test_utils.rs
- lightning/src/ln/monitor_tests.rs
- lightning/src/sign/ecdsa.rs
- lightning/src/util/test_channel_signer.rs
- lightning/src/util/test_utils.rs
Additional comments: 7
lightning/src/chain/channelmonitor.rs (6)
- 47-47: The imports are correctly organized, ensuring that all necessary traits and structures for the
ChannelMonitorand related functionality are available. This change aligns with the PR's objective to enhance asynchronous signer functionality.- 1419-1420: Refining trait bounds for the
FandLparameters in theload_outputs_to_watchfunction improves type safety and clarity. This change ensures that only appropriate types implementing thechain::FilterandLoggertraits can be passed to the function, enhancing maintainability and readability.- 1768-1787: The
signer_unblockedmethod, which triggers rebroadcasts of pending claims after a transaction signature generation failure, is a crucial addition for handling asynchronous signing scenarios. This method's implementation, including the use ofLowerBoundedFeeEstimatorandWithChannelMonitor, is consistent with the PR's objectives. However, ensure that the logic for handling rebroadcasts is thoroughly tested, especially given the complexity of asynchronous operations.Verification successful
The expanded search for the
signer_unblockedmethod across the entire repository has revealed multiple references to this method in the codebase, including inlightning/src/ln/async_signer_tests.rs. This file contains tests that specifically invoke thesigner_unblockedmethod in various scenarios, indicating that there is indeed test coverage for this method. These findings confirm that the initial review comment's concern about ensuring thorough testing for thesigner_unblockedmethod has been addressed in the codebase.* 1834-1838: The `do_signer_call` method in the test utilities is a useful addition for simulating asynchronous signing behavior. By allowing a closure to be executed with the `Signer`, it facilitates more flexible testing scenarios. Ensure that this method is used effectively in tests to cover the new asynchronous functionality introduced by the PR. * 2885-2885: Logging a warning when a potentially-unsafe holder commitment transaction is available to broadcast is a good practice. It alerts the operator to the risks involved and suggests consulting the documentation for manual action. This change enhances the robustness of error handling and user awareness. * 3536-3541: The logic for handling HTLC transactions in `unsafe_get_latest_holder_commitment_txn` has been updated to include a check for non-empty witness data before adding a transaction to the `holder_transactions` list. This change is crucial for ensuring that only properly signed transactions are considered, aligning with the PR's goal to handle transactions asynchronously and safely.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that tests covering the signer_unblocked method exist. rg --type rust 'signer_unblocked' tests/Length of output: 87
Script:
#!/bin/bash # Broaden the search for tests covering the signer_unblocked method across the entire repository. rg --type rust 'signer_unblocked'Length of output: 1916
lightning/src/ln/channel.rs (1)
- 1261-1264: The conditional compilation directives are correctly applied to toggle the visibility of
channel_keys_idbased on the compilation context. This is a common pattern to facilitate testing. However, it would be beneficial to add a comment explaining why this field's visibility is altered for tests, to provide context for future maintainers.
| @@ -637,10 +637,10 @@ impl PackageSolvingData { | |||
| match self { | |||
There was a problem hiding this comment.
Should this be renamed get_maybe_finalized_tx, then? (and finalize_untractable_package).
There was a problem hiding this comment.
Yeah, while doing this I also realized we were missing handling of signatures from non-external malleable requests (sign_counterparty_htlc_transaction, sign_justice_revoked_*) as well. Added with test coverage included.
wpaulino
force-pushed
the
retryable-holder-sigs
branch
from
4dea907
to
eb3c4ab
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (12)
- lightning/src/chain/chainmonitor.rs (1 hunks)
- lightning/src/chain/channelmonitor.rs (8 hunks)
- lightning/src/chain/onchaintx.rs (16 hunks)
- lightning/src/chain/package.rs (8 hunks)
- lightning/src/ln/async_signer_tests.rs (2 hunks)
- lightning/src/ln/channel.rs (1 hunks)
- lightning/src/ln/channelmanager.rs (1 hunks)
- lightning/src/ln/functional_test_utils.rs (1 hunks)
- lightning/src/ln/monitor_tests.rs (2 hunks)
- lightning/src/sign/ecdsa.rs (3 hunks)
- lightning/src/util/test_channel_signer.rs (4 hunks)
- lightning/src/util/test_utils.rs (4 hunks)
Files skipped from review as they are similar to previous changes (10)
- lightning/src/chain/onchaintx.rs
- lightning/src/chain/package.rs
- lightning/src/ln/async_signer_tests.rs
- lightning/src/ln/channel.rs
- lightning/src/ln/channelmanager.rs
- lightning/src/ln/functional_test_utils.rs
- lightning/src/ln/monitor_tests.rs
- lightning/src/sign/ecdsa.rs
- lightning/src/util/test_channel_signer.rs
- lightning/src/util/test_utils.rs
Additional comments: 6
lightning/src/chain/chainmonitor.rs (1)
- 639-659: The implementation of
signer_unblockedcorrectly handles the optionalOutPointto selectively trigger rebroadcasts for a specific channel monitor or all monitors if none is specified. This approach aligns with the intended functionality described in the PR objectives. The use of locking mechanisms (self.monitors.read().unwrap()) to access shared state is consistent with the rest of the file, ensuring thread safety. The function iterates over the relevant monitors and callssigner_unblockedon each, which is expected to handle the rebroadcast logic internally. Overall, the changes are logically sound and adhere to the project's coding standards.lightning/src/chain/channelmonitor.rs (5)
- 47-47: Imports are added for several types. Ensure these new imports are used within the file to avoid unnecessary dependencies.
- 1419-1420: The
load_outputs_to_watchmethod now specifies refined trait bounds for its parameters. Verify that these changes do not restrict the method's usability in existing contexts.- 1768-1787: The
signer_unblockedmethod triggers rebroadcasts of pending claims with a specificFeerateStrategy. Ensure that the chosen strategy aligns with the intended behavior and that the method's implementation correctly handles all edge cases.- 1835-1838: The
do_signer_callmethod is introduced for testing purposes. Confirm that its usage is restricted to test scenarios and does not expose any sensitive operations in production code.- 3536-3541: The logic for handling HTLC transactions in
unsafe_get_latest_holder_commitment_txnis adjusted. Verify that these changes correctly handle all cases, especially around the signing and inclusion of HTLC transactions.
| /// You may use this to broadcast the latest local commitment transaction, either because | ||
| /// a monitor update failed or because we've fallen behind (i.e. we've received proof that our | ||
| /// counterparty side knows a revocation secret we gave them that they shouldn't know). | ||
| /// | ||
| /// Broadcasting these transactions in the second case is UNSAFE, as they allow counterparty | ||
| /// Broadcasting these transactions in this manner is UNSAFE, as they allow counterparty | ||
| /// side to punish you. Nevertheless you may want to broadcast them if counterparty doesn't | ||
| /// close channel with their commitment transaction after a substantial amount of time. Best | ||
| /// may be to contact the other node operator out-of-band to coordinate other options available | ||
| /// to you. | ||
| /// | ||
| /// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager | ||
| pub fn get_latest_holder_commitment_txn<L: Deref>(&self, logger: &L) -> Vec<Transaction> | ||
| where L::Target: Logger { | ||
| pub fn broadcast_latest_holder_commitment_txn<B: Deref, F: Deref, L: Deref>( | ||
| &self, broadcaster: &B, fee_estimator: &F, logger: &L | ||
| ) | ||
| where | ||
| B::Target: BroadcasterInterface, | ||
| F::Target: FeeEstimator, | ||
| L::Target: Logger | ||
| { |
There was a problem hiding this comment.
The broadcast_latest_holder_commitment_txn method is introduced with detailed documentation on its unsafe nature and intended use. Ensure that all calls to this method are carefully reviewed to prevent misuse, given its potential to cause loss of funds if used incorrectly.
| @@ -2855,7 +2882,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> { | |||
| } else if !self.holder_tx_signed { | |||
| log_error!(logger, "WARNING: You have a potentially-unsafe holder commitment transaction available to broadcast"); | |||
| log_error!(logger, " in channel monitor for channel {}!", &self.channel_id()); | |||
| log_error!(logger, " Read the docs for ChannelMonitor::get_latest_holder_commitment_txn and take manual action!"); | |||
| log_error!(logger, " Read the docs for ChannelMonitor::broadcast_latest_holder_commitment_txn to take manual action!"); | |||
There was a problem hiding this comment.
Logging statements are added to warn about the availability of a potentially-unsafe holder commitment transaction. Ensure that these logs provide clear guidance to operators on how to proceed safely.
wpaulino
force-pushed
the
retryable-holder-sigs
branch
from
eb3c4ab
to
2ba1858
Compare
lightning/src/chain/onchaintx.rs
Outdated
| ) { | ||
| assert!(new_feerate != 0); | ||
|
|
||
| let transaction = cached_request.finalize_malleable_package( | ||
| let transaction = cached_request.maybe_finalize_malleable_package( | ||
| cur_height, self, output_value, self.destination_script.clone(), logger | ||
| ).unwrap(); | ||
| log_trace!(logger, "...with timer {} and feerate {}", new_timer, new_feerate); |
There was a problem hiding this comment.
What is this ...ing from? Is it supposed to be a followon to the now-removed Finalized transaction...ready to broadcast logs?
There was a problem hiding this comment.
+1, please add comment. Also, if we ever add a log statement between this one and the one it is ostensibly continuing from, it will become confusing
There was a problem hiding this comment.
I'm just going to remove it.
| } | ||
| log_debug!(logger, "Finalized transaction {} ready to broadcast", bumped_tx.txid()); |
There was a problem hiding this comment.
I mean, I'm fine removing this, but its not clear to me we have quite the same logs printing the output of what we're doing in generate_claim describing what we've done (though it'll show up when we broadcast eventually, presumably).
There was a problem hiding this comment.
Or you could leave the log statement if the finalization succeeded.
There was a problem hiding this comment.
We already have logs whenever we process the result of generate_claim so I opted to remove this one as it's not very useful anymore.
lightning/src/chain/onchaintx.rs
Outdated
| ) { | ||
| assert!(new_feerate != 0); | ||
|
|
||
| let transaction = cached_request.finalize_malleable_package( | ||
| let transaction = cached_request.maybe_finalize_malleable_package( | ||
| cur_height, self, output_value, self.destination_script.clone(), logger | ||
| ).unwrap(); | ||
| log_trace!(logger, "...with timer {} and feerate {}", new_timer, new_feerate); |
There was a problem hiding this comment.
+1, please add comment. Also, if we ever add a log statement between this one and the one it is ostensibly continuing from, it will become confusing
| // so we choose to bump our previous feerate by 25%, making sure we don't use a | ||
| // lower feerate or overpay by a large margin by limiting it to 5x the new fee | ||
| // estimate. | ||
| let previous_feerate = self.feerate_previous.try_into().unwrap_or(u32::max_value()); |
There was a problem hiding this comment.
can there ever be a scenario where previous_feerate does become u32::max_value()? Because that could potentially be rather catastrophic.
There was a problem hiding this comment.
Possibly if we keep bumping 25% on each block connected, but it's very unlikely to happen.
| } | ||
| log_debug!(logger, "Finalized transaction {} ready to broadcast", bumped_tx.txid()); |
There was a problem hiding this comment.
Or you could leave the log statement if the finalization succeeded.
wpaulino
force-pushed
the
retryable-holder-sigs
branch
from
2ba1858
to
f35adc7
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (12)
- lightning/src/chain/chainmonitor.rs (1 hunks)
- lightning/src/chain/channelmonitor.rs (7 hunks)
- lightning/src/chain/onchaintx.rs (16 hunks)
- lightning/src/chain/package.rs (8 hunks)
- lightning/src/ln/async_signer_tests.rs (2 hunks)
- lightning/src/ln/channel.rs (1 hunks)
- lightning/src/ln/channelmanager.rs (1 hunks)
- lightning/src/ln/functional_test_utils.rs (1 hunks)
- lightning/src/ln/monitor_tests.rs (2 hunks)
- lightning/src/sign/ecdsa.rs (6 hunks)
- lightning/src/util/test_channel_signer.rs (4 hunks)
- lightning/src/util/test_utils.rs (4 hunks)
Files skipped from review as they are similar to previous changes (11)
- lightning/src/chain/chainmonitor.rs
- lightning/src/chain/onchaintx.rs
- lightning/src/chain/package.rs
- lightning/src/ln/async_signer_tests.rs
- lightning/src/ln/channel.rs
- lightning/src/ln/channelmanager.rs
- lightning/src/ln/functional_test_utils.rs
- lightning/src/ln/monitor_tests.rs
- lightning/src/sign/ecdsa.rs
- lightning/src/util/test_channel_signer.rs
- lightning/src/util/test_utils.rs
Additional comments: 6
lightning/src/chain/channelmonitor.rs (6)
- 47-47: Imports are added for various traits and structs. Ensure these new imports are used in the modified or added code segments within this file to justify their inclusion.
- 1565-1581: The method
broadcast_latest_holder_commitment_txnis introduced with a clear warning about its unsafe nature. Ensure that the documentation and method signature accurately reflect its intended use and the risks involved. Additionally, verify that the trait bounds and parameters (broadcaster,fee_estimator,logger) are correctly utilized within the method body.- 1768-1787: The
signer_unblockedmethod is added to trigger rebroadcasts of pending claims. Ensure that the method's implementation correctly uses the providedbroadcaster,fee_estimator, andloggerparameters, and that theFeerateStrategy::RetryPreviousis appropriately applied.- 1835-1838: The
do_signer_calltest utility method is introduced. Confirm that this method is only available in test configurations and that it correctly invokes the provided closure with theSignerinstance.- 2885-2885: Logging statements are added to warn about the availability of a potentially-unsafe holder commitment transaction. Ensure that these logs provide clear guidance to operators on how to proceed safely.
- 3554-3559: The logic for handling HTLC transactions in
unsafe_get_latest_holder_commitment_txnis modified. Ensure that the changes correctly identify and include signed HTLC transactions inholder_transactions.
wpaulino
force-pushed
the
retryable-holder-sigs
branch
from
f35adc7
to
8e61b5f
Compare
|
Had to rebase to fix CI. |
This method is meant to be used as a last resort when a user is forced to broadcast the current state, even if it is stale, in an attempt to claim their funds in the channel. Previously, we'd return the commitment and HTLC transactions such that they broadcast them themselves. Doing so required a different code path, one which was not tested, to obtain these transactions than our usual path when force closing. It's not worth maintaining both, and it's much simpler for us to broadcast instead.
This refactors the existing `force_feerate_bump` flag into an enum as we plan to introduce a new flag/enum variant in a future commit.
As part of the ongoing async signer work, our holder signatures must also be capable of being obtained asynchronously. We expose a new `ChannelMonitor::signer_unblocked` method to retry pending onchain claims by re-signing and rebroadcasting transactions. Unfortunately, we cannot retry said claims without them being registered first, so if we're not able to obtain the signature synchronously, we must return the transaction as unsigned and ensure it is not broadcast.
wpaulino
force-pushed
the
retryable-holder-sigs
branch
from
8e61b5f
to
e38dcca
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (13)
- lightning/src/chain/chainmonitor.rs (1 hunks)
- lightning/src/chain/channelmonitor.rs (7 hunks)
- lightning/src/chain/onchaintx.rs (18 hunks)
- lightning/src/chain/package.rs (8 hunks)
- lightning/src/chain/transaction.rs (2 hunks)
- lightning/src/ln/async_signer_tests.rs (2 hunks)
- lightning/src/ln/channel.rs (1 hunks)
- lightning/src/ln/channelmanager.rs (1 hunks)
- lightning/src/ln/functional_test_utils.rs (1 hunks)
- lightning/src/ln/monitor_tests.rs (2 hunks)
- lightning/src/sign/ecdsa.rs (6 hunks)
- lightning/src/util/test_channel_signer.rs (4 hunks)
- lightning/src/util/test_utils.rs (4 hunks)
Files not reviewed due to errors (1)
- lightning/src/ln/functional_test_utils.rs (Error: unable to parse review)
Files skipped from review as they are similar to previous changes (10)
- lightning/src/chain/chainmonitor.rs
- lightning/src/chain/onchaintx.rs
- lightning/src/chain/package.rs
- lightning/src/ln/async_signer_tests.rs
- lightning/src/ln/channel.rs
- lightning/src/ln/channelmanager.rs
- lightning/src/ln/monitor_tests.rs
- lightning/src/sign/ecdsa.rs
- lightning/src/util/test_channel_signer.rs
- lightning/src/util/test_utils.rs
Additional comments: 7
lightning/src/chain/transaction.rs (1)
- 83-85: The method
is_fully_signedcorrectly checks if all inputs are fully signed by verifying the witness is not empty for each input. This logic is sound for determining the signature status of a transaction in the context of Bitcoin transactions where the presence of witness data is indicative of a signed input.lightning/src/chain/channelmonitor.rs (6)
- 47-47: The imports are updated to include new dependencies. Ensure that all newly imported types are used within the file to avoid unnecessary imports.
- 1565-1581: The method
broadcast_latest_holder_commitment_txnis introduced with a detailed explanation of its unsafe nature. Ensure that the method's usage is strictly controlled and documented, especially given its potential consequences. The method signature correctly uses trait bounds for the broadcaster, fee estimator, and logger, promoting flexibility in the implementation.- 1768-1787: The
signer_unblockedmethod is introduced for rebroadcasting pending claims after a transaction signature generation failure. The method signature and logic appear correct, using trait bounds for the broadcaster, fee estimator, and logger. The use ofLowerBoundedFeeEstimatorandWithChannelMonitorwrapper for logging is consistent with the rest of the file. Ensure that theFeerateStrategy::RetryPreviousis the intended strategy for this context.- 1835-1838: The
do_signer_callmethod is introduced for testing purposes, allowing a closure to be executed with theSigneras an argument. This method is correctly gated with a#[cfg(test)]attribute, ensuring it's only available during testing. The implementation is straightforward and serves its intended purpose without introducing unnecessary complexity.- 2885-2885: Logging statements are added to warn about the availability of a potentially-unsafe holder commitment transaction. This is a critical addition for informing operators of the risks associated with broadcasting such transactions. Ensure that the documentation for
ChannelMonitor::broadcast_latest_holder_commitment_txnprovides clear guidance on how to proceed safely.- 3554-3559: The logic for handling HTLC transactions in the
unsafe_get_latest_holder_commitment_txnmethod is updated. This change appears to correctly handle the retrieval and signing of HTLC transactions. Ensure that the method's usage ofis_fully_signedcorrectly reflects the transaction's signing status and that the handling of thehtlc_txis consistent with the intended logic for dealing with HTLCs.
| @@ -76,6 +76,15 @@ impl core::fmt::Display for OutPoint { | |||
|
|
|||
| impl_writeable!(OutPoint, { txid, index }); | |||
|
|
|||
| #[derive(Debug, Clone)] | |||
| pub(crate) struct MaybeSignedTransaction(pub Transaction); | |||
There was a problem hiding this comment.
Define the visibility of MaybeSignedTransaction more explicitly if it's intended for use outside of the crate; otherwise, ensure the restricted visibility is intentional.
| @@ -633,14 +634,14 @@ impl PackageSolvingData { | |||
| } | |||
| true | |||
| } | |||
| fn get_finalized_tx<Signer: WriteableEcdsaChannelSigner>(&self, outpoint: &BitcoinOutPoint, onchain_handler: &mut OnchainTxHandler<Signer>) -> Option<Transaction> { | |||
| fn get_maybe_finalized_tx<Signer: WriteableEcdsaChannelSigner>(&self, outpoint: &BitcoinOutPoint, onchain_handler: &mut OnchainTxHandler<Signer>) -> Option<MaybeSignedTransaction> { | |||
There was a problem hiding this comment.
I'm still kinda hung up on this Option thing. Because there are essentially three states now: None, Some(non-finalized tx), and Some(finalized tx). The nesting feels like it should give way to an enum. Not really worth the refactor imo, but I think it would improve legibility in the future.
As part of the ongoing async signer work, our holder signatures must also be capable of being obtained asynchronously. We rely on our existing
ChainMonitor::rebroadcast_pending_claimsmethod to retry pending onchain claims by re-signing and rebroadcasting transactions. Unfortunately, we cannot retry said claims without them being registered first, so if we're not able to obtain the signature synchronously, we must return the transaction as unsigned and ensure it is not broadcast.Fixes #2520.