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

Add unsafe_funding_transaction_generated #3056

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

Conversation

jbesraa
Copy link
Contributor

@jbesraa jbesraa commented May 9, 2024

..under ChannelManager. It replicates the functionality of the original funding_transaction_generated in the same module but without checking if the witness data is set in the funding transction inputs.

#3022

..under `ChannelManager`. It replicates the functionality of the
original `funding_transaction_generated` in the same module but without
checking if the witness data is set in the funding transction inputs.
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 96.87500% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 89.83%. Comparing base (38690bf) to head (2e18e65).

Files Patch % Lines
lightning/src/ln/functional_test_utils.rs 96.55% 1 Missing ⚠️
lightning/src/ln/functional_tests.rs 95.45% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3056      +/-   ##
==========================================
- Coverage   89.84%   89.83%   -0.01%     
==========================================
  Files         116      116              
  Lines       96303    96367      +64     
  Branches    96303    96367      +64     
==========================================
+ Hits        86521    86573      +52     
- Misses       7225     7232       +7     
- Partials     2557     2562       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jbesraa
Copy link
Contributor Author

jbesraa commented May 9, 2024

@TheBlueMatt while working on this I noticed the funding_transaction_generated would actually work with an empty array of inputs in the funding transaction.. is that by design?

/// channel (note that this should be trivially prevented by using unique funding transaction
/// keys per-channel).
///
/// Do NOT broadcast the funding transaction yourself. When we have safely received our
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oops, sorry for the confusion, my comment at #3024 (comment) wasn't really intended to be that we'd expect users to handle this crazy hacky indirection, but rather we could push the broadcast out via the Event described in that PR but just don't use a config knob and rather do it via a new channel funding method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the goal of this pr is to introduce an "unsafe_funding_tx_generated" that wouldnt check if the witness data is empty (https://github.com/lightningdevkit/rust-lightning/pull/3056/files?diff=split&w=0#diff-645d12c7269d09caab57d2e0c1fd34f672ee259994c86ad4bad7f4da9183671cR4682) but call batch_funding_transaction_generated_intern directly.
is it the only blocker for lightningdevkit/ldk-node#257.

Beside the top lines regarding unsafe, the docs are the same as the original funding_tx_generated.

I see how both PRs are related and introduce two separate functions(which is fine as is) but could be used in the same scenario(ie payjoin) but I would like to get this one merged first and unblock the ldk-node pr and complete the funding signed pr afterwards if that makes sense..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm, it seems a bit weird to introduce this here and then rewrite it one PR later to do something totally different? ISTM we could land #3024 without it taking toooo long.

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.

None yet

3 participants