Skip to content

Gate interactive commitment_signed on user approval during reestablish#4629

Merged
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom
wpaulino:require-user-approval-reestablish-interactive-funding
May 22, 2026
Merged

Gate interactive commitment_signed on user approval during reestablish#4629
TheBlueMatt merged 1 commit into
lightningdevkit:mainfrom
wpaulino:require-user-approval-reestablish-interactive-funding

Conversation

@wpaulino
Copy link
Copy Markdown
Contributor

Interactive funding transactions must be approved by the user via ChannelManager::funding_transaction_signed prior to exchanging signatures for it. This ensures the user is able to cancel up until the very last point throughout the handshake. When this was done in 83b2d3e, we forgot the cover the reestablish cases, which we do here.

@wpaulino wpaulino requested a review from jkczyz May 20, 2026 23:14
@ldk-reviews-bot
Copy link
Copy Markdown

ldk-reviews-bot commented May 20, 2026

👋 Thanks for assigning @jkczyz as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@wpaulino wpaulino self-assigned this May 20, 2026
@wpaulino wpaulino added this to the 0.3 milestone May 20, 2026
@ldk-claude-review-bot
Copy link
Copy Markdown
Collaborator

ldk-claude-review-bot commented May 20, 2026

I've thoroughly reviewed the entire PR diff again, cross-referencing the implementations of has_holder_witnesses(), holder_tx_signatures(), the funding_transaction_signed flow, and the reconnection test helper.

My prior review already covered all the important analysis points and concluded with no issues found. After this deeper dive, I confirm that assessment:

  • The has_holder_witnesses() gate correctly prevents both retransmit_funding_commit_sig and tx_signatures from being set before user approval.
  • holder_tx_signatures() already required has_holder_witnesses() internally, so tx_signatures was effectively already gated — the new code just makes the retransmit_funding_commit_sig path consistent.
  • The recovery path via funding_transaction_signed → generate commitment_signed → queue message works correctly.
  • The test properly validates the scenario end-to-end.

No new issues found beyond my prior review.

No issues found.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 21, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.65%. Comparing base (9ce02b3) to head (fad7505).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4629      +/-   ##
==========================================
+ Coverage   86.64%   86.65%   +0.01%     
==========================================
  Files         159      159              
  Lines      110568   110569       +1     
  Branches   110568   110569       +1     
==========================================
+ Hits        95797    95817      +20     
+ Misses      12240    12224      -16     
+ Partials     2531     2528       -3     
Flag Coverage Δ
fuzzing-fake-hashes 6.77% <0.00%> (-0.01%) ⬇️
fuzzing-real-hashes 23.28% <0.00%> (-0.01%) ⬇️
tests 86.24% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.

Copy link
Copy Markdown
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.

LGTM but needs rebase.

@ldk-reviews-bot
Copy link
Copy Markdown

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

Interactive funding transactions must be approved by the user via
`ChannelManager::funding_transaction_signed` prior to exchanging
signatures for it. This ensures the user is able to cancel up until the
very last point throughout the handshake. When this was done in 83b2d3e,
we forgot the cover the reestablish cases, which we do here.
@wpaulino wpaulino force-pushed the require-user-approval-reestablish-interactive-funding branch from ac3ce3d to fad7505 Compare May 21, 2026 17:58
@TheBlueMatt TheBlueMatt merged commit 4773cb8 into lightningdevkit:main May 22, 2026
24 checks passed
@wpaulino wpaulino deleted the require-user-approval-reestablish-interactive-funding branch May 22, 2026 20:02
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.

5 participants