-
Notifications
You must be signed in to change notification settings - Fork 417
Introduce FundingTransactionReadyForSignatures
event
#3889
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
👋 Thanks for assigning @wpaulino as a reviewer! |
165d59d
to
8ca6d79
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3889 +/- ##
==========================================
- Coverage 88.93% 88.80% -0.13%
==========================================
Files 174 174
Lines 123875 124733 +858
Branches 123875 124733 +858
==========================================
+ Hits 110169 110771 +602
- Misses 11253 11468 +215
- Partials 2453 2494 +41
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:
|
1 similar comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
890633d
to
a1de384
Compare
🔔 2nd Reminder Hey @wpaulino! This PR has been waiting for your review. |
7df5779
to
c8f981c
Compare
🔔 3rd Reminder Hey @wpaulino! This PR has been waiting for your review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if there was a problem rebasing, but some comments that had been marked resolved weren't fixed.
Yeah, they got lost on a rebase and somehow lost the commit. Rebased to get the one CI fix in. Fixing. |
c15f426
to
ff1489d
Compare
🔔 4th Reminder Hey @wpaulino! This PR has been waiting for your review. |
ff1489d
to
0a586e6
Compare
🔔 5th Reminder Hey @wpaulino! This PR has been waiting for your review. |
🔔 6th Reminder Hey @wpaulino! This PR has been waiting for your review. |
a9e1a3a
to
83e78d6
Compare
b161cba
to
4dde88e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed latest fixups. Comments left on remaining ones.
58b5381
to
50e7cf4
Compare
ca2d5e0
to
b45913a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there, the commit history could be cleaned up a bit to not have code added in one commit that gets removed in a later one
Thanks, I did intend to avoid that but I definitely lost track of the changes. |
e518bd2
to
0dbc64f
Compare
🔔 1st Reminder Hey @jkczyz! This PR has been waiting for your review. |
0b377f9
to
07e3526
Compare
The `FundingTransactionReadyForSignatures` event requests witnesses from the client for their contributed inputs to an interactively constructed transaction. The client calls `ChannelManager::funding_transaction_signed` to provide the witnesses to LDK. The `handle_channel_resumption` method handles resumption from both a channel re-establish and a monitor update. When the corresponding monitor update for the commitment_signed message completes, we will push the event here. We can thus only ever provide holder signatures after a monitor update has completed. We can also get rid of the reestablish code involved with `monitor_pending_tx_signatures` and remove that field too.
…hecks In a following commit, We'll use the contained scriptPubKeys to validate P2WPKH and P2TR key path spends and to assist in checking that signatures in provided holder witnesses use SIGHASH_ALL to prevent funds being frozen or held ransom.
LDK checks the following: * Each input spends an output that is one of P2WPKH, P2WSH, or P2TR. These were already checked by LDK when the inputs to be contributed were provided. * All signatures use the `SIGHASH_ALL` sighash type. * P2WPKH and P2TR key path spends are valid (verifies signatures) NOTE: * When checking P2WSH spends, LDK tries to decode 70-72 byte witness elements as ECDSA signatures with a sighash flag. If the internal DER-decoding fails, then LDK just assumes it wasn't a signature and carries with checks. If the element can be decoded as an ECDSA signature, the the sighash flag must be `SIGHASH_ALL`. * When checking P2TR script-path spends, LDK assumes all elements of exactly 65 bytes with the last byte matching any valid sighash flag byte are schnorr signatures and checks that the sighash type is `SIGHASH_ALL`. If the last byte is not any valid sighash flag, the element is assumed not to be a signature and is ignored. Elements of 64 bytes are not checked because if they were schnorr signatures then they would implicitly be `SIGHASH_DEFAULT` which is an alias of `SIGHASH_ALL`.
07e3526
to
de9d375
Compare
@@ -6856,13 +6810,12 @@ where | |||
channel_id: Some(self.context.channel_id()), | |||
}; | |||
|
|||
let tx_signatures = self | |||
let _ = self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let _ = self | |
self |
Since this now returns void, it's best to ignore the return value completely. Otherwise, if the return parameter changes, rustc won't complain about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Thanks!
return Ok((None, None)); | ||
} | ||
|
||
// Note that `holder_tx_signatures_opt` will be `None` if we sent `tx_signatures` first or if the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should remove the TODOs about verifying witnesses above, also the input count check seems redundant as it's already checked in received_tx_signatures
.
if holder_tx_signatures_opt.is_some() && self.is_awaiting_initial_mon_persist() { | ||
log_debug!(logger, "Not sending tx_signatures: a monitor update is in progress. Setting monitor_pending_tx_signatures."); | ||
self.context.monitor_pending_tx_signatures = holder_tx_signatures_opt; | ||
return Ok((None, None)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We used to return early if holder_tx_signatures
wasn't ready, but now we don't and immediately transition to AwaitingChannelReady
. Also this state isn't applicable for splicing, we'd want to go back to ChannelReady
, but I guess that could be left for follow-up work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a TODO(splicing)
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems I lost a check here when I was working on testing.
Will add a TODO
for splicing.
} | ||
return Err(()); | ||
}; | ||
let commitment_signed = if let Ok(update) = self.send_commitment_no_state_update(logger) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated rustfmt change snuck in?
self.context.monitor_pending_tx_signatures = session.holder_tx_signatures().clone(); | ||
} | ||
if session.holder_tx_signatures().is_none() { | ||
debug_assert!(self.context.channel_state.is_monitor_update_in_progress()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry this was my mistake, we actually can't guarantee this, similar to the other debug_assert
we dealt with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries. Thanks, I think I just forgot to remove this one too.
'elements: for element in witness { | ||
match element.len() { | ||
// Possibly a DER-encoded ECDSA signature with a sighash type byte assuming low-S | ||
70..=72 => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be 73 max to account for the sighash flag itself? Would be nice to have coverage of all lengths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right, 72 bytes is actually high-r and low-s which is standard. So should be 73 here.
Thanks!
]), | ||
// P2WSH (Swept lightning anchor) | ||
Witness::from_slice(&[ | ||
&vec![], // empty | ||
&<Vec<u8>>::from_hex("2102fd481d39bdbc090313b530fddfd1aa004a9e3263da1406cf806670fdeb8ebb91ac736460b268").unwrap(), | ||
]), | ||
// P2WSH (Swept lightning anchor) | ||
Witness::from_slice(&[ | ||
&vec![], // empty | ||
&<Vec<u8>>::from_hex("2102fd481d39bdbc090313b530fddfd1aa004a9e3263da1406cf806670fdeb8ebb91ac736460b268").unwrap(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these repeated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The real transaction I found had 3 identical spending conditions for lightning anchor sweeps.
&<Vec<u8>>::from_hex("30440220036e88464b21c8bd819d97ae746622da00053ec1374a932f33aa1ab60170c9da022041cabc146ebdd12f6316a2f72f870771e8e6ff51f3cadad4027eab2e4437701101").unwrap(), | ||
&<Vec<u8>>::from_hex("030c7196376bc1df61b6da6ee711868fd30e370dd273332bfb02a2287d11e2e9c5").unwrap(), | ||
]), | ||
// P2WSH (Swept lightning anchor) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also include one that has a signature.
}]; | ||
|
||
let outputs: Vec<InteractiveTxOutput> = vec![ | ||
InteractiveTxOutput { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: one output should be enough, no need to make the test longer than it needs to be
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I grabbed real transactions and their signatures from on-chain. I could probably construct them myself if you'd like?
@@ -3095,4 +3277,331 @@ mod tests { | |||
assert_eq!(res, Ok(Some(262))); | |||
} | |||
} | |||
|
|||
#[test] | |||
fn test_verify_tx_signatures_p2tr() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be testing more than just P2TR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I think handcrafting the transactions and actually creating sigs would be better than trying to find test candidates from on chain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please be sure to include remote inputs in any handcrafted transactions to ensure the index pairing it correct after filtering remote inputs (looks like it is correct). AFAICT, the NegotiatedTxInput
s below are only for one party, so we never test that the correct input_idx
is passed to the cache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to review the last commit still.
/// The transaction contains all inputs provided by both parties along with the channel's funding | ||
/// output and a change output if applicable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may contain outputs added during interactive construction, too.
/// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager | ||
/// [`ChannelManager::funding_transaction_signed`]: crate::ln::channelmanager::ChannelManager::funding_transaction_signed | ||
FundingTransactionReadyForSigning { | ||
/// The channel_id of the channel which you'll need to pass back into |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add ticks to channel_id
/// | ||
/// [`ChannelManager::funding_transaction_signed`]: crate::ln::channelmanager::ChannelManager::funding_transaction_signed | ||
channel_id: ChannelId, | ||
/// The counterparty's node_id, which you'll need to pass back into |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise for node_id
.
// so they'll be sent as soon as that's done. | ||
self.context.monitor_pending_tx_signatures = Some(tx_signatures); | ||
} | ||
self.interactive_tx_signing_session.as_mut().map(|session| session.received_commitment_signed()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use if let
instead of obscuring the a side effect in map
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! Sorry, terrible habit of mine mixing mutability in functional methods.
}, | ||
Err(err) => { | ||
result = Err(err); | ||
return notify; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a need for notify
or can we just use an explicit return of a NotifyOption
everywhere? Doesn't look like any code is executed later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. Can just return the literal here.
.as_mut() | ||
.ok_or_else(|| APIError::APIMisuseError { | ||
err: format!( | ||
"Channel with id {} not expecting funding signatures", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just say "Channel {} not expecting funding signatures
}, None)); | ||
} else if signing_session.local_inputs_count() == 0 { | ||
match signing_session.provide_holder_witnesses(channel.context.channel_id(), vec![]) { | ||
Ok(Some(tx_signatures)) => pending_msg_events.push(MessageSendEvent::SendTxSignatures { node_id: counterparty_node_id, msg: tx_signatures }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to call FundedChannel::funding_transaction_signed
(or refactor part of that code) so the setting is all in one place?
Ok(None) => debug_assert!(false, "If our tx_signatures is empty, then we should send it first!"), | ||
Err(_) => debug_assert!( | ||
false, | ||
"Zero inputs were provided & zero witnesses were provided, but a count mismatch was somehow found", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Zero inputs were provided & zero witnesses were provided, but a count mismatch was somehow found", | |
"Zero inputs and witnesses were provided, but a count mismatch was somehow found", |
if holder_tx_signatures_opt.is_some() && self.is_awaiting_initial_mon_persist() { | ||
log_debug!(logger, "Not sending tx_signatures: a monitor update is in progress. Setting monitor_pending_tx_signatures."); | ||
self.context.monitor_pending_tx_signatures = holder_tx_signatures_opt; | ||
return Ok((None, None)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a TODO(splicing)
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appreciate the review!
Reworking the PR now.
]), | ||
// P2WSH (Swept lightning anchor) | ||
Witness::from_slice(&[ | ||
&vec![], // empty | ||
&<Vec<u8>>::from_hex("2102fd481d39bdbc090313b530fddfd1aa004a9e3263da1406cf806670fdeb8ebb91ac736460b268").unwrap(), | ||
]), | ||
// P2WSH (Swept lightning anchor) | ||
Witness::from_slice(&[ | ||
&vec![], // empty | ||
&<Vec<u8>>::from_hex("2102fd481d39bdbc090313b530fddfd1aa004a9e3263da1406cf806670fdeb8ebb91ac736460b268").unwrap(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The real transaction I found had 3 identical spending conditions for lightning anchor sweeps.
}]; | ||
|
||
let outputs: Vec<InteractiveTxOutput> = vec![ | ||
InteractiveTxOutput { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I grabbed real transactions and their signatures from on-chain. I could probably construct them myself if you'd like?
'elements: for element in witness { | ||
match element.len() { | ||
// Possibly a DER-encoded ECDSA signature with a sighash type byte assuming low-S | ||
70..=72 => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right, 72 bytes is actually high-r and low-s which is standard. So should be 73 here.
Thanks!
@@ -3095,4 +3277,331 @@ mod tests { | |||
assert_eq!(res, Ok(Some(262))); | |||
} | |||
} | |||
|
|||
#[test] | |||
fn test_verify_tx_signatures_p2tr() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I think handcrafting the transactions and actually creating sigs would be better than trying to find test candidates from on chain.
// so they'll be sent as soon as that's done. | ||
self.context.monitor_pending_tx_signatures = Some(tx_signatures); | ||
} | ||
self.interactive_tx_signing_session.as_mut().map(|session| session.received_commitment_signed()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! Sorry, terrible habit of mine mixing mutability in functional methods.
@@ -6856,13 +6810,12 @@ where | |||
channel_id: Some(self.context.channel_id()), | |||
}; | |||
|
|||
let tx_signatures = self | |||
let _ = self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Thanks!
if holder_tx_signatures_opt.is_some() && self.is_awaiting_initial_mon_persist() { | ||
log_debug!(logger, "Not sending tx_signatures: a monitor update is in progress. Setting monitor_pending_tx_signatures."); | ||
self.context.monitor_pending_tx_signatures = holder_tx_signatures_opt; | ||
return Ok((None, None)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems I lost a check here when I was working on testing.
Will add a TODO
for splicing.
self.context.monitor_pending_tx_signatures = session.holder_tx_signatures().clone(); | ||
} | ||
if session.holder_tx_signatures().is_none() { | ||
debug_assert!(self.context.channel_state.is_monitor_update_in_progress()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries. Thanks, I think I just forgot to remove this one too.
.filter_map(|input| { | ||
if input.witness.is_empty() { | ||
None | ||
} else { | ||
Some(input.witness) | ||
} | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lol duh! thanks :)
}, | ||
Err(err) => { | ||
result = Err(err); | ||
return notify; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. Can just return the literal here.
fn prev_output(&self) -> &TxOut { | ||
match self { | ||
InputOwned::Single(single) => &single.prev_output, | ||
InputOwned::Shared(shared) => &shared.prev_output, | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this is never used.
@@ -363,6 +379,10 @@ impl ConstructedTransaction { | |||
.zip(witnesses) | |||
.for_each(|(input, witness)| input.witness = witness); | |||
} | |||
|
|||
pub fn holder_is_initiator(&self) -> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't need to be pub
.
APIError::APIMisuseError { err } | ||
})?; | ||
|
||
continue 'inputs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using helper functions instead of using labeled continue
s.
|
||
// P2WSH - No validation just sighash checks | ||
if script_pubkey.is_p2wsh() { | ||
'elements: for element in witness { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these labels even necessary if you never continue
to an outer loop?
let err = format!("An ECDSA signature in the witness for input {input_idx} does not use SIGHASH_ALL"); | ||
return Err(APIError::APIMisuseError { err }); | ||
}, | ||
_ => continue 'elements, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed.
.map(|sig| matches!(sig.sighash_type, EcdsaSighashType::All)) | ||
.unwrap_or(true) | ||
{ | ||
continue 'elements; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Negate the condition to avoid needing a continue
.
@@ -3095,4 +3277,331 @@ mod tests { | |||
assert_eq!(res, Ok(Some(262))); | |||
} | |||
} | |||
|
|||
#[test] | |||
fn test_verify_tx_signatures_p2tr() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please be sure to include remote inputs in any handcrafted transactions to ensure the index pairing it correct after filtering remote inputs (looks like it is correct). AFAICT, the NegotiatedTxInput
s below are only for one party, so we never test that the correct input_idx
is passed to the cache.
Cherry-picked from #3735 as it is relevant to splicing and will unblock testing after #3736 lands.
The
FundingTransactionReadyForSignatures
event requests witnesses from the client for their contributed inputs to an interactively constructed transaction.The client calls
ChannelManager::funding_transaction_signed
to provide the witnesses to LDK.