Gate interactive commitment_signed on user approval during reestablish

[wpaulino]

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.

[ldk-reviews-bot]

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

[ldk-claude-review-bot]

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]

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.

[jkczyz]

LGTM but needs rebase.

[ldk-reviews-bot]

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

[wpaulino]

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.

[ldk-reviews-bot]

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

[ldk-claude-review-bot]

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]

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.

[jkczyz]

LGTM but needs rebase.

[ldk-reviews-bot]

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

[wpaulino]

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.

[ldk-reviews-bot]

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

[ldk-claude-review-bot]

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]

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.

[jkczyz]

LGTM but needs rebase.

[ldk-reviews-bot]

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

[wpaulino]

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.

[ldk-reviews-bot]

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

[ldk-claude-review-bot]

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]

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.

[jkczyz]

LGTM but needs rebase.

[ldk-reviews-bot]

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

[wpaulino]

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.

[ldk-reviews-bot]

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

[ldk-claude-review-bot]

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]

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.

[jkczyz]

LGTM but needs rebase.

[ldk-reviews-bot]

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

[wpaulino]

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.

[ldk-reviews-bot]

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

[ldk-claude-review-bot]

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]

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.

[jkczyz]

LGTM but needs rebase.

[ldk-reviews-bot]

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

[wpaulino]

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.

[ldk-reviews-bot]

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

[ldk-claude-review-bot]

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]

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.

[jkczyz]

LGTM but needs rebase.

[ldk-reviews-bot]

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

[wpaulino]

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.

[ldk-reviews-bot]

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

[ldk-claude-review-bot]

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]

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.

[jkczyz]

LGTM but needs rebase.

[ldk-reviews-bot]

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