Skip to content
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

Dual funding extension, needed also for splicing: begin_interactive_funding_tx_construction #3443

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

optout21
Copy link
Contributor

@optout21 optout21 commented Dec 5, 2024

Add these two methods to the current dual funding implementation:
begin_interactive_funding_tx_construction()
calculate_change_output_value()

These changes are in the pipeline for dual funding implementation, and needed for dual funding negotiation during splicing, in #3444 .

@dunxen
Copy link
Contributor

dunxen commented Dec 5, 2024

Makes sense to get this this in so it can unblock you as #2302 will still take time. Is this much different from what is in #2302/any of the splicing PRs?

@jkczyz jkczyz self-requested a review December 5, 2024 15:22
@optout21
Copy link
Contributor Author

optout21 commented Dec 5, 2024

TODO: I need to run a comparison against current version of dual funding WIP

@optout21 optout21 force-pushed the splicing-dual-reqs2 branch 2 times, most recently from 4f7e41b to 0448cf9 Compare December 6, 2024 15:20
@optout21
Copy link
Contributor Author

optout21 commented Dec 6, 2024

Did the following:

  • break up maybe_add_funding_change_output() into two: one part for determining if a change needs to be added, and the other to actually add it
  • moved need_to_add_funding_change_output() into interactivetxs.rs
  • added unit tests for need_to_add_funding_change_output()

@optout21 optout21 changed the title [Draft] Dual funding extension needed for Splicing, begin_interactive_funding_tx_construction Dual funding extension needed for Splicing, begin_interactive_funding_tx_construction Dec 6, 2024
@optout21 optout21 marked this pull request as ready for review December 6, 2024 15:23
@optout21 optout21 changed the title Dual funding extension needed for Splicing, begin_interactive_funding_tx_construction Dual funding extension needed also for splicing: begin_interactive_funding_tx_construction Dec 6, 2024
@optout21 optout21 force-pushed the splicing-dual-reqs2 branch from 0448cf9 to 0e47819 Compare December 6, 2024 15:43
@optout21 optout21 requested a review from dunxen December 6, 2024 16:39
@optout21 optout21 force-pushed the splicing-dual-reqs2 branch from 48e0518 to 9e44d2b Compare December 6, 2024 16:41
@optout21 optout21 force-pushed the splicing-dual-reqs2 branch from 9e44d2b to b7e7a34 Compare December 9, 2024 07:20
@optout21 optout21 requested a review from dunxen December 9, 2024 07:22
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 update the commit message with the rationale for the change? (i.e., specifically how it will be used in splicing)

@optout21 optout21 force-pushed the splicing-dual-reqs2 branch from 70fde13 to b1ae668 Compare December 11, 2024 16:49
@optout21
Copy link
Contributor Author

Review comments addressed, all resolved from my side (left a few open for visibility)

@optout21 optout21 force-pushed the splicing-dual-reqs2 branch from b1ae668 to 86f9680 Compare December 11, 2024 16:56
@optout21 optout21 requested a review from jkczyz December 11, 2024 16:57
@optout21 optout21 force-pushed the splicing-dual-reqs2 branch from 86f9680 to a379f1d Compare December 11, 2024 19:37
Copy link

codecov bot commented Dec 11, 2024

Codecov Report

Attention: Patch coverage is 53.63636% with 102 lines in your changes missing coverage. Please review.

Project coverage is 89.16%. Comparing base (4c43a5b) to head (d1da349).

Files with missing lines Patch % Lines
lightning/src/ln/channel.rs 8.10% 102 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3443      +/-   ##
==========================================
- Coverage   89.18%   89.16%   -0.03%     
==========================================
  Files         155      155              
  Lines      119274   119486     +212     
  Branches   119274   119486     +212     
==========================================
+ Hits       106379   106543     +164     
- Misses      10290    10344      +54     
+ Partials     2605     2599       -6     

