Clean Up Funding Error Handling and shutdown by TheBlueMatt · Pull Request #2809 · lightningdevkit/rust-lightning

TheBlueMatt · 2023-12-29T06:33:55Z

When updating the full_stack_target fuzzer as a part of #2808, I found a number of ways that funding errors aren't entirely properly handled (ie missing events in edge cases) and one case of a potential panic in extremely rare deployments (which, sadly, we're fixing for a second time, cause the first time we couldn't figure out how to write a test and concluded it was unreachable, but then later regressed and the fuzzer found it again).

If we promote our channel to `AwaitingChannelReady` after adding funding info, but still have `MONITOR_UPDATE_IN_PROGRESS` set, we haven't broadcasted the funding transaction yet and thus should return values from `unbroadcasted_funding[_txid]` and generate a `DiscardFunding` event.

Currently the channel shutdown sequence has a number of steps which all the shutdown callsites have to call. Because many shutdown cases are rare error cases, its relatively easy to miss a call and leave users without `Event`s or miss some important cleanup. One of those steps, calling `issue_channel_close_events`, is rather easy to remove, as it only generates two events, which can simply be moved to another shutdown step. Here we move the first of the two events, `DiscardFunding`, into `finish_force_close_channel`.

Currently the channel shutdown sequence has a number of steps which all the shutdown callsites have to call. Because many shutdown cases are rare error cases, its relatively easy to miss a call and leave users without `Event`s or miss some important cleanup. One of those steps, calling `issue_channel_close_events`, is rather easy to remove, as it only generates two events, which can simply be moved to another shutdown step. Here we remove `issue_channel_close_events` by moving `ChannelClosed` event generation into `finish_force_close_channel`.

We'd previously assumed that LDK would receive `funding_transaction_generated` prior to our peer learning the txid and panicked if the peer tried to open a redundant channel to us with the same funding outpoint. While this assumption is generally safe, some users may have out-of-band protocols where they notify their LSP about a funding outpoint first, or this may be violated in the future with collaborative transaction construction protocols, i.e. the upcoming dual-funding protocol.

In e06484b, we added specific handling for outbound-channel initial monitor updates failing - in such a case we have a counterparty who tried to open a second channel with the same funding info we just gave them, causing us to force-close our outbound channel as it shows up as duplicate-funding. Its largely harmless as it leads to a spurious force-closure of a channel with a peer doing something absurd, however it causes the `full_stack_target` fuzzer to fail. Sadly, in 574c77e, as we were dropping handling of `PermanentFailure` handling for updates, we accidentally dropped handling for initial updates as well. Here we fix the issue (again) and add a test.

codecov-commenter · 2023-12-29T06:40:35Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (4deb263) 88.46% compared to head (7bc2a14) 88.79%.
Report is 12 commits behind head on main.

Files	Patch %	Lines
lightning/src/ln/channelmanager.rs	98.41%	1 Missing ⚠️
lightning/src/ln/functional_tests.rs	99.03%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2809 +/- ##
==========================================
+ Coverage 88.46% 88.79% +0.33% 
==========================================
Files 114 114 Lines 91806 95767 +3961 Branches 91806 95767 +3961 ==========================================
+ Hits 81214 85038 +3824 - Misses 8112 8289 +177 + Partials 2480 2440 -40

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

TheBlueMatt · 2023-12-29T06:33:55Z

When updating the full_stack_target fuzzer as a part of #2808, I found a number of ways that funding errors aren't entirely properly handled (ie missing events in edge cases) and one case of a potential panic in extremely rare deployments (which, sadly, we're fixing for a second time, cause the first time we couldn't figure out how to write a test and concluded it was unreachable, but then later regressed and the fuzzer found it again).

If we promote our channel to `AwaitingChannelReady` after adding funding info, but still have `MONITOR_UPDATE_IN_PROGRESS` set, we haven't broadcasted the funding transaction yet and thus should return values from `unbroadcasted_funding[_txid]` and generate a `DiscardFunding` event.

Currently the channel shutdown sequence has a number of steps which all the shutdown callsites have to call. Because many shutdown cases are rare error cases, its relatively easy to miss a call and leave users without `Event`s or miss some important cleanup. One of those steps, calling `issue_channel_close_events`, is rather easy to remove, as it only generates two events, which can simply be moved to another shutdown step. Here we move the first of the two events, `DiscardFunding`, into `finish_force_close_channel`.

Currently the channel shutdown sequence has a number of steps which all the shutdown callsites have to call. Because many shutdown cases are rare error cases, its relatively easy to miss a call and leave users without `Event`s or miss some important cleanup. One of those steps, calling `issue_channel_close_events`, is rather easy to remove, as it only generates two events, which can simply be moved to another shutdown step. Here we remove `issue_channel_close_events` by moving `ChannelClosed` event generation into `finish_force_close_channel`.

We'd previously assumed that LDK would receive `funding_transaction_generated` prior to our peer learning the txid and panicked if the peer tried to open a redundant channel to us with the same funding outpoint. While this assumption is generally safe, some users may have out-of-band protocols where they notify their LSP about a funding outpoint first, or this may be violated in the future with collaborative transaction construction protocols, i.e. the upcoming dual-funding protocol.

In e06484b, we added specific handling for outbound-channel initial monitor updates failing - in such a case we have a counterparty who tried to open a second channel with the same funding info we just gave them, causing us to force-close our outbound channel as it shows up as duplicate-funding. Its largely harmless as it leads to a spurious force-closure of a channel with a peer doing something absurd, however it causes the `full_stack_target` fuzzer to fail. Sadly, in 574c77e, as we were dropping handling of `PermanentFailure` handling for updates, we accidentally dropped handling for initial updates as well. Here we fix the issue (again) and add a test.

codecov-commenter · 2023-12-29T06:40:35Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (4deb263) 88.46% compared to head (7bc2a14) 88.79%.
Report is 12 commits behind head on main.

Files	Patch %	Lines
lightning/src/ln/channelmanager.rs	98.41%	1 Missing ⚠️
lightning/src/ln/functional_tests.rs	99.03%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2809 +/- ##
==========================================
+ Coverage 88.46% 88.79% +0.33% 
==========================================
Files 114 114 Lines 91806 95767 +3961 Branches 91806 95767 +3961 ==========================================
+ Hits 81214 85038 +3824 - Misses 8112 8289 +177 + Partials 2480 2440 -40

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

TheBlueMatt · 2023-12-29T06:33:55Z

When updating the full_stack_target fuzzer as a part of #2808, I found a number of ways that funding errors aren't entirely properly handled (ie missing events in edge cases) and one case of a potential panic in extremely rare deployments (which, sadly, we're fixing for a second time, cause the first time we couldn't figure out how to write a test and concluded it was unreachable, but then later regressed and the fuzzer found it again).

If we promote our channel to `AwaitingChannelReady` after adding funding info, but still have `MONITOR_UPDATE_IN_PROGRESS` set, we haven't broadcasted the funding transaction yet and thus should return values from `unbroadcasted_funding[_txid]` and generate a `DiscardFunding` event.

Currently the channel shutdown sequence has a number of steps which all the shutdown callsites have to call. Because many shutdown cases are rare error cases, its relatively easy to miss a call and leave users without `Event`s or miss some important cleanup. One of those steps, calling `issue_channel_close_events`, is rather easy to remove, as it only generates two events, which can simply be moved to another shutdown step. Here we move the first of the two events, `DiscardFunding`, into `finish_force_close_channel`.

Currently the channel shutdown sequence has a number of steps which all the shutdown callsites have to call. Because many shutdown cases are rare error cases, its relatively easy to miss a call and leave users without `Event`s or miss some important cleanup. One of those steps, calling `issue_channel_close_events`, is rather easy to remove, as it only generates two events, which can simply be moved to another shutdown step. Here we remove `issue_channel_close_events` by moving `ChannelClosed` event generation into `finish_force_close_channel`.

We'd previously assumed that LDK would receive `funding_transaction_generated` prior to our peer learning the txid and panicked if the peer tried to open a redundant channel to us with the same funding outpoint. While this assumption is generally safe, some users may have out-of-band protocols where they notify their LSP about a funding outpoint first, or this may be violated in the future with collaborative transaction construction protocols, i.e. the upcoming dual-funding protocol.

In e06484b, we added specific handling for outbound-channel initial monitor updates failing - in such a case we have a counterparty who tried to open a second channel with the same funding info we just gave them, causing us to force-close our outbound channel as it shows up as duplicate-funding. Its largely harmless as it leads to a spurious force-closure of a channel with a peer doing something absurd, however it causes the `full_stack_target` fuzzer to fail. Sadly, in 574c77e, as we were dropping handling of `PermanentFailure` handling for updates, we accidentally dropped handling for initial updates as well. Here we fix the issue (again) and add a test.

codecov-commenter · 2023-12-29T06:40:35Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (4deb263) 88.46% compared to head (7bc2a14) 88.79%.
Report is 12 commits behind head on main.

Files	Patch %	Lines
lightning/src/ln/channelmanager.rs	98.41%	1 Missing ⚠️
lightning/src/ln/functional_tests.rs	99.03%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2809 +/- ##
==========================================
+ Coverage 88.46% 88.79% +0.33% 
==========================================
Files 114 114 Lines 91806 95767 +3961 Branches 91806 95767 +3961 ==========================================
+ Hits 81214 85038 +3824 - Misses 8112 8289 +177 + Partials 2480 2440 -40

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

TheBlueMatt · 2023-12-29T06:33:55Z

When updating the full_stack_target fuzzer as a part of #2808, I found a number of ways that funding errors aren't entirely properly handled (ie missing events in edge cases) and one case of a potential panic in extremely rare deployments (which, sadly, we're fixing for a second time, cause the first time we couldn't figure out how to write a test and concluded it was unreachable, but then later regressed and the fuzzer found it again).

If we promote our channel to `AwaitingChannelReady` after adding funding info, but still have `MONITOR_UPDATE_IN_PROGRESS` set, we haven't broadcasted the funding transaction yet and thus should return values from `unbroadcasted_funding[_txid]` and generate a `DiscardFunding` event.

Currently the channel shutdown sequence has a number of steps which all the shutdown callsites have to call. Because many shutdown cases are rare error cases, its relatively easy to miss a call and leave users without `Event`s or miss some important cleanup. One of those steps, calling `issue_channel_close_events`, is rather easy to remove, as it only generates two events, which can simply be moved to another shutdown step. Here we move the first of the two events, `DiscardFunding`, into `finish_force_close_channel`.

Currently the channel shutdown sequence has a number of steps which all the shutdown callsites have to call. Because many shutdown cases are rare error cases, its relatively easy to miss a call and leave users without `Event`s or miss some important cleanup. One of those steps, calling `issue_channel_close_events`, is rather easy to remove, as it only generates two events, which can simply be moved to another shutdown step. Here we remove `issue_channel_close_events` by moving `ChannelClosed` event generation into `finish_force_close_channel`.

We'd previously assumed that LDK would receive `funding_transaction_generated` prior to our peer learning the txid and panicked if the peer tried to open a redundant channel to us with the same funding outpoint. While this assumption is generally safe, some users may have out-of-band protocols where they notify their LSP about a funding outpoint first, or this may be violated in the future with collaborative transaction construction protocols, i.e. the upcoming dual-funding protocol.

In e06484b, we added specific handling for outbound-channel initial monitor updates failing - in such a case we have a counterparty who tried to open a second channel with the same funding info we just gave them, causing us to force-close our outbound channel as it shows up as duplicate-funding. Its largely harmless as it leads to a spurious force-closure of a channel with a peer doing something absurd, however it causes the `full_stack_target` fuzzer to fail. Sadly, in 574c77e, as we were dropping handling of `PermanentFailure` handling for updates, we accidentally dropped handling for initial updates as well. Here we fix the issue (again) and add a test.

codecov-commenter · 2023-12-29T06:40:35Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (4deb263) 88.46% compared to head (7bc2a14) 88.79%.
Report is 12 commits behind head on main.

Files	Patch %	Lines
lightning/src/ln/channelmanager.rs	98.41%	1 Missing ⚠️
lightning/src/ln/functional_tests.rs	99.03%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2809 +/- ##
==========================================
+ Coverage 88.46% 88.79% +0.33% 
==========================================
Files 114 114 Lines 91806 95767 +3961 Branches 91806 95767 +3961 ==========================================
+ Hits 81214 85038 +3824 - Misses 8112 8289 +177 + Partials 2480 2440 -40

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

TheBlueMatt · 2023-12-29T06:33:55Z

When updating the full_stack_target fuzzer as a part of #2808, I found a number of ways that funding errors aren't entirely properly handled (ie missing events in edge cases) and one case of a potential panic in extremely rare deployments (which, sadly, we're fixing for a second time, cause the first time we couldn't figure out how to write a test and concluded it was unreachable, but then later regressed and the fuzzer found it again).

If we promote our channel to `AwaitingChannelReady` after adding funding info, but still have `MONITOR_UPDATE_IN_PROGRESS` set, we haven't broadcasted the funding transaction yet and thus should return values from `unbroadcasted_funding[_txid]` and generate a `DiscardFunding` event.

Currently the channel shutdown sequence has a number of steps which all the shutdown callsites have to call. Because many shutdown cases are rare error cases, its relatively easy to miss a call and leave users without `Event`s or miss some important cleanup. One of those steps, calling `issue_channel_close_events`, is rather easy to remove, as it only generates two events, which can simply be moved to another shutdown step. Here we move the first of the two events, `DiscardFunding`, into `finish_force_close_channel`.

Currently the channel shutdown sequence has a number of steps which all the shutdown callsites have to call. Because many shutdown cases are rare error cases, its relatively easy to miss a call and leave users without `Event`s or miss some important cleanup. One of those steps, calling `issue_channel_close_events`, is rather easy to remove, as it only generates two events, which can simply be moved to another shutdown step. Here we remove `issue_channel_close_events` by moving `ChannelClosed` event generation into `finish_force_close_channel`.

We'd previously assumed that LDK would receive `funding_transaction_generated` prior to our peer learning the txid and panicked if the peer tried to open a redundant channel to us with the same funding outpoint. While this assumption is generally safe, some users may have out-of-band protocols where they notify their LSP about a funding outpoint first, or this may be violated in the future with collaborative transaction construction protocols, i.e. the upcoming dual-funding protocol.

In e06484b, we added specific handling for outbound-channel initial monitor updates failing - in such a case we have a counterparty who tried to open a second channel with the same funding info we just gave them, causing us to force-close our outbound channel as it shows up as duplicate-funding. Its largely harmless as it leads to a spurious force-closure of a channel with a peer doing something absurd, however it causes the `full_stack_target` fuzzer to fail. Sadly, in 574c77e, as we were dropping handling of `PermanentFailure` handling for updates, we accidentally dropped handling for initial updates as well. Here we fix the issue (again) and add a test.

codecov-commenter · 2023-12-29T06:40:35Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (4deb263) 88.46% compared to head (7bc2a14) 88.79%.
Report is 12 commits behind head on main.

Files	Patch %	Lines
lightning/src/ln/channelmanager.rs	98.41%	1 Missing ⚠️
lightning/src/ln/functional_tests.rs	99.03%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2809 +/- ##
==========================================
+ Coverage 88.46% 88.79% +0.33% 
==========================================
Files 114 114 Lines 91806 95767 +3961 Branches 91806 95767 +3961 ==========================================
+ Hits 81214 85038 +3824 - Misses 8112 8289 +177 + Partials 2480 2440 -40

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

TheBlueMatt · 2023-12-29T06:33:55Z

When updating the full_stack_target fuzzer as a part of #2808, I found a number of ways that funding errors aren't entirely properly handled (ie missing events in edge cases) and one case of a potential panic in extremely rare deployments (which, sadly, we're fixing for a second time, cause the first time we couldn't figure out how to write a test and concluded it was unreachable, but then later regressed and the fuzzer found it again).

If we promote our channel to `AwaitingChannelReady` after adding funding info, but still have `MONITOR_UPDATE_IN_PROGRESS` set, we haven't broadcasted the funding transaction yet and thus should return values from `unbroadcasted_funding[_txid]` and generate a `DiscardFunding` event.

Currently the channel shutdown sequence has a number of steps which all the shutdown callsites have to call. Because many shutdown cases are rare error cases, its relatively easy to miss a call and leave users without `Event`s or miss some important cleanup. One of those steps, calling `issue_channel_close_events`, is rather easy to remove, as it only generates two events, which can simply be moved to another shutdown step. Here we move the first of the two events, `DiscardFunding`, into `finish_force_close_channel`.

Currently the channel shutdown sequence has a number of steps which all the shutdown callsites have to call. Because many shutdown cases are rare error cases, its relatively easy to miss a call and leave users without `Event`s or miss some important cleanup. One of those steps, calling `issue_channel_close_events`, is rather easy to remove, as it only generates two events, which can simply be moved to another shutdown step. Here we remove `issue_channel_close_events` by moving `ChannelClosed` event generation into `finish_force_close_channel`.

We'd previously assumed that LDK would receive `funding_transaction_generated` prior to our peer learning the txid and panicked if the peer tried to open a redundant channel to us with the same funding outpoint. While this assumption is generally safe, some users may have out-of-band protocols where they notify their LSP about a funding outpoint first, or this may be violated in the future with collaborative transaction construction protocols, i.e. the upcoming dual-funding protocol.

In e06484b, we added specific handling for outbound-channel initial monitor updates failing - in such a case we have a counterparty who tried to open a second channel with the same funding info we just gave them, causing us to force-close our outbound channel as it shows up as duplicate-funding. Its largely harmless as it leads to a spurious force-closure of a channel with a peer doing something absurd, however it causes the `full_stack_target` fuzzer to fail. Sadly, in 574c77e, as we were dropping handling of `PermanentFailure` handling for updates, we accidentally dropped handling for initial updates as well. Here we fix the issue (again) and add a test.

codecov-commenter · 2023-12-29T06:40:35Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (4deb263) 88.46% compared to head (7bc2a14) 88.79%.
Report is 12 commits behind head on main.

Files	Patch %	Lines
lightning/src/ln/channelmanager.rs	98.41%	1 Missing ⚠️
lightning/src/ln/functional_tests.rs	99.03%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2809 +/- ##
==========================================
+ Coverage 88.46% 88.79% +0.33% 
==========================================
Files 114 114 Lines 91806 95767 +3961 Branches 91806 95767 +3961 ==========================================
+ Hits 81214 85038 +3824 - Misses 8112 8289 +177 + Partials 2480 2440 -40

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

TheBlueMatt · 2023-12-29T06:33:55Z

When updating the full_stack_target fuzzer as a part of #2808, I found a number of ways that funding errors aren't entirely properly handled (ie missing events in edge cases) and one case of a potential panic in extremely rare deployments (which, sadly, we're fixing for a second time, cause the first time we couldn't figure out how to write a test and concluded it was unreachable, but then later regressed and the fuzzer found it again).

If we promote our channel to `AwaitingChannelReady` after adding funding info, but still have `MONITOR_UPDATE_IN_PROGRESS` set, we haven't broadcasted the funding transaction yet and thus should return values from `unbroadcasted_funding[_txid]` and generate a `DiscardFunding` event.

Currently the channel shutdown sequence has a number of steps which all the shutdown callsites have to call. Because many shutdown cases are rare error cases, its relatively easy to miss a call and leave users without `Event`s or miss some important cleanup. One of those steps, calling `issue_channel_close_events`, is rather easy to remove, as it only generates two events, which can simply be moved to another shutdown step. Here we move the first of the two events, `DiscardFunding`, into `finish_force_close_channel`.

Currently the channel shutdown sequence has a number of steps which all the shutdown callsites have to call. Because many shutdown cases are rare error cases, its relatively easy to miss a call and leave users without `Event`s or miss some important cleanup. One of those steps, calling `issue_channel_close_events`, is rather easy to remove, as it only generates two events, which can simply be moved to another shutdown step. Here we remove `issue_channel_close_events` by moving `ChannelClosed` event generation into `finish_force_close_channel`.

We'd previously assumed that LDK would receive `funding_transaction_generated` prior to our peer learning the txid and panicked if the peer tried to open a redundant channel to us with the same funding outpoint. While this assumption is generally safe, some users may have out-of-band protocols where they notify their LSP about a funding outpoint first, or this may be violated in the future with collaborative transaction construction protocols, i.e. the upcoming dual-funding protocol.

In e06484b, we added specific handling for outbound-channel initial monitor updates failing - in such a case we have a counterparty who tried to open a second channel with the same funding info we just gave them, causing us to force-close our outbound channel as it shows up as duplicate-funding. Its largely harmless as it leads to a spurious force-closure of a channel with a peer doing something absurd, however it causes the `full_stack_target` fuzzer to fail. Sadly, in 574c77e, as we were dropping handling of `PermanentFailure` handling for updates, we accidentally dropped handling for initial updates as well. Here we fix the issue (again) and add a test.

codecov-commenter · 2023-12-29T06:40:35Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (4deb263) 88.46% compared to head (7bc2a14) 88.79%.
Report is 12 commits behind head on main.

Files	Patch %	Lines
lightning/src/ln/channelmanager.rs	98.41%	1 Missing ⚠️
lightning/src/ln/functional_tests.rs	99.03%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2809 +/- ##
==========================================
+ Coverage 88.46% 88.79% +0.33% 
==========================================
Files 114 114 Lines 91806 95767 +3961 Branches 91806 95767 +3961 ==========================================
+ Hits 81214 85038 +3824 - Misses 8112 8289 +177 + Partials 2480 2440 -40

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

TheBlueMatt · 2023-12-29T06:33:55Z

When updating the full_stack_target fuzzer as a part of #2808, I found a number of ways that funding errors aren't entirely properly handled (ie missing events in edge cases) and one case of a potential panic in extremely rare deployments (which, sadly, we're fixing for a second time, cause the first time we couldn't figure out how to write a test and concluded it was unreachable, but then later regressed and the fuzzer found it again).

If we promote our channel to `AwaitingChannelReady` after adding funding info, but still have `MONITOR_UPDATE_IN_PROGRESS` set, we haven't broadcasted the funding transaction yet and thus should return values from `unbroadcasted_funding[_txid]` and generate a `DiscardFunding` event.

Currently the channel shutdown sequence has a number of steps which all the shutdown callsites have to call. Because many shutdown cases are rare error cases, its relatively easy to miss a call and leave users without `Event`s or miss some important cleanup. One of those steps, calling `issue_channel_close_events`, is rather easy to remove, as it only generates two events, which can simply be moved to another shutdown step. Here we move the first of the two events, `DiscardFunding`, into `finish_force_close_channel`.

Currently the channel shutdown sequence has a number of steps which all the shutdown callsites have to call. Because many shutdown cases are rare error cases, its relatively easy to miss a call and leave users without `Event`s or miss some important cleanup. One of those steps, calling `issue_channel_close_events`, is rather easy to remove, as it only generates two events, which can simply be moved to another shutdown step. Here we remove `issue_channel_close_events` by moving `ChannelClosed` event generation into `finish_force_close_channel`.

We'd previously assumed that LDK would receive `funding_transaction_generated` prior to our peer learning the txid and panicked if the peer tried to open a redundant channel to us with the same funding outpoint. While this assumption is generally safe, some users may have out-of-band protocols where they notify their LSP about a funding outpoint first, or this may be violated in the future with collaborative transaction construction protocols, i.e. the upcoming dual-funding protocol.

In e06484b, we added specific handling for outbound-channel initial monitor updates failing - in such a case we have a counterparty who tried to open a second channel with the same funding info we just gave them, causing us to force-close our outbound channel as it shows up as duplicate-funding. Its largely harmless as it leads to a spurious force-closure of a channel with a peer doing something absurd, however it causes the `full_stack_target` fuzzer to fail. Sadly, in 574c77e, as we were dropping handling of `PermanentFailure` handling for updates, we accidentally dropped handling for initial updates as well. Here we fix the issue (again) and add a test.

codecov-commenter · 2023-12-29T06:40:35Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (4deb263) 88.46% compared to head (7bc2a14) 88.79%.
Report is 12 commits behind head on main.

Files	Patch %	Lines
lightning/src/ln/channelmanager.rs	98.41%	1 Missing ⚠️
lightning/src/ln/functional_tests.rs	99.03%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2809 +/- ##
==========================================
+ Coverage 88.46% 88.79% +0.33% 
==========================================
Files 114 114 Lines 91806 95767 +3961 Branches 91806 95767 +3961 ==========================================
+ Hits 81214 85038 +3824 - Misses 8112 8289 +177 + Partials 2480 2440 -40

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

TheBlueMatt added 6 commits December 29, 2023 06:27

Drop now-unused fields from MsgHandleErrInternal
bfe911d

TheBlueMatt added this to the 0.0.120 milestone Dec 29, 2023

TheBlueMatt mentioned this pull request Dec 29, 2023
Update full_stack_target to take an arbitrary config object #2810
Merged

wpaulino approved these changes Jan 4, 2024
View reviewed changes

TheBlueMatt mentioned this pull request Jan 9, 2024
Add channel funding txo to Channel Event::ChannelClosed #2800
Merged

dunxen approved these changes Jan 10, 2024
View reviewed changes

TheBlueMatt merged commit ccf710d into lightningdevkit:mainJan 10, 2024

TheBlueMatt added 6 commits December 29, 2023 06:27

Drop now-unused fields from MsgHandleErrInternal
bfe911d

TheBlueMatt added this to the 0.0.120 milestone Dec 29, 2023

TheBlueMatt mentioned this pull request Dec 29, 2023
Update full_stack_target to take an arbitrary config object #2810
Merged

wpaulino approved these changes Jan 4, 2024
View reviewed changes

TheBlueMatt mentioned this pull request Jan 9, 2024
Add channel funding txo to Channel Event::ChannelClosed #2800
Merged

dunxen approved these changes Jan 10, 2024
View reviewed changes

TheBlueMatt merged commit ccf710d into lightningdevkit:mainJan 10, 2024

TheBlueMatt added 6 commits December 29, 2023 06:27

Drop now-unused fields from MsgHandleErrInternal
bfe911d

TheBlueMatt added this to the 0.0.120 milestone Dec 29, 2023

TheBlueMatt mentioned this pull request Dec 29, 2023
Update full_stack_target to take an arbitrary config object #2810
Merged

wpaulino approved these changes Jan 4, 2024
View reviewed changes

TheBlueMatt mentioned this pull request Jan 9, 2024
Add channel funding txo to Channel Event::ChannelClosed #2800
Merged

dunxen approved these changes Jan 10, 2024
View reviewed changes

TheBlueMatt merged commit ccf710d into lightningdevkit:mainJan 10, 2024

TheBlueMatt added 6 commits December 29, 2023 06:27

Drop now-unused fields from MsgHandleErrInternal
bfe911d

TheBlueMatt added this to the 0.0.120 milestone Dec 29, 2023

TheBlueMatt mentioned this pull request Dec 29, 2023
Update full_stack_target to take an arbitrary config object #2810
Merged

wpaulino approved these changes Jan 4, 2024
View reviewed changes

TheBlueMatt mentioned this pull request Jan 9, 2024
Add channel funding txo to Channel Event::ChannelClosed #2800
Merged

dunxen approved these changes Jan 10, 2024
View reviewed changes

TheBlueMatt merged commit ccf710d into lightningdevkit:mainJan 10, 2024

TheBlueMatt added 6 commits December 29, 2023 06:27

Drop now-unused fields from MsgHandleErrInternal
bfe911d

TheBlueMatt added this to the 0.0.120 milestone Dec 29, 2023

TheBlueMatt mentioned this pull request Dec 29, 2023
Update full_stack_target to take an arbitrary config object #2810
Merged

wpaulino approved these changes Jan 4, 2024
View reviewed changes

TheBlueMatt mentioned this pull request Jan 9, 2024
Add channel funding txo to Channel Event::ChannelClosed #2800
Merged

dunxen approved these changes Jan 10, 2024
View reviewed changes

TheBlueMatt merged commit ccf710d into lightningdevkit:mainJan 10, 2024

TheBlueMatt added 6 commits December 29, 2023 06:27

Drop now-unused fields from MsgHandleErrInternal
bfe911d

TheBlueMatt added this to the 0.0.120 milestone Dec 29, 2023

TheBlueMatt mentioned this pull request Dec 29, 2023
Update full_stack_target to take an arbitrary config object #2810
Merged

wpaulino approved these changes Jan 4, 2024
View reviewed changes

TheBlueMatt mentioned this pull request Jan 9, 2024
Add channel funding txo to Channel Event::ChannelClosed #2800
Merged

dunxen approved these changes Jan 10, 2024
View reviewed changes

TheBlueMatt merged commit ccf710d into lightningdevkit:mainJan 10, 2024

TheBlueMatt added 6 commits December 29, 2023 06:27

Drop now-unused fields from MsgHandleErrInternal
bfe911d

TheBlueMatt added this to the 0.0.120 milestone Dec 29, 2023

TheBlueMatt mentioned this pull request Dec 29, 2023
Update full_stack_target to take an arbitrary config object #2810
Merged

wpaulino approved these changes Jan 4, 2024
View reviewed changes

TheBlueMatt mentioned this pull request Jan 9, 2024
Add channel funding txo to Channel Event::ChannelClosed #2800
Merged

dunxen approved these changes Jan 10, 2024
View reviewed changes

TheBlueMatt merged commit ccf710d into lightningdevkit:mainJan 10, 2024

TheBlueMatt added 6 commits December 29, 2023 06:27

Drop now-unused fields from MsgHandleErrInternal
bfe911d

TheBlueMatt added this to the 0.0.120 milestone Dec 29, 2023

TheBlueMatt mentioned this pull request Dec 29, 2023
Update full_stack_target to take an arbitrary config object #2810
Merged

wpaulino approved these changes Jan 4, 2024
View reviewed changes

TheBlueMatt mentioned this pull request Jan 9, 2024
Add channel funding txo to Channel Event::ChannelClosed #2800
Merged

dunxen approved these changes Jan 10, 2024
View reviewed changes

TheBlueMatt merged commit ccf710d into lightningdevkit:mainJan 10, 2024

Uh oh!

Conversation

TheBlueMatt commented Dec 29, 2023

Uh oh!

codecov-commenter commented Dec 29, 2023

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

TheBlueMatt commented Dec 29, 2023

Uh oh!

codecov-commenter commented Dec 29, 2023

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

TheBlueMatt commented Dec 29, 2023

Uh oh!

codecov-commenter commented Dec 29, 2023

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

TheBlueMatt commented Dec 29, 2023

Uh oh!

codecov-commenter commented Dec 29, 2023

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

TheBlueMatt commented Dec 29, 2023

Uh oh!

codecov-commenter commented Dec 29, 2023

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

TheBlueMatt commented Dec 29, 2023

Uh oh!

codecov-commenter commented Dec 29, 2023