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
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Contribute funding inputs on accepting dual-funded channel #3735

wants to merge 2 commits into from
+437 −105

Conversation

dunxen
Copy link
Contributor

@dunxen dunxen commented Apr 15, 2025 •
edited
Loading

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.

@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/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
Merged
@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 2 times, most recently from 9a2a5ce to 8006303 Compare June 27, 2025 07:19
@codecov Codecov
Copy link

codecov bot commented Jun 27, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 94.63602% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.82%. Comparing base (2772bfd) to head (c059e03).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
lightning/src/ln/dual_funding_tests.rs 96.89% 4 Missing and 1 partial ⚠️
lightning/src/ln/interactivetxs.rs 91.30% 4 Missing ⚠️
lightning/src/ln/channel.rs 90.00% 1 Missing and 2 partials ⚠️
lightning/src/ln/channelmanager.rs 90.90% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3735      +/-   ##
==========================================
+ Coverage   88.75%   88.82%   +0.07%     
==========================================
  Files         176      176              
  Lines      128817   129026     +209     
  Branches   128817   129026     +209     
==========================================
+ Hits       114327   114610     +283     
+ Misses      11893    11825      -68     
+ Partials     2597     2591       -6
Flag Coverage Δ
fuzzing 21.91% <8.33%> (-0.02%) ⬇️
tests 88.66% <94.63%> (+0.07%) ⬆️

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

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

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

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

@ldk-reviews-bot
Copy link

🔔 8th 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

🔔 9th 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

🔔 10th 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

🔔 11th 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 removed the request for review from jkczyz July 14, 2025 08:39
@dunxen dunxen force-pushed the 2025-04-contributeinputs branch from 8006303 to e17eedb Compare August 20, 2025 09:07
@dunxen dunxen force-pushed the 2025-04-contributeinputs branch 3 times, most recently from 35e1689 to 92ca1f1 Compare August 28, 2025 09:50
@dunxen dunxen marked this pull request as ready for review August 28, 2025 09:50
@dunxen
Allow passing funding inputs when manually accepting a dual-funded ch…
a93ea54 
…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.
@valentinewallace valentinewallace removed their request for review August 28, 2025 14:41
@dunxen
Test acceptor contributions in dual-funding functional tests
c059e03 
We can now run through the case where the acceptor contributes to an
inbound channel, with either more value in inputs, or less value, leading
to a different `tx_signatures` exchange order.

We also cannot use dummy P2WPKH funding inputs and witnesses anymore as
`funding_transaction_signed` internally verifies signatures. Hence, we
create external keypairs that we can create outputs for and sign with.
@dunxen dunxen force-pushed the 2025-04-contributeinputs branch from 92ca1f1 to c059e03 Compare August 28, 2025 15:52
@dunxen dunxen requested review from wpaulino and jkczyz August 29, 2025 09:31
@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.

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

@wpaulino wpaulino Awaiting requested review from wpaulino

@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.
3 participants
@dunxen @ldk-reviews-bot @wpaulino