☔ 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 splicing-dual-reqs2 branch from a379f1d to 9256a47 Compare December 14, 2024 12:40
@optout21 optout21 force-pushed the splicing-dual-reqs2 branch from 9256a47 to 5194458 Compare January 6, 2025 09:57
@optout21
Copy link
Contributor Author

optout21 commented Jan 6, 2025

Rebased.

@optout21 optout21 removed the weekly goal Someone wants to land this this week label Jan 23, 2025
@optout21 optout21 force-pushed the splicing-dual-reqs2 branch from c186145 to 2488430 Compare February 9, 2025 19:44
@optout21 optout21 force-pushed the splicing-dual-reqs2 branch from 2488430 to 9644612 Compare February 21, 2025 16:54
@optout21
Copy link
Contributor Author

Rebased, including FundingScope change (#3592 )

@optout21 optout21 force-pushed the splicing-dual-reqs2 branch from e8d57fe to 9a9e511 Compare February 22, 2025 00:21
optout21 added a commit to optout21/rust-lightning that referenced this pull request Feb 28, 2025
optout21 added a commit to optout21/rust-lightning that referenced this pull request Feb 28, 2025
Copy link
Contributor

@dunxen dunxen left a comment

Choose a reason for hiding this comment

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

Needs rebase, and I think the fixups can be squashed. Will do another round of review then.

}

let mut funding_inputs_prev_outputs: Vec<&TxOut> = Vec::with_capacity(funding_inputs.len());
// Check that vouts exist for each TxIn in provided transactions.
Copy link
Contributor

Choose a reason for hiding this comment

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

That seems ok to me. I don't have a specific preference.

optout21 added a commit to optout21/rust-lightning that referenced this pull request Mar 4, 2025
@optout21 optout21 force-pushed the splicing-dual-reqs2 branch from 9a9e511 to c65c14e Compare March 4, 2025 23:46
@optout21
Copy link
Contributor Author

optout21 commented Mar 4, 2025

Rebased

@optout21 optout21 force-pushed the splicing-dual-reqs2 branch from c65c14e to c57393e Compare March 4, 2025 23:51
@jkczyz
Copy link
Contributor

jkczyz commented Mar 5, 2025

Let's squash the fixups.

@optout21 optout21 force-pushed the splicing-dual-reqs2 branch 3 times, most recently from 5aa36aa to 3afdfce Compare March 5, 2025 20:31
@optout21
Copy link
Contributor Author

optout21 commented Mar 5, 2025

Squashed into one commit, prepared for merge.

@optout21 optout21 requested a review from wpaulino March 5, 2025 20:32
@optout21 optout21 marked this pull request as draft March 7, 2025 15:22
@optout21 optout21 added the weekly goal Someone wants to land this this week label Mar 7, 2025
@optout21 optout21 force-pushed the splicing-dual-reqs2 branch from 3afdfce to e49751d Compare March 8, 2025 07:06
@optout21
Copy link
Contributor Author

optout21 commented Mar 8, 2025

Rebased, post-#3407

@optout21 optout21 force-pushed the splicing-dual-reqs2 branch 2 times, most recently from 15a61e0 to e6da872 Compare March 9, 2025 06:56
This method is needed by both V2 channel open and splicing.
Auxiliary changes:
- New method `calculate_change_output_value()` for determining if
  a change output is needed, and with what value.
- In `interactivetxs.rs` adjust the visibility of
  `SharedOwnedOutput` and `OutputOwned` structs (were not used before).
- In `DualFundingChannelContext` add a new field for the counterparty
  contribution, `their_funding_satoshis`.
@optout21 optout21 force-pushed the splicing-dual-reqs2 branch from e6da872 to d1da349 Compare March 9, 2025 07:15
@optout21 optout21 marked this pull request as ready for review March 9, 2025 16:40
@optout21 optout21 requested a review from dunxen March 9, 2025 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Splicing weekly goal Someone wants to land this this week
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants