lightningdevkit / rust-lightning

A highly modular Bitcoin Lightning library written in Rust. It's rust-lightning, not Rusty's Lightning!
Other
1.16k stars 367 forks source link

[Splicing] Preserve `funding_transaction` for the later lifecycle of the channel #3317

Closed optout21 closed 2 weeks ago

optout21 commented 2 months ago

Fixes #3300

In splicing, the funding transaction (not just the ID) is needed during splice negotiation. Funding transaction is kept in field ChannelContext.funding_transaction. However, the way it is currently used is that it is cleared when the tx is broadcast, and this fact is used in some logic.

This change:

So behavior of not (re)broadcasting twice is preserve (though I am not sure whether this was intentional and it is needed, one test fails if this is changed (test_completed_payment_not_retryable_on_reload)).

Alternative would be to leave funding_transaction unchanged, and introduce a new funding_transaction_saved field, that is set the same way, but never cleared. This way existing logic would not be affected, but the tx would be kept in two copies.

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 90.88%. Comparing base (66fb520) to head (8339270). Report is 64 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #3317 +/- ## ========================================== + Coverage 89.66% 90.88% +1.22% ========================================== Files 126 127 +1 Lines 102676 115316 +12640 Branches 102676 115316 +12640 ========================================== + Hits 92062 104806 +12744 + Misses 7894 7869 -25 + Partials 2720 2641 -79 ``` | [Flag](https://app.codecov.io/gh/lightningdevkit/rust-lightning/pull/3317/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=lightningdevkit) | Coverage Δ | | |---|---|---| | [](https://app.codecov.io/gh/lightningdevkit/rust-lightning/pull/3317/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=lightningdevkit) | `?` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=lightningdevkit#carryforward-flags-in-the-pull-request-comment) to find out more.

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

optout21 commented 1 month ago

Note: Changing this behavior causes a single test case to fail: ln::payment_tests::test_completed_payment_not_retryable_on_reload (reproducible by not resetting funding_transaction in the original code, or keeping funding_transaction_rebroadcast_flag always false in the version of this PR)

The check that fails expects 1 broadcasted transaction, while with the change it will have the transaction broadcasted 4 times. I suspect this change does not really affect the desired behavior, this checks is simply over-specific and that can be updated, and the change is acceptable.

  assert_eq!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0).len(), 4);
optout21 commented 3 weeks ago

Alternative simpler solution in #3380 -- never clears the funding_transaction field, no new fields, which may result in additional rebroadcasts. One should be chosen -- I lean for the simpler solution, but I have no strong opinion.

TheBlueMatt commented 2 weeks ago

Supersceded by #3380.