Skip to content

Avoid early return upon confirmation of channel funding #2622

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

Conversation

wpaulino
Copy link
Contributor

@wpaulino wpaulino commented Sep 29, 2023

This early return is only possible if the channel requires a single confirmation, allowing a channel_ready message to go out. This can be problematic though if a commitment transaction (specifically from the counterparty, as the channel would be immediately closed if a local commitment is broadcast) also confirms within the same block. The
ChannelMonitor will detect both, but it won't inform the ChannelManager at all. Luckily, while the channel still is considered open to the ChannelManager, the ChannelMonitor will reject any further updates to the channel state.

@moneyball moneyball added this to the 0.0.117 milestone Sep 29, 2023
@codecov-commenter
Copy link

codecov-commenter commented Sep 29, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (6016101) 89.03% compared to head (92fcdd3) 88.99%.
Report is 14 commits behind head on main.

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2622      +/-   ##
==========================================
- Coverage   89.03%   88.99%   -0.05%     
==========================================
  Files         112      112              
  Lines       86351    86405      +54     
  Branches    86351    86405      +54     
==========================================
+ Hits        76879    76892      +13     
- Misses       7235     7274      +39     
- Partials     2237     2239       +2     
Files Coverage Δ
lightning/src/ln/channel.rs 88.33% <100.00%> (+<0.01%) ⬆️
lightning/src/ln/functional_tests.rs 97.23% <97.95%> (-0.12%) ⬇️

... and 11 files with indirect coverage changes

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

Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

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

LGTM, and test fails as expected. I think we generally prefer that each commit passes tests if you wanna reorder or squash these.

This early return is only possible if the channel requires a single
confirmation, allowing a `channel_ready` message to go out. This can be
problematic though if a commitment transaction (specifically from the
counterparty, as the channel would be immediately closed if a local
commitment is broadcast) also confirms within the same block. The
`ChannelMonitor` will detect both, but it won't inform the
`ChannelManager` at all. Luckily, while the channel still is considered
open to the `ChannelManager`, the `ChannelMonitor` will reject any
further updates to the channel state.
@wpaulino wpaulino force-pushed the funding-and-commitment-tx-confirm-same-block branch from 35b02cb to 92fcdd3 Compare September 29, 2023 18:46
@TheBlueMatt TheBlueMatt removed this from the 0.0.117 milestone Sep 29, 2023
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Not actually required for 0.0.117, but worth doing anyway.

@TheBlueMatt TheBlueMatt merged commit b58f057 into lightningdevkit:main Sep 29, 2023
@wpaulino wpaulino deleted the funding-and-commitment-tx-confirm-same-block branch September 29, 2023 21:08
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.

6 participants