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 @TheBlueMatt 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.
Review SummaryAfter thoroughly reviewing all files in this PR diff, I found one new issue not covered by prior review comments. Inline comments posted:
Cross-cutting observations:
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4579 +/- ##
==========================================
- Coverage 87.15% 86.13% -1.03%
==========================================
Files 161 157 -4
Lines 109251 108916 -335
Branches 109251 108916 -335
==========================================
- Hits 95215 93810 -1405
- Misses 11560 12490 +930
- Partials 2476 2616 +140
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. |
There was a problem hiding this comment.
TBH it seems like a weird API to pass shared_input_signature None when it was a splice. At a minimum it needs to be documented but it kinda feels like it'd be nicer to separately call provide_holder_shared_input_signature, dunno how crazy the refactor would be though. Same with the call to on_tx_signatures_exchange below.
There was a problem hiding this comment.
Let me know if what I pushed is what you had in mind. Not sure what you meant by the on_tx_signatures_exchange comment though.
| @@ -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).
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
9df5f83 to
fc1921c
Compare
| if let Some(msg) = msgs | ||
| .funding_tx_signed | ||
| .as_mut() | ||
| .and_then(|funding_tx_signed| funding_tx_signed.commitment_signed.take()) | ||
| { |
There was a problem hiding this comment.
The signer_unblocked handler processes FundingTxSigned fields individually via .take(), but never asserts that counterparty_initial_commitment_signed_result is None. Both internal_tx_signatures (line 12207) and monitor_updated_completion_actions (line 11059) include this assertion. Without it, if signer_maybe_unblocked ever populates this field in a future change, the monitor update would be silently dropped — a data-loss bug.
| if let Some(msg) = msgs | |
| .funding_tx_signed | |
| .as_mut() | |
| .and_then(|funding_tx_signed| funding_tx_signed.commitment_signed.take()) | |
| { | |
| if let Some(msg) = msgs | |
| .funding_tx_signed | |
| .as_mut() | |
| .and_then(|funding_tx_signed| funding_tx_signed.commitment_signed.take()) | |
| { | |
| debug_assert!(msgs.funding_tx_signed.as_ref() | |
| .map(|f| f.counterparty_initial_commitment_signed_result.is_none()) | |
| .unwrap_or(true)); |
4 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.