Free holding cell in remaining quiescence-exit code paths by wpaulino · Pull Request #4415 · lightningdevkit/rust-lightning

wpaulino · 2026-02-12T19:26:32Z

Depends on #4412.

According to the spec, handling a splice_init and splice_ack may return tx_abort to reject the splice, but we always send a warning and disconnect instead, so those are not included here.

ldk-reviews-bot · 2026-02-12T19:26:35Z

👋 Thanks for assigning @TheBlueMatt 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.

codecov · 2026-02-12T21:26:04Z

Codecov Report

❌ Patch coverage is 71.09635% with 87 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.90%. Comparing base (0a20fa5) to head (1b8617c).
⚠️ Report is 21 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	75.98%	56 Missing and 5 partials ⚠️
lightning/src/ln/channel.rs	44.68%	26 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4415 +/- ##
==========================================
- Coverage 85.91% 85.90% -0.02% 
==========================================
Files 156 156 Lines 103963 103990 +27 Branches 103963 103990 +27 ==========================================
+ Hits 89322 89333 +11 - Misses 12119 12135 +16 
Partials 2522 2522

Flag	Coverage Δ
tests	`85.90% <71.09%> (-0.02%)`	⬇️

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.

TheBlueMatt

lgtm

The merge-base changed after approval.

ldk-reviews-bot · 2026-02-12T22:53:52Z

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

TheBlueMatt · 2026-02-12T22:54:15Z

note that github wont let this be merged until its rebased. Also not sure if @jkczyz wants to take a look or not.

wpaulino · 2026-02-12T23:28:12Z

Rebased and pushed fixups for #4412

jkczyz

note that github wont let this be merged until its rebased. Also not sure if @jkczyz wants to take a look or not.

Skimmed though it. Just one comment on the approach.

jkczyz

Looks like fail_splice_on_tx_complete_error is failing in CI.

wpaulino · 2026-02-13T17:46:44Z

Had to rebase due to an import conflict in channelmanager.rs

ldk-reviews-bot · 2026-02-16T00:00:58Z

🔔 1st Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

After cad88af, a few code paths that also lead to a quiescence exit were not accounted for. This commit addresses the path where we exit quiescence due to a processing error on a counterparty's `tx_add_input/output`, `tx_remove_input/output`, or `tx_complete` message.

After cad88af, a few code paths that also lead to a quiescence exit were not accounted for. This commit addresses the path where we exit quiescence due to processing a counterparty's `tx_abort` message.

After cad88af, a few code paths that also lead to a quiescence exit were not accounted for. This commit addresses the last remaining path where we exit quiescence when we exchange `tx_signatures` with the counterparty.

wpaulino · 2026-02-17T23:36:17Z

Had to rebase again after #4290

ldk-reviews-bot · 2026-02-18T00:01:24Z

🔔 2nd Reminder

Hey @TheBlueMatt @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

jkczyz · 2026-02-18T16:23:53Z

+ debug_assert!(res.as_ref().err().map_or(true, |err| !err.closes_channel()));
 let _ = self.handle_error(res, counterparty_node_id);
-persist
+self.event_persist_notifier.notify();


Hmm... why do we need to manually notify? Shouldn't dropping the PersistenceNotifierGuard cause notification if handle_error (via handle_holding_cell_free_result) sets the persistence flag (and only if it does)? Instead of manually_notify, I was thinking something like notify_if_needed.

handle_holding_cell_free_result is not guaranteed to happen so we still need to notify the event handling when we have a response to the message

Ah, though it looks like we unconditionally notify now whereas before we may have not (e.g., upon exchanging tx_complete when no signatures are needed from the user). Are we fine with that?

If tx_complete is exchanged then we're already persisting because we have a new signing session

These errors will only ever affect our in-memory state, so there's no need to persist the ChannelManager when we come across one. Note that `tx_abort` is not included here because there is a possibility we force close the channel, which we should persist.

wpaulino · 2026-02-12T19:26:32Z

Depends on #4412.

According to the spec, handling a splice_init and splice_ack may return tx_abort to reject the splice, but we always send a warning and disconnect instead, so those are not included here.

ldk-reviews-bot · 2026-02-12T19:26:35Z

👋 Thanks for assigning @TheBlueMatt 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.

codecov · 2026-02-12T21:26:04Z

Codecov Report

❌ Patch coverage is 71.09635% with 87 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.90%. Comparing base (0a20fa5) to head (1b8617c).
⚠️ Report is 21 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	75.98%	56 Missing and 5 partials ⚠️
lightning/src/ln/channel.rs	44.68%	26 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4415 +/- ##
==========================================
- Coverage 85.91% 85.90% -0.02% 
==========================================
Files 156 156 Lines 103963 103990 +27 Branches 103963 103990 +27 ==========================================
+ Hits 89322 89333 +11 - Misses 12119 12135 +16 
Partials 2522 2522

Flag	Coverage Δ
tests	`85.90% <71.09%> (-0.02%)`	⬇️

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.

TheBlueMatt

lgtm

The merge-base changed after approval.

ldk-reviews-bot · 2026-02-12T22:53:52Z

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

TheBlueMatt · 2026-02-12T22:54:15Z

note that github wont let this be merged until its rebased. Also not sure if @jkczyz wants to take a look or not.

wpaulino · 2026-02-12T23:28:12Z

Rebased and pushed fixups for #4412

jkczyz

note that github wont let this be merged until its rebased. Also not sure if @jkczyz wants to take a look or not.

Skimmed though it. Just one comment on the approach.

jkczyz

Looks like fail_splice_on_tx_complete_error is failing in CI.

wpaulino · 2026-02-13T17:46:44Z

Had to rebase due to an import conflict in channelmanager.rs

ldk-reviews-bot · 2026-02-16T00:00:58Z

🔔 1st Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

After cad88af, a few code paths that also lead to a quiescence exit were not accounted for. This commit addresses the path where we exit quiescence due to a processing error on a counterparty's `tx_add_input/output`, `tx_remove_input/output`, or `tx_complete` message.

After cad88af, a few code paths that also lead to a quiescence exit were not accounted for. This commit addresses the path where we exit quiescence due to processing a counterparty's `tx_abort` message.

After cad88af, a few code paths that also lead to a quiescence exit were not accounted for. This commit addresses the last remaining path where we exit quiescence when we exchange `tx_signatures` with the counterparty.

wpaulino · 2026-02-17T23:36:17Z

Had to rebase again after #4290

ldk-reviews-bot · 2026-02-18T00:01:24Z

🔔 2nd Reminder

Hey @TheBlueMatt @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

jkczyz · 2026-02-18T16:23:53Z

+ debug_assert!(res.as_ref().err().map_or(true, |err| !err.closes_channel()));
 let _ = self.handle_error(res, counterparty_node_id);
-persist
+self.event_persist_notifier.notify();


Hmm... why do we need to manually notify? Shouldn't dropping the PersistenceNotifierGuard cause notification if handle_error (via handle_holding_cell_free_result) sets the persistence flag (and only if it does)? Instead of manually_notify, I was thinking something like notify_if_needed.

handle_holding_cell_free_result is not guaranteed to happen so we still need to notify the event handling when we have a response to the message

Ah, though it looks like we unconditionally notify now whereas before we may have not (e.g., upon exchanging tx_complete when no signatures are needed from the user). Are we fine with that?

If tx_complete is exchanged then we're already persisting because we have a new signing session

These errors will only ever affect our in-memory state, so there's no need to persist the ChannelManager when we come across one. Note that `tx_abort` is not included here because there is a possibility we force close the channel, which we should persist.

wpaulino · 2026-02-12T19:26:32Z

Depends on #4412.

According to the spec, handling a splice_init and splice_ack may return tx_abort to reject the splice, but we always send a warning and disconnect instead, so those are not included here.

ldk-reviews-bot · 2026-02-12T19:26:35Z

👋 Thanks for assigning @TheBlueMatt 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.

codecov · 2026-02-12T21:26:04Z

Codecov Report

❌ Patch coverage is 71.09635% with 87 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.90%. Comparing base (0a20fa5) to head (1b8617c).
⚠️ Report is 21 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	75.98%	56 Missing and 5 partials ⚠️
lightning/src/ln/channel.rs	44.68%	26 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4415 +/- ##
==========================================
- Coverage 85.91% 85.90% -0.02% 
==========================================
Files 156 156 Lines 103963 103990 +27 Branches 103963 103990 +27 ==========================================
+ Hits 89322 89333 +11 - Misses 12119 12135 +16 
Partials 2522 2522

Flag	Coverage Δ
tests	`85.90% <71.09%> (-0.02%)`	⬇️

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.

TheBlueMatt

lgtm

The merge-base changed after approval.

ldk-reviews-bot · 2026-02-12T22:53:52Z

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

TheBlueMatt · 2026-02-12T22:54:15Z

note that github wont let this be merged until its rebased. Also not sure if @jkczyz wants to take a look or not.

wpaulino · 2026-02-12T23:28:12Z

Rebased and pushed fixups for #4412

jkczyz

note that github wont let this be merged until its rebased. Also not sure if @jkczyz wants to take a look or not.

Skimmed though it. Just one comment on the approach.

jkczyz

Looks like fail_splice_on_tx_complete_error is failing in CI.

wpaulino · 2026-02-13T17:46:44Z

Had to rebase due to an import conflict in channelmanager.rs

ldk-reviews-bot · 2026-02-16T00:00:58Z

🔔 1st Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

After cad88af, a few code paths that also lead to a quiescence exit were not accounted for. This commit addresses the path where we exit quiescence due to a processing error on a counterparty's `tx_add_input/output`, `tx_remove_input/output`, or `tx_complete` message.

After cad88af, a few code paths that also lead to a quiescence exit were not accounted for. This commit addresses the path where we exit quiescence due to processing a counterparty's `tx_abort` message.

After cad88af, a few code paths that also lead to a quiescence exit were not accounted for. This commit addresses the last remaining path where we exit quiescence when we exchange `tx_signatures` with the counterparty.

wpaulino · 2026-02-17T23:36:17Z

Had to rebase again after #4290

ldk-reviews-bot · 2026-02-18T00:01:24Z

🔔 2nd Reminder

Hey @TheBlueMatt @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

jkczyz · 2026-02-18T16:23:53Z

+ debug_assert!(res.as_ref().err().map_or(true, |err| !err.closes_channel()));
 let _ = self.handle_error(res, counterparty_node_id);
-persist
+self.event_persist_notifier.notify();


Hmm... why do we need to manually notify? Shouldn't dropping the PersistenceNotifierGuard cause notification if handle_error (via handle_holding_cell_free_result) sets the persistence flag (and only if it does)? Instead of manually_notify, I was thinking something like notify_if_needed.

handle_holding_cell_free_result is not guaranteed to happen so we still need to notify the event handling when we have a response to the message

Ah, though it looks like we unconditionally notify now whereas before we may have not (e.g., upon exchanging tx_complete when no signatures are needed from the user). Are we fine with that?

If tx_complete is exchanged then we're already persisting because we have a new signing session

These errors will only ever affect our in-memory state, so there's no need to persist the ChannelManager when we come across one. Note that `tx_abort` is not included here because there is a possibility we force close the channel, which we should persist.

wpaulino · 2026-02-12T19:26:32Z

Depends on #4412.

According to the spec, handling a splice_init and splice_ack may return tx_abort to reject the splice, but we always send a warning and disconnect instead, so those are not included here.

ldk-reviews-bot · 2026-02-12T19:26:35Z

👋 Thanks for assigning @TheBlueMatt 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.

codecov · 2026-02-12T21:26:04Z

Codecov Report

❌ Patch coverage is 71.09635% with 87 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.90%. Comparing base (0a20fa5) to head (1b8617c).
⚠️ Report is 21 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	75.98%	56 Missing and 5 partials ⚠️
lightning/src/ln/channel.rs	44.68%	26 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4415 +/- ##
==========================================
- Coverage 85.91% 85.90% -0.02% 
==========================================
Files 156 156 Lines 103963 103990 +27 Branches 103963 103990 +27 ==========================================
+ Hits 89322 89333 +11 - Misses 12119 12135 +16 
Partials 2522 2522

Flag	Coverage Δ
tests	`85.90% <71.09%> (-0.02%)`	⬇️

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.

TheBlueMatt

lgtm

The merge-base changed after approval.

ldk-reviews-bot · 2026-02-12T22:53:52Z

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

TheBlueMatt · 2026-02-12T22:54:15Z

note that github wont let this be merged until its rebased. Also not sure if @jkczyz wants to take a look or not.

wpaulino · 2026-02-12T23:28:12Z

Rebased and pushed fixups for #4412

jkczyz

note that github wont let this be merged until its rebased. Also not sure if @jkczyz wants to take a look or not.

Skimmed though it. Just one comment on the approach.

jkczyz

Looks like fail_splice_on_tx_complete_error is failing in CI.

wpaulino · 2026-02-13T17:46:44Z

Had to rebase due to an import conflict in channelmanager.rs

ldk-reviews-bot · 2026-02-16T00:00:58Z

🔔 1st Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

After cad88af, a few code paths that also lead to a quiescence exit were not accounted for. This commit addresses the path where we exit quiescence due to a processing error on a counterparty's `tx_add_input/output`, `tx_remove_input/output`, or `tx_complete` message.

After cad88af, a few code paths that also lead to a quiescence exit were not accounted for. This commit addresses the path where we exit quiescence due to processing a counterparty's `tx_abort` message.

After cad88af, a few code paths that also lead to a quiescence exit were not accounted for. This commit addresses the last remaining path where we exit quiescence when we exchange `tx_signatures` with the counterparty.

wpaulino · 2026-02-17T23:36:17Z

Had to rebase again after #4290

ldk-reviews-bot · 2026-02-18T00:01:24Z

🔔 2nd Reminder

Hey @TheBlueMatt @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

jkczyz · 2026-02-18T16:23:53Z

+ debug_assert!(res.as_ref().err().map_or(true, |err| !err.closes_channel()));
 let _ = self.handle_error(res, counterparty_node_id);
-persist
+self.event_persist_notifier.notify();


Hmm... why do we need to manually notify? Shouldn't dropping the PersistenceNotifierGuard cause notification if handle_error (via handle_holding_cell_free_result) sets the persistence flag (and only if it does)? Instead of manually_notify, I was thinking something like notify_if_needed.

handle_holding_cell_free_result is not guaranteed to happen so we still need to notify the event handling when we have a response to the message

Ah, though it looks like we unconditionally notify now whereas before we may have not (e.g., upon exchanging tx_complete when no signatures are needed from the user). Are we fine with that?

If tx_complete is exchanged then we're already persisting because we have a new signing session

These errors will only ever affect our in-memory state, so there's no need to persist the ChannelManager when we come across one. Note that `tx_abort` is not included here because there is a possibility we force close the channel, which we should persist.

wpaulino · 2026-02-12T19:26:32Z

Depends on #4412.

According to the spec, handling a splice_init and splice_ack may return tx_abort to reject the splice, but we always send a warning and disconnect instead, so those are not included here.

ldk-reviews-bot · 2026-02-12T19:26:35Z

👋 Thanks for assigning @TheBlueMatt 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.

codecov · 2026-02-12T21:26:04Z

Codecov Report

❌ Patch coverage is 71.09635% with 87 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.90%. Comparing base (0a20fa5) to head (1b8617c).
⚠️ Report is 21 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	75.98%	56 Missing and 5 partials ⚠️
lightning/src/ln/channel.rs	44.68%	26 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4415 +/- ##
==========================================
- Coverage 85.91% 85.90% -0.02% 
==========================================
Files 156 156 Lines 103963 103990 +27 Branches 103963 103990 +27 ==========================================
+ Hits 89322 89333 +11 - Misses 12119 12135 +16 
Partials 2522 2522

Flag	Coverage Δ
tests	`85.90% <71.09%> (-0.02%)`	⬇️

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.

TheBlueMatt

lgtm

The merge-base changed after approval.

ldk-reviews-bot · 2026-02-12T22:53:52Z

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

TheBlueMatt · 2026-02-12T22:54:15Z

note that github wont let this be merged until its rebased. Also not sure if @jkczyz wants to take a look or not.

wpaulino · 2026-02-12T23:28:12Z

Rebased and pushed fixups for #4412

jkczyz

