Skip to content

Contribute funding inputs on accepting dual-funded channel #3735

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Draft

Contribute funding inputs on accepting dual-funded channel #3735

wants to merge 9 commits into from
+1,473 −701

Conversation

dunxen
Copy link
Contributor

@dunxen dunxen commented Apr 15, 2025 •
edited
Loading

WIP. Node restart tests pending. Will update this detail when ready for full review.

Based on #3842 and #3889

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Apr 15, 2025 •
edited
Loading

👋 Thanks for assigning @jkczyz 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.

@dunxen dunxen force-pushed the 2025-04-contributeinputs branch from e22fa33 to fabbf86 Compare April 25, 2025 18:20
@dunxen dunxen force-pushed the 2025-04-contributeinputs branch 4 times, most recently from c42f3d0 to 186d66b Compare May 12, 2025 10:51
@dunxen dunxen mentioned this pull request May 14, 2025
Merged
@dunxen dunxen force-pushed the 2025-04-contributeinputs branch from 186d66b to c7d8e89 Compare May 19, 2025 11:07
@dunxen dunxen force-pushed the 2025-04-contributeinputs branch from c7d8e89 to 540b8a7 Compare June 12, 2025 08:40
@wpaulino
Copy link
Contributor

Looks like this needs a rebase after #3637

@dunxen dunxen force-pushed the 2025-04-contributeinputs branch from 540b8a7 to 94bf848 Compare June 13, 2025 05:35
@dunxen
Copy link
Contributor Author

dunxen commented Jun 13, 2025

Looks like this needs a rebase after #3637

ugh right!

@wpaulino
Copy link
Contributor

Don't the first three commits need to be dropped?

@dunxen dunxen force-pushed the 2025-04-contributeinputs branch 2 times, most recently from 67504f0 to 9f276d8 Compare June 17, 2025 10:40
@dunxen
Copy link
Contributor Author

dunxen commented Jun 17, 2025

Just needs node reload tests but it can still get some initial review.

@dunxen dunxen force-pushed the 2025-04-contributeinputs branch from 9f276d8 to 60f58b1 Compare June 17, 2025 16:26
wpaulino
wpaulino reviewed Jun 18, 2025
View reviewed changes
lightning/src/ln/channelmanager.rs

fn length_limit_holder_input_prev_txs(
funding_inputs: Vec<(TxIn, Transaction, Weight)>,
) -> Result<Vec<(TxIn, TransactionU16LenLimited, Weight)>, APIError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's return a structured type here that also includes the corresponding TxOut as a field so we don't have to continue doing checks that it exists everywhere.

lightning/src/ln/channel.rs
@@ -11110,6 +11111,31 @@ where
script_pubkey: funding.get_funding_redeemscript().to_p2wsh(),
};

// Optionally add change output
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this already be covered by begin_interactive_funding_tx_construction?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh indeed. begin_interactive_funding_tx_construction was modified to include that in a previous pre-splicing work. Thanks.

lightning/src/ln/channel.rs
@@ -11110,6 +11111,31 @@ where
script_pubkey: funding.get_funding_redeemscript().to_p2wsh(),
};

// Optionally add change output
let change_script = signer_provider.get_destination_script(context.channel_keys_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe let the user optionally provide a change address along with their inputs?

@jkczyz jkczyz self-requested a review June 18, 2025 18:19
@wpaulino
Copy link
Contributor

Since the FundingTransactionReadyForSigning work is also relevant for splicing (though it will need some minor additional changes), maybe we should split it up from the inbound contribution work, so that we can unblock testing on the splicing side once #3736 lands.

@ldk-reviews-bot
Copy link

🔔 1st 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

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

@dunxen
Copy link
Contributor Author

dunxen commented Jun 24, 2025

Since the FundingTransactionReadyForSigning work is also relevant for splicing (though it will need some minor additional changes), maybe we should split it up from the inbound contribution work, so that we can unblock testing on the splicing side once #3736 lands.

Agreed. Apologies for the late response. Back home now. I'll split it into a separate PR and we can discuss changes.

@dunxen dunxen mentioned this pull request Jun 24, 2025
Open
@ldk-reviews-bot
Copy link

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

@dunxen dunxen force-pushed the 2025-04-contributeinputs branch from 60f58b1 to 9a2a5ce Compare June 26, 2025 08:30
dunxen added 3 commits June 27, 2025 09:18
@dunxen
Introduce FundingTransactionReadyForSignatures event
a9d653b 
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.
@dunxen
f misc fixups
d124aab 
* remove extra spaces * add failure and persistence section * remove
mentions of dual-funding methods * remove unnecessary
into_iter().collect() in provide_holder_witnesses()
@dunxen
f use bool for monitor_pending_tx_signatures
62c52d9
optout21 and others added 6 commits June 27, 2025 09:18
@optout21 @dunxen
Refactor shared output support (interactivetxs)
ad1078d
@optout21 @dunxen
Add Shared Input support
b6363fd
@optout21 @dunxen
Fix tx-add-input serialization
f3073ce
@dunxen
Add dual-funding docs for `FundingTransactionReadyForSignatures::user…
00ea143 
…_channel_id`
@dunxen
Allow passing funding inputs when manually accepting a dual-funded ch…
dbde4ee 
…annel

We introduce a `ChannelManager::accept_inbound_channel_with_contribution`
method allowing contributing to the overall channel capacity of an inbound
dual-funded channel by contributing inputs.
@dunxen
wip acceptor contributes in tests
8006303
@dunxen dunxen force-pushed the 2025-04-contributeinputs branch from 9a2a5ce to 8006303 Compare June 27, 2025 07:19
@codecov Codecov
Copy link

codecov bot commented Jun 27, 2025

Codecov Report

Attention: Patch coverage is 86.43068% with 138 lines in your changes missing coverage. Please review.

Project coverage is 88.96%. Comparing base (f66d568) to head (8006303).

Files with missing lines Patch % Lines
lightning/src/ln/interactivetxs.rs 92.54% 40 Missing and 8 partials ⚠️
lightning/src/ln/channelmanager.rs 58.53% 30 Missing and 4 partials ⚠️
lightning/src/ln/msgs.rs 47.36% 22 Missing and 8 partials ⚠️
lightning/src/ln/channel.rs 78.35% 16 Missing and 5 partials ⚠️
lightning/src/ln/dual_funding_tests.rs 97.03% 3 Missing and 1 partial ⚠️
lightning/src/events/mod.rs 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@           Coverage Diff            @@
##             main    #3735    +/-   ##
========================================
  Coverage   88.96%   88.96%            
========================================
  Files         164      164            
  Lines      119033   119542   +509     
  Branches   119033   119542   +509     
========================================
+ Hits       105892   106353   +461     
- Misses      10822    10856    +34     
- Partials     2319     2333    +14

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wpaulino wpaulino wpaulino left review comments

@jkczyz jkczyz Awaiting requested review from jkczyz

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
Dual-funding
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dunxen @ldk-reviews-bot @wpaulino @optout21