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.
Review SummaryNew issue found
Prior comments status (still applicable)
|
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.
There was a problem hiding this comment.
Yea, this is much better.
| @@ -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).
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
| let holder_tx_signatures = self.holder_tx_signatures.as_mut().ok_or_else(|| { | ||
| "Holder witnesses must be provided before the shared input signature".to_string() | ||
| })?; | ||
| if holder_tx_signatures.shared_input_signature.is_some() { | ||
| return Err("The shared input signature was already provided".to_string()); | ||
| } |
There was a problem hiding this comment.
Should these never happen? i.e., We'll only call provide_holder_shared_input_signature after the user has called funding_transaction_signed and only for a splice.
Any reason why we don't allow providing the shared signature independent of the other witnesses? Or is it simpler to just query the signer once we have the other witnesses?
There was a problem hiding this comment.
They should never happen, we could debug_assert instead if you'd like. No particular reason to not do it before funding_transaction_signed, but why bother signing something when there's a possibility it gets canceled instead?
There was a problem hiding this comment.
Ah, right makes sense in case it is canceled. No strong opinion on the assertion.
| .map(|tx_signatures| { | ||
| self.shared_input().is_some() && tx_signatures.shared_input_signature.is_none() | ||
| }) | ||
| .unwrap_or(false) |
There was a problem hiding this comment.
I see how this is called, but it seems odd that we are only considered awaiting them once the user has provided their own witnesses.
wpaulino
force-pushed
the
async-sign-shared-input
branch
from
436c5fb to
6cc0b8c
Compare
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
6cc0b8c to
25cd9e0
Compare
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.