channelmonitor: Persist force-close broadcast preference

[martinsaposnic]

fixes#1563

Add allow_automated_broadcast flag to ChannelMonitor to prevent automatic commitment transaction broadcasting when channel was previously force-closed with should_broadcast=false.

Fixes unsafe broadcast on startup when ChannelManager finds orphaned monitors that were intentionally closed without broadcasting.

[ldk-reviews-bot]

👋 I see @joostjager was un-assigned.
If you'd like another reviewer assignemnt, please click here.

[codecov]

Codecov Report

❌ Patch coverage is 98.34711% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.77%. Comparing base (15724ec) to head (1eefa37).
⚠️ Report is 1768 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/reload_tests.rs	97.72%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3893 +/- ##
==========================================
- Coverage 89.65% 88.77% -0.89% 
==========================================
Files 164 164 Lines 134659 117502 -17157 Branches 134659 117502 -17157 ==========================================
- Hits 120731 104313 -16418 + Misses 11248 10846 -402 + Partials 2680 2343 -337 

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

[tnull]

This seems to conflict with the changes in #3881 which entirely removes the 'force close without broadcasting' case.

@TheBlueMatt@martinsaposnic Any thoughts how to resolve this?

[martinsaposnic]

This seems to conflict with the changes in #3881 which entirely removes the 'force close without broadcasting' case.

It seems should_broadcast: false can still be passed to ChannelForceClosed in #3881 (see https://github.com/lightningdevkit/rust-lightning/pull/3881/files#diff-8d28e96b6f27edb9a9739b2dce334f8906ec906155d421a7b5b02aa8e6c96057R5336-R5338). Also, the ChannelManager deserialization logic that re-emits the event with should_broadcast: true when encountering an orphaned ChannelMonitor (https://github.com/lightningdevkit/rust-lightning/blob/main/lightning/src/ln/channelmanager.rs#L14840) remains unchanged in #3881 .

Unless there’s a follow up plan to refactor that path as well, the bug addressed in this PR could still occur even after #3881 is merged.

I’m still getting familiar with this part of the codebase, so let me know if I’ve misunderstood anything @tnull

cc @TheBlueMatt

[TheBlueMatt]

#3881 doesn't attempt to "fix" the issue, but rather remove the issue by just calling the API itself a bug and deciding that we don't want have the ability to force-close a channel without broadcasting at all (only setting the skip-broadcasting flag in cases where the channel was never opened, and, indeed, not being perfect about it but its not a major issue at that point, vs today where a user took an action implying they are running with stale state and we broadcasted their state anyway!).

Instead, I was hoping that we could transition the "I have stale state and want to reclaim my funds without broadcasting that state" logic to a new flow entirely (what was started in #2943) which is basically just "here's a pile of ChannelMonitors and info to connect to peers so that we can get peer backups, please ask peers to close and get back whatever we can". This is a much cleaner API than the current "please start without connecting to peers, force-close-without-broadcasting, then connect to peers and then run normally" suggested flow, which can go wrong in several ways, eg if the channel got force-closed somehow on startup or on a reorg.

I was suggesting we go ahead and remove the existing flow because I don't actually think anyone has ever implemented it, though I admit getting a new flow in place might take some time (@adi2011 seems to have slowed down his contribution sadly). I certainly don't have a super strong opinion, though its also probably the case that doing a non-peer-storage-utilizing MVP of the new flow may be quite simple (just a ChainMonitor wrapper that tells all the ChannelMonitors to not broadcast the latest state).

[ldk-reviews-bot]

🔔 1st Reminder

Hey @joostjager! 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.

[joostjager]

Unassigned myself as reviewer, as there are open design questions.

[martinsaposnic]

Closing in favor of #3881 that was merged and fixed the issue

[martinsaposnic]

fixes#1563

Add allow_automated_broadcast flag to ChannelMonitor to prevent automatic commitment transaction broadcasting when channel was previously force-closed with should_broadcast=false.

Fixes unsafe broadcast on startup when ChannelManager finds orphaned monitors that were intentionally closed without broadcasting.

[ldk-reviews-bot]

👋 I see @joostjager was un-assigned.
If you'd like another reviewer assignemnt, please click here.

[codecov]

Codecov Report

❌ Patch coverage is 98.34711% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.77%. Comparing base (15724ec) to head (1eefa37).
⚠️ Report is 1768 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/reload_tests.rs	97.72%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3893 +/- ##
==========================================
- Coverage 89.65% 88.77% -0.89% 
==========================================
Files 164 164 Lines 134659 117502 -17157 Branches 134659 117502 -17157 ==========================================
- Hits 120731 104313 -16418 + Misses 11248 10846 -402 + Partials 2680 2343 -337 

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

[tnull]

This seems to conflict with the changes in #3881 which entirely removes the 'force close without broadcasting' case.

@TheBlueMatt@martinsaposnic Any thoughts how to resolve this?

[martinsaposnic]

This seems to conflict with the changes in #3881 which entirely removes the 'force close without broadcasting' case.

It seems should_broadcast: false can still be passed to ChannelForceClosed in #3881 (see https://github.com/lightningdevkit/rust-lightning/pull/3881/files#diff-8d28e96b6f27edb9a9739b2dce334f8906ec906155d421a7b5b02aa8e6c96057R5336-R5338). Also, the ChannelManager deserialization logic that re-emits the event with should_broadcast: true when encountering an orphaned ChannelMonitor (https://github.com/lightningdevkit/rust-lightning/blob/main/lightning/src/ln/channelmanager.rs#L14840) remains unchanged in #3881 .

Unless there’s a follow up plan to refactor that path as well, the bug addressed in this PR could still occur even after #3881 is merged.

I’m still getting familiar with this part of the codebase, so let me know if I’ve misunderstood anything @tnull

cc @TheBlueMatt

[TheBlueMatt]

#3881 doesn't attempt to "fix" the issue, but rather remove the issue by just calling the API itself a bug and deciding that we don't want have the ability to force-close a channel without broadcasting at all (only setting the skip-broadcasting flag in cases where the channel was never opened, and, indeed, not being perfect about it but its not a major issue at that point, vs today where a user took an action implying they are running with stale state and we broadcasted their state anyway!).

Instead, I was hoping that we could transition the "I have stale state and want to reclaim my funds without broadcasting that state" logic to a new flow entirely (what was started in #2943) which is basically just "here's a pile of ChannelMonitors and info to connect to peers so that we can get peer backups, please ask peers to close and get back whatever we can". This is a much cleaner API than the current "please start without connecting to peers, force-close-without-broadcasting, then connect to peers and then run normally" suggested flow, which can go wrong in several ways, eg if the channel got force-closed somehow on startup or on a reorg.

I was suggesting we go ahead and remove the existing flow because I don't actually think anyone has ever implemented it, though I admit getting a new flow in place might take some time (@adi2011 seems to have slowed down his contribution sadly). I certainly don't have a super strong opinion, though its also probably the case that doing a non-peer-storage-utilizing MVP of the new flow may be quite simple (just a ChainMonitor wrapper that tells all the ChannelMonitors to not broadcast the latest state).

[ldk-reviews-bot]

🔔 1st Reminder

Hey @joostjager! 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.

[joostjager]

Unassigned myself as reviewer, as there are open design questions.

[martinsaposnic]

Closing in favor of #3881 that was merged and fixed the issue

[martinsaposnic]

fixes#1563

Add allow_automated_broadcast flag to ChannelMonitor to prevent automatic commitment transaction broadcasting when channel was previously force-closed with should_broadcast=false.

Fixes unsafe broadcast on startup when ChannelManager finds orphaned monitors that were intentionally closed without broadcasting.

[ldk-reviews-bot]

👋 I see @joostjager was un-assigned.
If you'd like another reviewer assignemnt, please click here.

[codecov]

Codecov Report

❌ Patch coverage is 98.34711% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.77%. Comparing base (15724ec) to head (1eefa37).
⚠️ Report is 1768 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/reload_tests.rs	97.72%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3893 +/- ##
==========================================
- Coverage 89.65% 88.77% -0.89% 
==========================================
Files 164 164 Lines 134659 117502 -17157 Branches 134659 117502 -17157 ==========================================
- Hits 120731 104313 -16418 + Misses 11248 10846 -402 + Partials 2680 2343 -337 

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

[tnull]

This seems to conflict with the changes in #3881 which entirely removes the 'force close without broadcasting' case.

@TheBlueMatt@martinsaposnic Any thoughts how to resolve this?

[martinsaposnic]

This seems to conflict with the changes in #3881 which entirely removes the 'force close without broadcasting' case.

It seems should_broadcast: false can still be passed to ChannelForceClosed in #3881 (see https://github.com/lightningdevkit/rust-lightning/pull/3881/files#diff-8d28e96b6f27edb9a9739b2dce334f8906ec906155d421a7b5b02aa8e6c96057R5336-R5338). Also, the ChannelManager deserialization logic that re-emits the event with should_broadcast: true when encountering an orphaned ChannelMonitor (https://github.com/lightningdevkit/rust-lightning/blob/main/lightning/src/ln/channelmanager.rs#L14840) remains unchanged in #3881 .

Unless there’s a follow up plan to refactor that path as well, the bug addressed in this PR could still occur even after #3881 is merged.

I’m still getting familiar with this part of the codebase, so let me know if I’ve misunderstood anything @tnull

cc @TheBlueMatt

[TheBlueMatt]

#3881 doesn't attempt to "fix" the issue, but rather remove the issue by just calling the API itself a bug and deciding that we don't want have the ability to force-close a channel without broadcasting at all (only setting the skip-broadcasting flag in cases where the channel was never opened, and, indeed, not being perfect about it but its not a major issue at that point, vs today where a user took an action implying they are running with stale state and we broadcasted their state anyway!).

Instead, I was hoping that we could transition the "I have stale state and want to reclaim my funds without broadcasting that state" logic to a new flow entirely (what was started in #2943) which is basically just "here's a pile of ChannelMonitors and info to connect to peers so that we can get peer backups, please ask peers to close and get back whatever we can". This is a much cleaner API than the current "please start without connecting to peers, force-close-without-broadcasting, then connect to peers and then run normally" suggested flow, which can go wrong in several ways, eg if the channel got force-closed somehow on startup or on a reorg.

I was suggesting we go ahead and remove the existing flow because I don't actually think anyone has ever implemented it, though I admit getting a new flow in place might take some time (@adi2011 seems to have slowed down his contribution sadly). I certainly don't have a super strong opinion, though its also probably the case that doing a non-peer-storage-utilizing MVP of the new flow may be quite simple (just a ChainMonitor wrapper that tells all the ChannelMonitors to not broadcast the latest state).

[ldk-reviews-bot]

🔔 1st Reminder

Hey @joostjager! 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.

[joostjager]

Unassigned myself as reviewer, as there are open design questions.

[martinsaposnic]

Closing in favor of #3881 that was merged and fixed the issue

[martinsaposnic]

fixes#1563

Add allow_automated_broadcast flag to ChannelMonitor to prevent automatic commitment transaction broadcasting when channel was previously force-closed with should_broadcast=false.

Fixes unsafe broadcast on startup when ChannelManager finds orphaned monitors that were intentionally closed without broadcasting.

[ldk-reviews-bot]

👋 I see @joostjager was un-assigned.
If you'd like another reviewer assignemnt, please click here.

[codecov]

Codecov Report

❌ Patch coverage is 98.34711% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.77%. Comparing base (15724ec) to head (1eefa37).
⚠️ Report is 1768 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/reload_tests.rs	97.72%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3893 +/- ##
==========================================
- Coverage 89.65% 88.77% -0.89% 
==========================================
Files 164 164 Lines 134659 117502 -17157 Branches 134659 117502 -17157 ==========================================
- Hits 120731 104313 -16418 + Misses 11248 10846 -402 + Partials 2680 2343 -337 

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

[tnull]

This seems to conflict with the changes in #3881 which entirely removes the 'force close without broadcasting' case.

@TheBlueMatt@martinsaposnic Any thoughts how to resolve this?

[martinsaposnic]

This seems to conflict with the changes in #3881 which entirely removes the 'force close without broadcasting' case.

It seems should_broadcast: false can still be passed to ChannelForceClosed in #3881 (see https://github.com/lightningdevkit/rust-lightning/pull/3881/files#diff-8d28e96b6f27edb9a9739b2dce334f8906ec906155d421a7b5b02aa8e6c96057R5336-R5338). Also, the ChannelManager deserialization logic that re-emits the event with should_broadcast: true when encountering an orphaned ChannelMonitor (https://github.com/lightningdevkit/rust-lightning/blob/main/lightning/src/ln/channelmanager.rs#L14840) remains unchanged in #3881 .

Unless there’s a follow up plan to refactor that path as well, the bug addressed in this PR could still occur even after #3881 is merged.

I’m still getting familiar with this part of the codebase, so let me know if I’ve misunderstood anything @tnull

cc @TheBlueMatt

[TheBlueMatt]

#3881 doesn't attempt to "fix" the issue, but rather remove the issue by just calling the API itself a bug and deciding that we don't want have the ability to force-close a channel without broadcasting at all (only setting the skip-broadcasting flag in cases where the channel was never opened, and, indeed, not being perfect about it but its not a major issue at that point, vs today where a user took an action implying they are running with stale state and we broadcasted their state anyway!).

Instead, I was hoping that we could transition the "I have stale state and want to reclaim my funds without broadcasting that state" logic to a new flow entirely (what was started in #2943) which is basically just "here's a pile of ChannelMonitors and info to connect to peers so that we can get peer backups, please ask peers to close and get back whatever we can". This is a much cleaner API than the current "please start without connecting to peers, force-close-without-broadcasting, then connect to peers and then run normally" suggested flow, which can go wrong in several ways, eg if the channel got force-closed somehow on startup or on a reorg.

I was suggesting we go ahead and remove the existing flow because I don't actually think anyone has ever implemented it, though I admit getting a new flow in place might take some time (@adi2011 seems to have slowed down his contribution sadly). I certainly don't have a super strong opinion, though its also probably the case that doing a non-peer-storage-utilizing MVP of the new flow may be quite simple (just a ChainMonitor wrapper that tells all the ChannelMonitors to not broadcast the latest state).

[ldk-reviews-bot]

🔔 1st Reminder

Hey @joostjager! 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.

[joostjager]

Unassigned myself as reviewer, as there are open design questions.

[martinsaposnic]

Closing in favor of #3881 that was merged and fixed the issue

[martinsaposnic]

fixes#1563

Add allow_automated_broadcast flag to ChannelMonitor to prevent automatic commitment transaction broadcasting when channel was previously force-closed with should_broadcast=false.

Fixes unsafe broadcast on startup when ChannelManager finds orphaned monitors that were intentionally closed without broadcasting.

[ldk-reviews-bot]

👋 I see @joostjager was un-assigned.
If you'd like another reviewer assignemnt, please click here.

[codecov]

Codecov Report

❌ Patch coverage is 98.34711% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.77%. Comparing base (15724ec) to head (1eefa37).
⚠️ Report is 1768 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/reload_tests.rs	97.72%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3893 +/- ##
==========================================
- Coverage 89.65% 88.77% -0.89% 
==========================================
Files 164 164 Lines 134659 117502 -17157 Branches 134659 117502 -17157 ==========================================
- Hits 120731 104313 -16418 + Misses 11248 10846 -402 + Partials 2680 2343 -337 

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

[tnull]

This seems to conflict with the changes in #3881 which entirely removes the 'force close without broadcasting' case.

@TheBlueMatt@martinsaposnic Any thoughts how to resolve this?

[martinsaposnic]

This seems to conflict with the changes in #3881 which entirely removes the 'force close without broadcasting' case.

It seems should_broadcast: false can still be passed to ChannelForceClosed in #3881 (see https://github.com/lightningdevkit/rust-lightning/pull/3881/files#diff-8d28e96b6f27edb9a9739b2dce334f8906ec906155d421a7b5b02aa8e6c96057R5336-R5338). Also, the ChannelManager deserialization logic that re-emits the event with should_broadcast: true when encountering an orphaned ChannelMonitor (https://github.com/lightningdevkit/rust-lightning/blob/main/lightning/src/ln/channelmanager.rs#L14840) remains unchanged in #3881 .

Unless there’s a follow up plan to refactor that path as well, the bug addressed in this PR could still occur even after #3881 is merged.

I’m still getting familiar with this part of the codebase, so let me know if I’ve misunderstood anything @tnull

cc @TheBlueMatt

[TheBlueMatt]

#3881 doesn't attempt to "fix" the issue, but rather remove the issue by just calling the API itself a bug and deciding that we don't want have the ability to force-close a channel without broadcasting at all (only setting the skip-broadcasting flag in cases where the channel was never opened, and, indeed, not being perfect about it but its not a major issue at that point, vs today where a user took an action implying they are running with stale state and we broadcasted their state anyway!).

Instead, I was hoping that we could transition the "I have stale state and want to reclaim my funds without broadcasting that state" logic to a new flow entirely (what was started in #2943) which is basically just "here's a pile of ChannelMonitors and info to connect to peers so that we can get peer backups, please ask peers to close and get back whatever we can". This is a much cleaner API than the current "please start without connecting to peers, force-close-without-broadcasting, then connect to peers and then run normally" suggested flow, which can go wrong in several ways, eg if the channel got force-closed somehow on startup or on a reorg.

I was suggesting we go ahead and remove the existing flow because I don't actually think anyone has ever implemented it, though I admit getting a new flow in place might take some time (@adi2011 seems to have slowed down his contribution sadly). I certainly don't have a super strong opinion, though its also probably the case that doing a non-peer-storage-utilizing MVP of the new flow may be quite simple (just a ChainMonitor wrapper that tells all the ChannelMonitors to not broadcast the latest state).

[ldk-reviews-bot]

🔔 1st Reminder

Hey @joostjager! 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.

[joostjager]

Unassigned myself as reviewer, as there are open design questions.

[martinsaposnic]

Closing in favor of #3881 that was merged and fixed the issue

[martinsaposnic]

fixes#1563

Add allow_automated_broadcast flag to ChannelMonitor to prevent automatic commitment transaction broadcasting when channel was previously force-closed with should_broadcast=false.

Fixes unsafe broadcast on startup when ChannelManager finds orphaned monitors that were intentionally closed without broadcasting.

[ldk-reviews-bot]

👋 I see @joostjager was un-assigned.
If you'd like another reviewer assignemnt, please click here.

[codecov]

Codecov Report

❌ Patch coverage is 98.34711% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.77%. Comparing base (15724ec) to head (1eefa37).
⚠️ Report is 1768 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/reload_tests.rs	97.72%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3893 +/- ##
==========================================
- Coverage 89.65% 88.77% -0.89% 
==========================================
Files 164 164 Lines 134659 117502 -17157 Branches 134659 117502 -17157 ==========================================
- Hits 120731 104313 -16418 + Misses 11248 10846 -402 + Partials 2680 2343 -337 

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

[tnull]

This seems to conflict with the changes in #3881 which entirely removes the 'force close without broadcasting' case.

@TheBlueMatt@martinsaposnic Any thoughts how to resolve this?

[martinsaposnic]

This seems to conflict with the changes in #3881 which entirely removes the 'force close without broadcasting' case.

It seems should_broadcast: false can still be passed to ChannelForceClosed in #3881 (see https://github.com/lightningdevkit/rust-lightning/pull/3881/files#diff-8d28e96b6f27edb9a9739b2dce334f8906ec906155d421a7b5b02aa8e6c96057R5336-R5338). Also, the ChannelManager deserialization logic that re-emits the event with should_broadcast: true when encountering an orphaned ChannelMonitor (https://github.com/lightningdevkit/rust-lightning/blob/main/lightning/src/ln/channelmanager.rs#L14840) remains unchanged in #3881 .

Unless there’s a follow up plan to refactor that path as well, the bug addressed in this PR could still occur even after #3881 is merged.

I’m still getting familiar with this part of the codebase, so let me know if I’ve misunderstood anything @tnull

cc @TheBlueMatt

[TheBlueMatt]

#3881 doesn't attempt to "fix" the issue, but rather remove the issue by just calling the API itself a bug and deciding that we don't want have the ability to force-close a channel without broadcasting at all (only setting the skip-broadcasting flag in cases where the channel was never opened, and, indeed, not being perfect about it but its not a major issue at that point, vs today where a user took an action implying they are running with stale state and we broadcasted their state anyway!).

Instead, I was hoping that we could transition the "I have stale state and want to reclaim my funds without broadcasting that state" logic to a new flow entirely (what was started in #2943) which is basically just "here's a pile of ChannelMonitors and info to connect to peers so that we can get peer backups, please ask peers to close and get back whatever we can". This is a much cleaner API than the current "please start without connecting to peers, force-close-without-broadcasting, then connect to peers and then run normally" suggested flow, which can go wrong in several ways, eg if the channel got force-closed somehow on startup or on a reorg.

I was suggesting we go ahead and remove the existing flow because I don't actually think anyone has ever implemented it, though I admit getting a new flow in place might take some time (@adi2011 seems to have slowed down his contribution sadly). I certainly don't have a super strong opinion, though its also probably the case that doing a non-peer-storage-utilizing MVP of the new flow may be quite simple (just a ChainMonitor wrapper that tells all the ChannelMonitors to not broadcast the latest state).

[ldk-reviews-bot]

🔔 1st Reminder

Hey @joostjager! 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.

[joostjager]

Unassigned myself as reviewer, as there are open design questions.

[martinsaposnic]

Closing in favor of #3881 that was merged and fixed the issue

[martinsaposnic]

fixes#1563

Add allow_automated_broadcast flag to ChannelMonitor to prevent automatic commitment transaction broadcasting when channel was previously force-closed with should_broadcast=false.

Fixes unsafe broadcast on startup when ChannelManager finds orphaned monitors that were intentionally closed without broadcasting.

[ldk-reviews-bot]

👋 I see @joostjager was un-assigned.
If you'd like another reviewer assignemnt, please click here.

[codecov]

Codecov Report

❌ Patch coverage is 98.34711% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.77%. Comparing base (15724ec) to head (1eefa37).
⚠️ Report is 1768 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/reload_tests.rs	97.72%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3893 +/- ##
==========================================
- Coverage 89.65% 88.77% -0.89% 
==========================================
Files 164 164 Lines 134659 117502 -17157 Branches 134659 117502 -17157 ==========================================
- Hits 120731 104313 -16418 + Misses 11248 10846 -402 + Partials 2680 2343 -337 

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

[tnull]

This seems to conflict with the changes in #3881 which entirely removes the 'force close without broadcasting' case.

@TheBlueMatt@martinsaposnic Any thoughts how to resolve this?

[martinsaposnic]

This seems to conflict with the changes in #3881 which entirely removes the 'force close without broadcasting' case.

It seems should_broadcast: false can still be passed to ChannelForceClosed in #3881 (see https://github.com/lightningdevkit/rust-lightning/pull/3881/files#diff-8d28e96b6f27edb9a9739b2dce334f8906ec906155d421a7b5b02aa8e6c96057R5336-R5338). Also, the ChannelManager deserialization logic that re-emits the event with should_broadcast: true when encountering an orphaned ChannelMonitor (https://github.com/lightningdevkit/rust-lightning/blob/main/lightning/src/ln/channelmanager.rs#L14840) remains unchanged in #3881 .

Unless there’s a follow up plan to refactor that path as well, the bug addressed in this PR could still occur even after #3881 is merged.

I’m still getting familiar with this part of the codebase, so let me know if I’ve misunderstood anything @tnull

cc @TheBlueMatt

[TheBlueMatt]

#3881 doesn't attempt to "fix" the issue, but rather remove the issue by just calling the API itself a bug and deciding that we don't want have the ability to force-close a channel without broadcasting at all (only setting the skip-broadcasting flag in cases where the channel was never opened, and, indeed, not being perfect about it but its not a major issue at that point, vs today where a user took an action implying they are running with stale state and we broadcasted their state anyway!).

Instead, I was hoping that we could transition the "I have stale state and want to reclaim my funds without broadcasting that state" logic to a new flow entirely (what was started in #2943) which is basically just "here's a pile of ChannelMonitors and info to connect to peers so that we can get peer backups, please ask peers to close and get back whatever we can". This is a much cleaner API than the current "please start without connecting to peers, force-close-without-broadcasting, then connect to peers and then run normally" suggested flow, which can go wrong in several ways, eg if the channel got force-closed somehow on startup or on a reorg.

I was suggesting we go ahead and remove the existing flow because I don't actually think anyone has ever implemented it, though I admit getting a new flow in place might take some time (@adi2011 seems to have slowed down his contribution sadly). I certainly don't have a super strong opinion, though its also probably the case that doing a non-peer-storage-utilizing MVP of the new flow may be quite simple (just a ChainMonitor wrapper that tells all the ChannelMonitors to not broadcast the latest state).

[ldk-reviews-bot]

🔔 1st Reminder

Hey @joostjager! 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.

[joostjager]

Unassigned myself as reviewer, as there are open design questions.

[martinsaposnic]

Closing in favor of #3881 that was merged and fixed the issue