note that github wont let this be merged until its rebased. Also not sure if @jkczyz wants to take a look or not.

Skimmed though it. Just one comment on the approach.

jkczyz

Looks like fail_splice_on_tx_complete_error is failing in CI.

wpaulino · 2026-02-13T17:46:44Z

Had to rebase due to an import conflict in channelmanager.rs

ldk-reviews-bot · 2026-02-16T00:00:58Z

🔔 1st Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

After cad88af, a few code paths that also lead to a quiescence exit were not accounted for. This commit addresses the path where we exit quiescence due to a processing error on a counterparty's `tx_add_input/output`, `tx_remove_input/output`, or `tx_complete` message.

After cad88af, a few code paths that also lead to a quiescence exit were not accounted for. This commit addresses the path where we exit quiescence due to processing a counterparty's `tx_abort` message.

After cad88af, a few code paths that also lead to a quiescence exit were not accounted for. This commit addresses the last remaining path where we exit quiescence when we exchange `tx_signatures` with the counterparty.

wpaulino · 2026-02-17T23:36:17Z

Had to rebase again after #4290

ldk-reviews-bot · 2026-02-18T00:01:24Z

🔔 2nd Reminder

Hey @TheBlueMatt @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

jkczyz · 2026-02-18T16:23:53Z

+ debug_assert!(res.as_ref().err().map_or(true, |err| !err.closes_channel()));
 let _ = self.handle_error(res, counterparty_node_id);
-persist
+self.event_persist_notifier.notify();


Hmm... why do we need to manually notify? Shouldn't dropping the PersistenceNotifierGuard cause notification if handle_error (via handle_holding_cell_free_result) sets the persistence flag (and only if it does)? Instead of manually_notify, I was thinking something like notify_if_needed.

handle_holding_cell_free_result is not guaranteed to happen so we still need to notify the event handling when we have a response to the message

Ah, though it looks like we unconditionally notify now whereas before we may have not (e.g., upon exchanging tx_complete when no signatures are needed from the user). Are we fine with that?

If tx_complete is exchanged then we're already persisting because we have a new signing session

These errors will only ever affect our in-memory state, so there's no need to persist the ChannelManager when we come across one. Note that `tx_abort` is not included here because there is a possibility we force close the channel, which we should persist.

wpaulino · 2026-02-12T19:26:32Z

Depends on #4412.

According to the spec, handling a splice_init and splice_ack may return tx_abort to reject the splice, but we always send a warning and disconnect instead, so those are not included here.

ldk-reviews-bot · 2026-02-12T19:26:35Z

👋 Thanks for assigning @TheBlueMatt 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.

codecov · 2026-02-12T21:26:04Z

Codecov Report

❌ Patch coverage is 71.09635% with 87 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.90%. Comparing base (0a20fa5) to head (1b8617c).
⚠️ Report is 21 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	75.98%	56 Missing and 5 partials ⚠️
lightning/src/ln/channel.rs	44.68%	26 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4415 +/- ##
==========================================
- Coverage 85.91% 85.90% -0.02% 
==========================================
Files 156 156 Lines 103963 103990 +27 Branches 103963 103990 +27 ==========================================
+ Hits 89322 89333 +11 - Misses 12119 12135 +16 
Partials 2522 2522

Flag	Coverage Δ
tests	`85.90% <71.09%> (-0.02%)`	⬇️

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.

TheBlueMatt

lgtm

The merge-base changed after approval.

ldk-reviews-bot · 2026-02-12T22:53:52Z

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

TheBlueMatt · 2026-02-12T22:54:15Z

note that github wont let this be merged until its rebased. Also not sure if @jkczyz wants to take a look or not.

wpaulino · 2026-02-12T23:28:12Z

Rebased and pushed fixups for #4412

jkczyz

note that github wont let this be merged until its rebased. Also not sure if @jkczyz wants to take a look or not.

Skimmed though it. Just one comment on the approach.

jkczyz

Looks like fail_splice_on_tx_complete_error is failing in CI.

wpaulino · 2026-02-13T17:46:44Z

Had to rebase due to an import conflict in channelmanager.rs

ldk-reviews-bot · 2026-02-16T00:00:58Z

🔔 1st Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

After cad88af, a few code paths that also lead to a quiescence exit were not accounted for. This commit addresses the path where we exit quiescence due to a processing error on a counterparty's `tx_add_input/output`, `tx_remove_input/output`, or `tx_complete` message.

After cad88af, a few code paths that also lead to a quiescence exit were not accounted for. This commit addresses the path where we exit quiescence due to processing a counterparty's `tx_abort` message.

After cad88af, a few code paths that also lead to a quiescence exit were not accounted for. This commit addresses the last remaining path where we exit quiescence when we exchange `tx_signatures` with the counterparty.

wpaulino · 2026-02-17T23:36:17Z

Had to rebase again after #4290

ldk-reviews-bot · 2026-02-18T00:01:24Z

🔔 2nd Reminder

Hey @TheBlueMatt @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

jkczyz · 2026-02-18T16:23:53Z

+ debug_assert!(res.as_ref().err().map_or(true, |err| !err.closes_channel()));
 let _ = self.handle_error(res, counterparty_node_id);
-persist
+self.event_persist_notifier.notify();


Hmm... why do we need to manually notify? Shouldn't dropping the PersistenceNotifierGuard cause notification if handle_error (via handle_holding_cell_free_result) sets the persistence flag (and only if it does)? Instead of manually_notify, I was thinking something like notify_if_needed.

handle_holding_cell_free_result is not guaranteed to happen so we still need to notify the event handling when we have a response to the message

Ah, though it looks like we unconditionally notify now whereas before we may have not (e.g., upon exchanging tx_complete when no signatures are needed from the user). Are we fine with that?

If tx_complete is exchanged then we're already persisting because we have a new signing session

These errors will only ever affect our in-memory state, so there's no need to persist the ChannelManager when we come across one. Note that `tx_abort` is not included here because there is a possibility we force close the channel, which we should persist.

wpaulino · 2026-02-12T19:26:32Z

Depends on #4412.

According to the spec, handling a splice_init and splice_ack may return tx_abort to reject the splice, but we always send a warning and disconnect instead, so those are not included here.

ldk-reviews-bot · 2026-02-12T19:26:35Z

👋 Thanks for assigning @TheBlueMatt 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.

codecov · 2026-02-12T21:26:04Z

Codecov Report

❌ Patch coverage is 71.09635% with 87 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.90%. Comparing base (0a20fa5) to head (1b8617c).
⚠️ Report is 21 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	75.98%	56 Missing and 5 partials ⚠️
lightning/src/ln/channel.rs	44.68%	26 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4415 +/- ##
==========================================
- Coverage 85.91% 85.90% -0.02% 
==========================================
Files 156 156 Lines 103963 103990 +27 Branches 103963 103990 +27 ==========================================
+ Hits 89322 89333 +11 - Misses 12119 12135 +16 
Partials 2522 2522

Flag	Coverage Δ
tests	`85.90% <71.09%> (-0.02%)`	⬇️

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.

TheBlueMatt

lgtm

The merge-base changed after approval.

ldk-reviews-bot · 2026-02-12T22:53:52Z

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

TheBlueMatt · 2026-02-12T22:54:15Z

note that github wont let this be merged until its rebased. Also not sure if @jkczyz wants to take a look or not.

wpaulino · 2026-02-12T23:28:12Z

Rebased and pushed fixups for #4412

jkczyz

note that github wont let this be merged until its rebased. Also not sure if @jkczyz wants to take a look or not.

Skimmed though it. Just one comment on the approach.

jkczyz

Looks like fail_splice_on_tx_complete_error is failing in CI.

wpaulino · 2026-02-13T17:46:44Z

Had to rebase due to an import conflict in channelmanager.rs

ldk-reviews-bot · 2026-02-16T00:00:58Z

🔔 1st Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

After cad88af, a few code paths that also lead to a quiescence exit were not accounted for. This commit addresses the path where we exit quiescence due to a processing error on a counterparty's `tx_add_input/output`, `tx_remove_input/output`, or `tx_complete` message.

After cad88af, a few code paths that also lead to a quiescence exit were not accounted for. This commit addresses the path where we exit quiescence due to processing a counterparty's `tx_abort` message.

After cad88af, a few code paths that also lead to a quiescence exit were not accounted for. This commit addresses the last remaining path where we exit quiescence when we exchange `tx_signatures` with the counterparty.

wpaulino · 2026-02-17T23:36:17Z

Had to rebase again after #4290

ldk-reviews-bot · 2026-02-18T00:01:24Z

🔔 2nd Reminder

Hey @TheBlueMatt @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

jkczyz · 2026-02-18T16:23:53Z

+ debug_assert!(res.as_ref().err().map_or(true, |err| !err.closes_channel()));
 let _ = self.handle_error(res, counterparty_node_id);
-persist
+self.event_persist_notifier.notify();


Hmm... why do we need to manually notify? Shouldn't dropping the PersistenceNotifierGuard cause notification if handle_error (via handle_holding_cell_free_result) sets the persistence flag (and only if it does)? Instead of manually_notify, I was thinking something like notify_if_needed.

handle_holding_cell_free_result is not guaranteed to happen so we still need to notify the event handling when we have a response to the message

Ah, though it looks like we unconditionally notify now whereas before we may have not (e.g., upon exchanging tx_complete when no signatures are needed from the user). Are we fine with that?

If tx_complete is exchanged then we're already persisting because we have a new signing session

These errors will only ever affect our in-memory state, so there's no need to persist the ChannelManager when we come across one. Note that `tx_abort` is not included here because there is a possibility we force close the channel, which we should persist.

wpaulino · 2026-02-12T19:26:32Z

Depends on #4412.

According to the spec, handling a splice_init and splice_ack may return tx_abort to reject the splice, but we always send a warning and disconnect instead, so those are not included here.

ldk-reviews-bot · 2026-02-12T19:26:35Z

👋 Thanks for assigning @TheBlueMatt 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.

codecov · 2026-02-12T21:26:04Z

Codecov Report

❌ Patch coverage is 71.09635% with 87 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.90%. Comparing base (0a20fa5) to head (1b8617c).
⚠️ Report is 21 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	75.98%	56 Missing and 5 partials ⚠️
lightning/src/ln/channel.rs	44.68%	26 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4415 +/- ##
==========================================
- Coverage 85.91% 85.90% -0.02% 
==========================================
Files 156 156 Lines 103963 103990 +27 Branches 103963 103990 +27 ==========================================
+ Hits 89322 89333 +11 - Misses 12119 12135 +16 
Partials 2522 2522

Flag	Coverage Δ
tests	`85.90% <71.09%> (-0.02%)`	⬇️

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.

TheBlueMatt

lgtm

The merge-base changed after approval.

ldk-reviews-bot · 2026-02-12T22:53:52Z

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

TheBlueMatt · 2026-02-12T22:54:15Z

note that github wont let this be merged until its rebased. Also not sure if @jkczyz wants to take a look or not.

wpaulino · 2026-02-12T23:28:12Z

Rebased and pushed fixups for #4412

jkczyz

note that github wont let this be merged until its rebased. Also not sure if @jkczyz wants to take a look or not.

Skimmed though it. Just one comment on the approach.

jkczyz

Looks like fail_splice_on_tx_complete_error is failing in CI.

wpaulino · 2026-02-13T17:46:44Z

Had to rebase due to an import conflict in channelmanager.rs

ldk-reviews-bot · 2026-02-16T00:00:58Z

🔔 1st Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

After cad88af, a few code paths that also lead to a quiescence exit were not accounted for. This commit addresses the path where we exit quiescence due to a processing error on a counterparty's `tx_add_input/output`, `tx_remove_input/output`, or `tx_complete` message.

After cad88af, a few code paths that also lead to a quiescence exit were not accounted for. This commit addresses the path where we exit quiescence due to processing a counterparty's `tx_abort` message.

After cad88af, a few code paths that also lead to a quiescence exit were not accounted for. This commit addresses the last remaining path where we exit quiescence when we exchange `tx_signatures` with the counterparty.

wpaulino · 2026-02-17T23:36:17Z

Had to rebase again after #4290

ldk-reviews-bot · 2026-02-18T00:01:24Z

🔔 2nd Reminder

Hey @TheBlueMatt @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

jkczyz · 2026-02-18T16:23:53Z

+ debug_assert!(res.as_ref().err().map_or(true, |err| !err.closes_channel()));
 let _ = self.handle_error(res, counterparty_node_id);
-persist
+self.event_persist_notifier.notify();


Hmm... why do we need to manually notify? Shouldn't dropping the PersistenceNotifierGuard cause notification if handle_error (via handle_holding_cell_free_result) sets the persistence flag (and only if it does)? Instead of manually_notify, I was thinking something like notify_if_needed.

handle_holding_cell_free_result is not guaranteed to happen so we still need to notify the event handling when we have a response to the message

Ah, though it looks like we unconditionally notify now whereas before we may have not (e.g., upon exchanging tx_complete when no signatures are needed from the user). Are we fine with that?

If tx_complete is exchanged then we're already persisting because we have a new signing session

These errors will only ever affect our in-memory state, so there's no need to persist the ChannelManager when we come across one. Note that `tx_abort` is not included here because there is a possibility we force close the channel, which we should persist.

wpaulino added this to the 0.3 milestone Feb 12, 2026

wpaulino requested a review from TheBlueMatt February 12, 2026 19:26

wpaulino self-assigned this Feb 12, 2026

wpaulino added this to Weekly Goals Feb 12, 2026

TheBlueMatt previously approved these changes Feb 12, 2026
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated

TheBlueMatt requested a review from jkczyz February 12, 2026 22:54

wpaulino force-pushed the free-holding-cells-quiescence-exit branch from d1ea893 to 5a610c7Compare February 12, 2026 23:27

jkczyz reviewed Feb 12, 2026
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated

wpaulino force-pushed the free-holding-cells-quiescence-exit branch from 5a610c7 to 826113bCompare February 13, 2026 00:57

wpaulino requested review from TheBlueMatt and jkczyz February 13, 2026 00:59

jkczyz reviewed Feb 13, 2026
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated

wpaulino force-pushed the free-holding-cells-quiescence-exit branch 2 times, most recently from 4b7cfee to f55c452Compare February 13, 2026 17:46

wpaulino requested a review from jkczyz February 17, 2026 17:15

wpaulino force-pushed the free-holding-cells-quiescence-exit branch from f55c452 to ac2631fCompare February 17, 2026 23:22

wpaulino added 3 commits February 17, 2026 15:32

Free holding cell upon handling a counterparty tx_abort
2665465
After cad88af, a few code paths that also lead to a quiescence exit were not accounted for. This commit addresses the path where we exit quiescence due to processing a counterparty's `tx_abort` message.

Free holding cell upon tx_signatures exchange
3e8b060
After cad88af, a few code paths that also lead to a quiescence exit were not accounted for. This commit addresses the last remaining path where we exit quiescence when we exchange `tx_signatures` with the counterparty.

wpaulino force-pushed the free-holding-cells-quiescence-exit branch from ac2631f to 0766da8Compare February 17, 2026 23:35

TheBlueMatt previously approved these changes Feb 18, 2026
View reviewed changes

jkczyz reviewed Feb 18, 2026
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated

wpaulino added 3 commits February 18, 2026 11:27

Inline unreachable debug assertion in MsgHandlErrInternal::no_such_peer
f6ea33e

Bind Channel::commitment_signed result in internal_commitment_signed
1b8617c

wpaulino dismissed TheBlueMatt’s stale review via 1b8617cFebruary 18, 2026 19:28

wpaulino force-pushed the free-holding-cells-quiescence-exit branch from 0766da8 to 1b8617cCompare February 18, 2026 19:28

jkczyz approved these changes Feb 18, 2026
View reviewed changes

wpaulino requested a review from TheBlueMatt February 18, 2026 22:09

TheBlueMatt approved these changes Feb 18, 2026
View reviewed changes

TheBlueMatt merged commit 153e57e into lightningdevkit:mainFeb 18, 2026
16 of 17 checks passed

github-project-automationBot moved this to Done in Weekly Goals Feb 18, 2026

wpaulino deleted the free-holding-cells-quiescence-exit branch February 18, 2026 22:57

jkczyz mentioned this pull request Mar 19, 2026
Splicing / Dual Funding #4284
Open
50 tasks