rust-lightning

rust-lightning copied to clipboard

lightningdevkit

Reame
Issues

[Splicing] Tx negotiation during splicing

Open optout21 opened this issue 8 months ago • 14 comments

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)

Apr 15 '25 13:04 optout21

👋 I see @wpaulino was un-assigned. If you'd like another reviewer assignment, please click here.

Apr 15 '25 13:04 ldk-reviews-bot

🔔 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.

Apr 17 '25 14:04 ldk-reviews-bot

🔔 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.

Apr 17 '25 14:04 ldk-reviews-bot

🔔 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.

Apr 19 '25 14:04 ldk-reviews-bot

🔔 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.

Apr 19 '25 14:04 ldk-reviews-bot

🔔 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.

Apr 21 '25 14:04 ldk-reviews-bot

🔔 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.

Apr 21 '25 14:04 ldk-reviews-bot

🔔 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.

Apr 23 '25 14:04 ldk-reviews-bot

🔔 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.

Apr 23 '25 14:04 ldk-reviews-bot

🔔 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.

Apr 26 '25 00:04 ldk-reviews-bot

🔔 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.

Apr 26 '25 00:04 ldk-reviews-bot

🔔 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.

Apr 28 '25 00:04 ldk-reviews-bot

🔔 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.

Apr 28 '25 00:04 ldk-reviews-bot

Codecov Report

:x: Patch coverage is 67.36111% with 94 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 88.95%. Comparing base (7ec13dc) to head (41cbc1f). :warning: Report is 49 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	59.83%	49 Missing :warning:
lightning/src/ln/channelmanager.rs	33.33%	32 Missing and 6 partials :warning:
lightning/src/ln/interactivetxs.rs	93.57%	6 Missing and 1 partial :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3736      +/-   ##
==========================================
+ Coverage   88.92%   88.95%   +0.02%     
==========================================
  Files         174      174              
  Lines      123869   124200     +331     
  Branches   123869   124200     +331     
==========================================
+ Hits       110152   110476     +324     
+ Misses      11256    11247       -9     
- Partials     2461     2477      +16

Flag	Coverage Δ
fuzzing	`22.63% <0.00%> (+0.45%)`	:arrow_up:
tests	`88.77% <67.36%> (+0.02%)`	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

:snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Apr 30 '25 19:04 codecov[bot]

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.

May 06 '25 13:05 optout21

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

May 13 '25 10:05 optout21

I've done some of the bigger changes, but there are still several outstanding.

May 20 '25 20:05 optout21

Rebased to current main (there were some conflicts)

May 29 '25 07:05 optout21

Comments addressed, I will clean/reorg up the commits.

May 29 '25 09:05 optout21

I have rearranged the commits in this PR (there were some reverted changes, many fixes, etc.), into these 4:

preparation: Rename DualFundingContext
partial: Extend begin_interactive_...() with splicing-specific parameters
partial: Introduce NegotiatingV2ChannelView to bridge Funded and PendingV2
main logic: Implement transaction negotiation during splicing

Otherwise, no change applied this time. I'd appreciate a new round of review.

(Note: the old commits are available on branch splice-dual-tx4-save)

Jun 03 '25 10:06 optout21

For reference, summary of NOT-included changes:

Always pass previous funding TX to begin_interactive_funding_tx_construction(), not only in the splice initiator case, but in the splice acceptor case as well. The initiator should add it as an input to the interactive contructor. The acceptor should also add it as a Shared Input (this is TODO in IATX), to be able to verify, that initiator has added it. Reason for being deferred: on the acceptor side (v1 channel) the field funding.funding_transaction is currently never set. Set to be addressed in PR #3741.
Method internal_splice_channel takes owned input (not a reference). The drawback is that splice_channel needs to clone, due to optionally_notify. Could be solved by changing the FnMut in optionally_notify to FnOnce.

Jun 03 '25 21:06 optout21

Always pass previous funding TX to begin_interactive_funding_tx_construction(), not only in the splice initiator case, but in the splice acceptor case as well. The initiator should add it as an input to the interactive contructor. The acceptor should also add it as a Shared Input (this is TODO in IATX), to be able to verify, that initiator has added it. Reason for being deferred: on the acceptor side (v1 channel) the field funding.funding_transaction is currently never set. Set to be addressed in PR Exchange splice_locked messages #3741.

We had some discussion on https://github.com/lightningdevkit/rust-lightning/pull/3741#discussion_r2127512308 and on the spec PR (https://github.com/lightning/bolts/pull/1160#issuecomment-2942997584) about this. Turns out we need to make TxAddInput::prevtx optional or possible an enum with TxAddInput::shared_input_txid. We'll avoid storing the funding transaction.

Jun 05 '25 15:06 jkczyz

🔔 1st Reminder

Hey @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.

Jun 05 '25 21:06 ldk-reviews-bot

We had some discussion on #3741 (comment) and on the spec PR (lightning/bolts#1160 (comment)) about this. Turns out we need to make TxAddInput::prevtx optional or possible an enum with TxAddInput::shared_input_txid. We'll avoid storing the funding transaction.

Getting closer :) As I see the changes needed:

make TxAddInput::prevtx Optional
Enhance InteractiveConstructor to differentiate non-shared and shared inputs. For shared inputs set shared_input_txid (only the initiator). The acceptor should check if shared input was added.
To begin_interactive_... we should supply the prev_funding_txo, in both inititator&acceptor case. Both would add it to InteractiveConstructor as shared input.

In this PR or another one? Probably best would be to include these into this PR (causing some further processing time).

Jun 05 '25 21:06 optout21

In this PR or another one? Probably best would be to include these into this PR (causing some further processing time).

Yeah, I think we need to do it in this PR.

Jun 05 '25 21:06 jkczyz

I'm working on shared input support, can be seen in draft PR #3842 . WIP.

Jun 10 '25 15:06 optout21

Comments addressed, summary of pending changes:

Extra clone() added in channelmanager.rs when invoking internal_splice_channel: root cause addressed in #3835 .
Shared input support: Pending, see #3842. If 3842 is merged first, I will add the changes here; if this PR is merged first, I will update 3842.
Quiescence checks: the plan is to include them after this PR (wpaulino)

Jun 11 '25 20:06 optout21

Let us know when this is ready for another round, no need to wait on #3842 to get more review on this.

Jun 12 '25 22:06 wpaulino

Let us know when this is ready for another round, no need to wait on #3842 to get more review on this.

I was busy with #3842, but I agree that this PR should be treated independently of 3842. I will squash the fixups, and this should be ready for a new round of review.

Jun 16 '25 13:06 optout21

Commits squashed, rebased (post #3741), fmt. Ready for a new round of review.

Jun 17 '25 04:06 optout21