Skip to content

Avoid redundant broadcast of local commitment transaction #1922

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

This change follows the rationale of commit 62236c7 and addresses the last remaining redundant local commitment broadcast.

There's no need to broadcast our local commitment transaction if we've already seen a confirmed one as it'll be immediately rejected as a duplicate/conflict.

This will also help prevent dispatching spurious events for bumping commitment and HTLC transactions through anchor outputs since the dispatch for said events follows the same flow as our usual commitment broadcast.

The bulk of this patch is addressing a series of test failures resulting from the broadcast logic change.

This change follows the rationale of commit 62236c7 and addresses the
last remaining redundant local commitment broadcast.

There's no need to broadcast our local commitment transaction if we've
already seen a confirmed one as it'll be immediately rejected as a
duplicate/conflict.

This will also help prevent dispatching spurious events for bumping
commitment and HTLC transactions through anchor outputs since the
dispatch for said events follows the same flow as our usual commitment
broadcast.
@codecov-commenter
Copy link

Codecov Report

Base: 90.73% // Head: 90.73% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (ff48f5d) compared to base (56afbf5).
Patch coverage: 91.13% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1922      +/-   ##
==========================================
- Coverage   90.73%   90.73%   -0.01%     
==========================================
  Files          94       94              
  Lines       49603    49538      -65     
  Branches    49603    49538      -65     
==========================================
- Hits        45006    44947      -59     
+ Misses       4597     4591       -6     
Impacted Files Coverage Δ
lightning/src/ln/functional_tests.rs 96.90% <89.23%> (-0.05%) ⬇️
lightning/src/chain/channelmonitor.rs 91.03% <100.00%> (+0.13%) ⬆️
lightning/src/ln/monitor_tests.rs 99.56% <100.00%> (-0.01%) ⬇️
lightning/src/ln/payment_tests.rs 98.73% <100.00%> (-0.01%) ⬇️
lightning/src/ln/reorg_tests.rs 100.00% <100.00%> (ø)
lightning/src/util/events.rs 29.58% <0.00%> (+0.22%) ⬆️
lightning/src/chain/onchaintx.rs 95.38% <0.00%> (+0.83%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@TheBlueMatt
Copy link
Collaborator

Only mostly related to this PR, but should we not also have a check like the one added here in provide_latest_holder_commitment_tx to refuse new updates after the monitor has seen a funding spend confirmed, whether that monitor has broadcasted a funding spend itself or not. Can/maybe should happen in a followup anyway.

Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

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

LGTM.

Nice! I just recently ran into the issue of duplicate commitment broadcasts when testing force closes in LDK Lite (see lightningdevkit/ldk-node#33 (comment)). Of course, this won't fix the issue there, as we still would try to broadcast when we don't see the commitment on chain, but still nice to have!

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.

4 participants