Support async signing of splice shared input#4579
Support async signing of splice shared input#4579wpaulino wants to merge 1 commit intolightningdevkit:mainfrom
Conversation
|
👋 Thanks for assigning @jkczyz as a reviewer! |
| if let Some((splice_tx, tx_type)) = msgs | ||
| .funding_tx_signed | ||
| .as_mut() | ||
| .and_then(|funding_tx_signed| funding_tx_signed.funding_tx.take()) | ||
| { | ||
| debug_assert!(matches!(tx_type, TransactionType::Splice { .. })); | ||
| log_info!( | ||
| logger, | ||
| "Broadcasting signed splice transaction with txid {}", | ||
| splice_tx.compute_txid(), | ||
| ); | ||
| self.tx_broadcaster.broadcast_transactions(&[(&splice_tx, tx_type)]); |
There was a problem hiding this comment.
Nit: The variable name splice_tx and the debug_assert!(matches!(tx_type, TransactionType::Splice { .. })) assume this funding_tx is always a splice transaction. While this assertion is correct for all currently reachable paths (V2 initial funding cannot reach on_tx_signatures_exchange from signer_maybe_unblocked because the counterparty can't send tx_signatures before receiving our commitment_signed), the FundingTxSigned struct is generic enough to carry either type. If V2 dual-funding support evolves and this path becomes reachable for initial funding, the assert would fire and emit_channel_pending_event! would be missing (unlike the internal_tx_signatures handler which calls broadcast_interactive_funding).
Consider either renaming splice_tx → funding_tx and handling both cases, or at minimum adding a comment explaining why this is splice-only.
| let mut shared_input_signature_unblocked = false; | ||
| { | ||
| if let Some(signing_session) = self.context.interactive_tx_signing_session.as_mut() { | ||
| if signing_session.awaiting_holder_shared_input_signature() { | ||
| let splice_input_index = signing_session | ||
| .unsigned_tx() | ||
| .shared_input_index() | ||
| .expect("Missing shared input index while awaiting a splice signature"); | ||
| log_trace!(logger, "Attempting to generate pending splice shared input signature..."); | ||
| if let Ok(shared_input_signature) = self.context.holder_signer.sign_splice_shared_input( | ||
| &self.funding.channel_transaction_parameters, | ||
| signing_session.unsigned_tx().tx(), | ||
| splice_input_index as usize, | ||
| &self.context.secp_ctx, | ||
| ) { | ||
| shared_input_signature_unblocked = true; | ||
| signing_session | ||
| .provide_holder_shared_input_signature(shared_input_signature) | ||
| .map_err(ChannelError::close)?; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| let mut tx_signatures = None; | ||
| let mut funding_tx = None; | ||
| if funding_commit_sig.is_some() || shared_input_signature_unblocked { | ||
| if let Some(signing_session) = self.context.interactive_tx_signing_session.as_ref() { | ||
| signing_session.holder_tx_signatures().filter(|_| !self.is_awaiting_monitor_update()) | ||
| if !self.is_awaiting_monitor_update() && !self.context.signer_pending_funding { | ||
| tx_signatures = signing_session.holder_tx_signatures(); | ||
| funding_tx = tx_signatures.as_ref().and_then(|_| signing_session.signed_tx()); | ||
| } | ||
| } else { | ||
| debug_assert!(false); | ||
| None | ||
| } | ||
| } else { | ||
| None | ||
| }; | ||
| } |
There was a problem hiding this comment.
Minor: the return value of provide_holder_shared_input_signature (which includes (Option<TxSignatures>, Option<Transaction>)) is discarded at line 9974 and then re-fetched via signing_session.holder_tx_signatures() and signing_session.signed_tx() at lines 9986-9987. Each call to signed_tx() internally calls holder_tx_signatures() again, resulting in 3 total calls to holder_tx_signatures() (which clones and rebuilds each time). Not a correctness issue, but you could reuse the values from provide_holder_shared_input_signature to avoid redundant cloning.
| let holder_tx_signatures = self.holder_tx_signatures.as_ref()?; | ||
| let counterparty_tx_signatures = self.counterparty_tx_signatures.as_ref()?; | ||
| let shared_input_signature = self.shared_input_signature.as_ref(); | ||
| let holder_tx_signatures = self.holder_tx_signatures()?; |
There was a problem hiding this comment.
Note: signed_tx() previously accessed self.holder_tx_signatures (the field) directly, but now calls self.holder_tx_signatures() (the method). The method adds two filters:
- Shared input signature must be present (new in this PR - correct for async signing)
- Timing condition:
(has_received_commitment_signed && holder_sends_tx_signatures_first) || has_received_tx_signatures()
The timing filter is effectively a no-op here because counterparty_tx_signatures.clone()? on the next line already returns None if counterparty hasn't sent theirs. But it changes signed_tx() from being purely about "are all signatures available" to also encoding protocol ordering constraints.
|
All previously identified critical issues have been resolved in the current version of the code:
No new inline comments posted in this review pass. All prior inline comments (efficiency nits on return value reuse, naming suggestions) remain applicable but were already posted. Review SummaryNo new issues found. The core async signing flow for splice shared inputs is correct:
Prior inline comments still applicable (already posted, not re-posted)
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4579 +/- ##
==========================================
+ Coverage 86.09% 86.10% +0.01%
==========================================
Files 157 157
Lines 108828 108941 +113
Branches 108828 108941 +113
==========================================
+ Hits 93694 93803 +109
+ Misses 12519 12512 -7
- Partials 2615 2626 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
🔔 1st Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
| @@ -612,9 +613,21 @@ impl InteractiveTxSigningSession { | |||
| self.holder_tx_signatures.is_some() | |||
There was a problem hiding this comment.
Shouldn't most callers of this now be checking if we have the shared input as well?
There was a problem hiding this comment.
I already accounted for this. All callers of has_holder_tx_signatures now only care about whether the user approved the transaction by signing, not necessarily if it's fully signed (with the shared input signature).
There was a problem hiding this comment.
Hmm.the log in channel_reestablish probably needs updating, maybe also the not_broadcasted_initial_funding in shutdown and is_funding_broadcastable, (though those are for V2, so I guess it doesn't matter). Also the handling in commitment_signed isn't clear to me at all - that is load-bearing in a few places, do we handle it correctly if we suddenly allow state to move forward a few steps but are still somehow waiting to send our last sig?
There was a problem hiding this comment.
ISTM this method probably needs renaming, though, cause it currently implies we have all the holder's tx signatures, but we don't.
There was a problem hiding this comment.
the log in channel_reestablish probably needs updating
This seems fine? If we don't have the holder tx_signatures and the user hasn't provided their witnesses yet, then InteractiveTxSigningSession::holder_tx_signatures should be None and we log we're waiting for the user's signatures.
Also the handling in commitment_signed isn't clear to me at all - that is load-bearing in a few places, do we handle it correctly if we suddenly allow state to move forward a few steps but are still somehow waiting to send our last sig?
I don't quite follow (maybe the name change helps with understanding), processing the counterparty's commitment_signed is something that needs to happen prior to us releasing our tx_signatures, which we do within the same lock in ChannelManager::funding_transaction_signed regardless of whether the shared input signature is async or not.
wpaulino
force-pushed
the
async-sign-shared-input
branch
from
9df5f83 to
fc1921c
Compare
wpaulino
force-pushed
the
async-sign-shared-input
branch
from
fc1921c to
436c5fb
Compare
wpaulino
force-pushed
the
async-sign-shared-input
branch
2 times, most recently
from
6cc0b8c to
25cd9e0
Compare
wpaulino
force-pushed
the
async-sign-shared-input
branch
from
25cd9e0 to
513e8cf
Compare
| shared_input_signature_unblocked = true; | ||
| signing_session | ||
| .provide_holder_shared_input_signature(shared_input_signature) | ||
| .map_err(ChannelError::close)?; |
There was a problem hiding this comment.
Nit: The return value of provide_holder_shared_input_signature (which is Result<(Option<TxSignatures>, Option<Transaction>), String>) is discarded here, then immediately re-fetched via signing_session.holder_tx_signatures() and signing_session.signed_tx() at lines 10044-10045. Each call to signed_tx() internally calls holder_tx_signatures() again, resulting in 3-4 total calls to holder_tx_signatures() (which clones every time) and 2 calls to finalize(). Consider reusing the return value:
let (unblocked_tx_signatures, unblocked_funding_tx) = signing_session
.provide_holder_shared_input_signature(shared_input_signature)
.map_err(ChannelError::close)?;Then use these values directly instead of re-fetching.
There was a problem hiding this comment.
This doesn't make sense, if we have a shared input then we'd never have a finalized transaction in the earlier call.
While user signatures may be provided whenever ready at the user's discretion when handling a `FundingTransactionReadyForSigning` event, it does not cover the user's signature for the 2-of-2 multisig input in a splice. This signature is obtained via the `EcdsaChannelSigner`, which did not support providing it asynchronously. Since the splice shared input signature is part of the `tx_signatures` message, we're not allowed to send the message until it's complete. This results in us needing to explicitly handle the signature exchange logic when the signer unblocks the shared input signature.
wpaulino
force-pushed
the
async-sign-shared-input
branch
from
513e8cf to
d794030
Compare
| } | ||
|
|
||
| pub fn has_holder_tx_signatures(&self) -> bool { | ||
| pub fn has_holder_tx_witnesses(&self) -> bool { |
There was a problem hiding this comment.
Consider naming has_holder_witnesses to match provide_holder_witnesses.
5 participants
While user signatures may be provided whenever ready at the user's discretion when handling a
FundingTransactionReadyForSigningevent, it does not cover the user's signature for the 2-of-2 multisig input in a splice. This signature is obtained via theEcdsaChannelSigner, which did not support providing it asynchronously.Since the splice shared input signature is part of the
tx_signaturesmessage, we're not allowed to send the message until it's complete. This results in us needing to explicitly handle the signature exchange logic when the signer unblocks the shared input signature.Fixes #4533.