-
Notifications
You must be signed in to change notification settings - Fork 417
[Splicing] Tx negotiation during splicing #3736
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! |
7f6dfbd
to
c3778bc
Compare
1 similar comment
1 similar comment
1 similar comment
1 similar comment
1 similar comment
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.
Sorry about the late review. We were traveling to an off site last week. Just a high-level pass on the first four commits. Will need to take a closer look at the last one.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3736 +/- ##
==========================================
+ Coverage 89.73% 90.28% +0.55%
==========================================
Files 159 160 +1
Lines 128910 132032 +3122
Branches 128910 132032 +3122
==========================================
+ Hits 115676 119207 +3531
+ Misses 10536 10172 -364
+ Partials 2698 2653 -45 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
88d2e83
to
866368d
Compare
Ready for a new round of review. I have addressed the comments, applied most of them. There is still one to-do (update channel reserve values), that I will do, but the rest is ready for review. |
Ready for a new round of review. All pending and newly raised comments addressed. |
🔔 2nd Reminder Hey @jkczyz! This PR has been waiting for your review. |
3577418
to
586a094
Compare
Comments addressed, minor fixes. Ready for 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.
Please go back through all previous review comments, there are still a handful that need to addressed...
fn begin_interactive_funding_tx_construction<ES: Deref>( | ||
&mut self, signer_provider: &SP, entropy_source: &ES, holder_node_id: PublicKey, | ||
change_destination_opt: Option<ScriptBuf>, | ||
is_initiator: bool, change_destination_opt: Option<ScriptBuf>, |
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.
If we make is_initiator
part of FundingNegotiationContext
, it seems like we could just provide a reference of it to calculate_change_output_value
to clean things up a bit.
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.
FundingNegotiationContext
is defined in channel.rs
, and using it in interactivetxs.rs
doesn't seem right.
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.
Really calculate_change_output_value
doesn't belong in interactivetxs.rs
, the protocol is not concerned with change outputs.
🔔 4th Reminder Hey @jkczyz! This PR has been waiting for your review. |
This is a simple rename, DualFundingContext to FundingNegotiationContext, to suggest that this is use not only in dual-funded channel open. Also rename the field dual_funding_context to funding_negotiation_context.
The begin_interactive_funding_tx_construction() method is extended with `is_initiator` parameter (splice initiator), and `prev_funding_input` optional parameter, containing the previous funding transaction, which will be added to the negotiation as an input by the initiator.
Introduce struct NegotiatingChannelView to perform transaction negotiation logic, on top of either PendingV2Channel (dual-funded channel open) or FundedChannel (splicing).
0afb374
to
79b8083
Compare
Rebased, some new comments addressed ( |
Thanks for the heads up, I did miss some comments that are visible in the Files Changed, but not in the main Conversation tab. I will address those. |
38e773a
to
596e052
Compare
Addressed some pending comments, I've rechecked all outstanding (some only show up in Files Changed view), and currently I don't see any items I could address or need to do changes for.
|
🔔 5th Reminder Hey @jkczyz! This PR has been waiting for your review. |
🔔 6th Reminder Hey @jkczyz! This PR has been waiting for your review. |
🔔 7th Reminder Hey @jkczyz! 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.
Could you squash the fixups with their relevant commits?
))); | ||
} | ||
|
||
self.context.assert_no_commitment_advancement(self.holder_commitment_transaction_number, "initial commitment_signed"); | ||
let commitment_signed = self.context.get_initial_commitment_signed(&self.funding, 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.
Hmm... I think this is referring to:
rust-lightning/lightning/src/ln/channel.rs
Line 8458 in 257ebad
let commitment_signed = self.context.get_initial_commitment_signed(&self.funding, logger)?; |
This is an FYI to me, so you shouldn't need to address anything.
lightning/src/ln/channel.rs
Outdated
let pre_channel_value = self.funding.get_value_satoshis(); | ||
let their_funding_contribution = msg.funding_contribution_satoshis; | ||
|
||
let post_channel_value = PendingSplice::compute_post_value( | ||
pre_channel_value, | ||
our_funding_contribution, | ||
their_funding_contribution, | ||
); | ||
|
||
let (our_funding_satoshis, their_funding_satoshis) = calculate_total_funding_contribution( | ||
pre_channel_value, | ||
our_funding_contribution, | ||
msg.funding_contribution_satoshis, | ||
false, // is_outbound | ||
)?; |
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 any of this be moved into FundingScope::for_splice
? It would be nice if we could avoid the debug_assert
in there.
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 really. The debug assert in FundingScope::for_splice
says that we cannot splice out more than our balance. The reserve check should implicitly guard against this as well: we cannot splice out more than our balance plus the minimum reserve. (#3641).
The calculate_total_funding_contribution
does not deal with current balances, so that's a different check.
Fill the logic for including transaction negotiation during splicing, implement the functions: splice_channel, splice_init, splice_ack, funding_tx_constructed. Also extend the test case test_v1_splice_in with the steps for funding negotiation during splicing.
Fixes applied to commits, new comments addressed. Ready for review. |
fn begin_interactive_funding_tx_construction<ES: Deref>( | ||
&mut self, signer_provider: &SP, entropy_source: &ES, holder_node_id: PublicKey, | ||
change_destination_opt: Option<ScriptBuf>, | ||
is_initiator: bool, change_destination_opt: Option<ScriptBuf>, |
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.
Really calculate_change_output_value
doesn't belong in interactivetxs.rs
, the protocol is not concerned with change outputs.
ChannelPhase::UnfundedV2(chan) => Ok(chan.as_negotiating_channel()), | ||
#[cfg(splicing)] | ||
ChannelPhase::Funded(chan) => { | ||
Ok(chan.as_renegotiating_channel().map_err(|err| ChannelError::Warn(err.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.
Ok(chan.as_renegotiating_channel().map_err(|err| ChannelError::Warn(err.into()))?) | |
chan.as_renegotiating_channel().map_err(|err| ChannelError::Warn(err.into())) |
pub our_funding_contribution: i64, | ||
funding: Option<FundingScope>, | ||
funding_negotiation_context: FundingNegotiationContext, |
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 we shouldn't need our_funding_contribution
above anymore
let (commitment_signed, event) = | ||
negotiating_channel.funding_tx_constructed(signing_session, &&logger)?; | ||
Ok((commitment_signed, event)) |
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 (commitment_signed, event) = | |
negotiating_channel.funding_tx_constructed(signing_session, &&logger)?; | |
Ok((commitment_signed, event)) | |
negotiating_channel.funding_tx_constructed(signing_session, &&logger) |
#[cfg(splicing)] | ||
fn as_renegotiating_channel(&mut self) -> Result<NegotiatingChannelView<SP>, &'static str> { | ||
if let Some(ref mut pending_splice) = &mut self.pending_splice { | ||
if let Some(ref mut funding) = &mut pending_splice.funding { |
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.
On second thought, I think checking for the interactive_tx_constructor
instead is more correct, as that's what we're mainly using whenever as_renegotiating_channel
gets called.
ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(false) }, | ||
))), chan_entry) | ||
Err(err) => { | ||
try_channel_entry!(self, peer_state, Err(err), chan_entry) |
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 think we can just send a warning here, as we do with the other tx_*
messages
interactive_tx_constructor: &'a mut Option<InteractiveTxConstructor>, | ||
interactive_tx_signing_session: &'a mut Option<InteractiveTxSigningSession>, |
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 don't think we need to track interactive_tx_signing_session
at all here, as long as we separately set the session in funding_tx_constructed
.
interactive_tx_constructor
also shouldn't be an Option
, but it seems to be that way right now because of begin_interactive_funding_tx_construction
and funding_tx_constructed
.
For begin_interactive_funding_tx_construction
, maybe it makes more sense to make it a standalone function (no longer a member on NegotiatingChannelView
) that simply returns an InteractiveTxConstructor
that we can set on the channel separately.
return Err(ChannelError::Warn(format!("Channel is not in pending splice"))); | ||
}; | ||
|
||
// TODO(splicing): Add check that we are the splice (quiescence) initiator | ||
|
||
if pending_splice.funding.is_some() || pending_splice.interactive_tx_constructor.is_some() { |
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.
Once we have #3911, funding
will move into negotiated_candidates
after exchanging tx_signatures
, so we'll also need to check that negotiated_candidates
is not empty before proceeding.
channel_value_satoshis: post_channel_value, | ||
}; | ||
if let Some(ref mut counterparty_parameters) = | ||
post_channel_transaction_parameters.counterparty_parameters |
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 think we can expect
this
msg.funding_pubkey, | ||
)?; | ||
|
||
let (our_funding_satoshis, their_funding_satoshis) = calculate_total_funding_contribution( |
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'm a bit lost with the goal here, why are we mixing past contributions with new ones? When we say we're contributing 50k sats to a splice, we're adding 50k sats to the channel value. We shouldn't consider the past channel value as part of our contribution.
Implementation of transaction negotiation during splicing.
Builds on 3407 and 3443.
No new phase,Funded(FundedChannel)
is used throughout splicingFundedChannel
andPendingV2Channel
can act as a transaction constructorPendingV2Channel
logic is put behind a trait --FundingTxConstructorV2
RenegotiatingScope
is used to store extra state during splicingFundingChannel
can act as aFundingTxConstructorV2
, using the state fromRenegotiatingScope
(if present)Since bothFundedChannel
andFundingTxConstructor
has context(), context accessors are extracted into a common base trait,ChannelContextProvider
(it is also shared byInitialRemoteCommitmentReceiver
).(Also relevant: #3444)