Skip to content

[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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

optout21
Copy link
Contributor

@optout21 optout21 commented Apr 15, 2025

Implementation of transaction negotiation during splicing.
Builds on 3407 and 3443.

  • No new phase, Funded(FundedChannel) is used throughout splicing
  • Both FundedChannel and PendingV2Channel can act as a transaction constructor
  • PendingV2Channel logic is put behind a trait -- FundingTxConstructorV2
  • A RenegotiatingScope is used to store extra state during splicing
  • FundingChannel can act as a FundingTxConstructorV2, using the state from RenegotiatingScope (if present)
  • Since both FundedChannel and FundingTxConstructor has context(), context accessors are extracted into a common base trait, ChannelContextProvider (it is also shared by InitialRemoteCommitmentReceiver).

(Also relevant: #3444)

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Apr 15, 2025

👋 Thanks for assigning @wpaulino as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@ldk-reviews-bot
Copy link

🔔 1st Reminder

Hey @jkczyz @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

1 similar comment
@ldk-reviews-bot
Copy link

🔔 1st Reminder

Hey @jkczyz @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link

🔔 2nd Reminder

Hey @jkczyz @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

1 similar comment
@ldk-reviews-bot
Copy link

🔔 2nd Reminder

Hey @jkczyz @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link

🔔 3rd Reminder

Hey @jkczyz @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

1 similar comment
@ldk-reviews-bot
Copy link

🔔 3rd Reminder

Hey @jkczyz @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link

🔔 4th Reminder

Hey @jkczyz @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

1 similar comment
@ldk-reviews-bot
Copy link

🔔 4th Reminder

Hey @jkczyz @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link

🔔 5th Reminder

Hey @jkczyz @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

1 similar comment
@ldk-reviews-bot
Copy link

🔔 5th Reminder

Hey @jkczyz @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link

🔔 6th Reminder

Hey @jkczyz @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

1 similar comment
@ldk-reviews-bot
Copy link

🔔 6th Reminder

Hey @jkczyz @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

Copy link
Contributor

@jkczyz jkczyz left a 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.

Copy link

codecov bot commented Apr 30, 2025

Codecov Report

Attention: Patch coverage is 56.30252% with 52 lines in your changes missing coverage. Please review.

Project coverage is 90.28%. Comparing base (8aae34e) to head (f1c280d).
Report is 15 commits behind head on main.

Files with missing lines Patch % Lines
lightning/src/ln/channel.rs 55.23% 46 Missing and 1 partial ⚠️
lightning/src/ln/channelmanager.rs 64.28% 4 Missing and 1 partial ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@optout21 optout21 force-pushed the splice-dual-tx4 branch 3 times, most recently from 88d2e83 to 866368d Compare May 5, 2025 11:59
@optout21
Copy link
Contributor Author

optout21 commented May 6, 2025

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.
I did the changes in separate 'fix' commits.

@optout21
Copy link
Contributor Author

Ready for a new round of review. All pending and newly raised comments addressed.

@ldk-reviews-bot
Copy link

🔔 2nd Reminder

Hey @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@optout21
Copy link
Contributor Author

Comments addressed, minor fixes. Ready for review.

@optout21 optout21 requested a review from wpaulino June 21, 2025 13:38
@ldk-reviews-bot
Copy link

🔔 3rd Reminder

Hey @jkczyz @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link

🔔 1st Reminder

Hey @jkczyz @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

Copy link
Contributor

@wpaulino wpaulino left a 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>,
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@ldk-reviews-bot
Copy link

🔔 4th Reminder

Hey @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

optout21 added 3 commits June 26, 2025 06:20
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).
@optout21
Copy link
Contributor Author

optout21 commented Jun 26, 2025

Rebased, some new comments addressed (there are still a few outstanding).

@optout21
Copy link
Contributor Author

Please go back through all previous review comments, there are still a handful that need to 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.

@optout21 optout21 force-pushed the splice-dual-tx4 branch 2 times, most recently from 38e773a to 596e052 Compare June 26, 2025 07:13
@optout21
Copy link
Contributor Author

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.
So I'm waiting for feedback.
There are a few comment discussions left open, which may result if further changes, notably:

@ldk-reviews-bot
Copy link

🔔 5th Reminder

Hey @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link

🔔 6th Reminder

Hey @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link

🔔 7th Reminder

Hey @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

Copy link
Contributor

@jkczyz jkczyz left a 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);
Copy link
Contributor

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:

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.

Comment on lines 10588 to 10599
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
)?;
Copy link
Contributor

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.

Copy link
Contributor Author

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.

optout21 added 2 commits July 7, 2025 11:33
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.
@optout21
Copy link
Contributor Author

optout21 commented Jul 7, 2025

Fixes applied to commits, new comments addressed. Ready for review.

@jkczyz jkczyz mentioned this pull request Jul 7, 2025
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>,
Copy link
Contributor

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()))?)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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,
Copy link
Contributor

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

Comment on lines +1787 to +1789
let (commitment_signed, event) =
negotiating_channel.funding_tx_constructed(signing_session, &&logger)?;
Ok((commitment_signed, event))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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 {
Copy link
Contributor

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)
Copy link
Contributor

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

Comment on lines +2790 to +2791
interactive_tx_constructor: &'a mut Option<InteractiveTxConstructor>,
interactive_tx_signing_session: &'a mut Option<InteractiveTxSigningSession>,
Copy link
Contributor

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() {
Copy link
Contributor

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
Copy link
Contributor

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(
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants