[0.1] Backports for 0.1.4 by TheBlueMatt · Pull Request #3794 · lightningdevkit/rust-lightning

TheBlueMatt · 2025-05-23T14:04:54Z

Backport of #3772, two commits from #3584, and #3790. The two commits from #3584 are kinda awkward (testing upgrade from 0.1 to 0.1) but it seems weirder to drop the test in a backport and it exposes a few more methods as public (which may be helpful for later upgrade tests from 0.1 to git/0.2/etc).

EDIT: Plus #3796

When we begin claiming a payment, we move the tracking of it from `claimable_payments` to `claiming_payments`. This ensures we only ever have one payment which is in the process of being claimed with a given payment hash at a time and lets us keep track of when all parts have been claimed with their `ChannelMonitor`s. However, on startup, we check that failing to move a payment from `claimable_payments` to `claiming_payments` implies that it is not present in `claiming_payments`. This is fine if the payment doesn't exist, but if the payment has already started being claimed, this will fail and we'll refuse to deserialize the `ChannelManager` (with a `debug_assert` failure in debug mode). Here we resolve this by checking if a payment is already being claimed before we attempt to initiate claiming and skip the failing check in that case.

ldk-reviews-bot · 2025-05-23T14:04:57Z

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

wpaulino · 2025-05-23T16:32:48Z

Should probably fix the fuzz build before merging

The signer we use in tests tracks the state of the channel and refuses to sign when the channel attempts an invalid state transition. In the next commit, however, we'll add an upgrade test which will fail these checks as the the state won't get copied from previous versions of LDK to this version. Thus, here, we add the ability to disable all state-based checks in the signer.

One major hole in our test coverage historically has been tests covering upgrades or downgrades across LDK versions. Luckily, these aren't particularly hard to write as cargo lets us depend on previous versions of the `lightning` crate directly, which we can use in tests. Here we add a simple initial test of upgrading from LDK 0.1 while there's a pending payment to be claimed.

In 93b4479 we fixed an issue which could cause a `ChannelMonitorUpdate` to get marked as blocked on itself, leading to an eventual force-closure. One potential side-effect of that issue, however, is that any further `ChannelMonitorUpdate`s to the same channel while it is blocked will not have any post-update actions processed (as there is a pending blocked `ChannelMonitorUpdate` sitting in the channel). This can leave a dangling `MonitorUpdateCompletionAction` sitting around even after the channel is closed. In 0.1, because `ChannelMonitorUpdate`s to closed channels were finally fully tracked, we started enforcing that any post-update completion action we had on startup corresponded to a peer entry, while at the same time no longer creating peer entries just because we had serialized one in the data we were loading (only creating them if we had channel(s) or a `ChannelMonitor`). This can cause some `ChannelManager` to no longer deserialize on 0.1 as we might have a left-over dangling `MonitorUpdateCompletionAction` and will no longer always have a peer entry just because of it. Here we fix this issue by specifically checking for dangling `MonitorUpdateCompletionAction::PaymentClaim` entries and dropping them if there is no corresponding channel or peer state entry. We only check for `PaymentClaimed` actions rather than allowing for any dangling actions as 93b4479 was only triggerable with MPP claims, so dangling `MonitorUpdateCompletionAction`s for forwarded payments should be exceedingly rare. This also adds an upgrade test to test a slightly convoluted version of this scenario. Trivial conflicts addressed in: * lightning/src/ln/channelmanager.rs

TheBlueMatt · 2025-05-23T17:59:43Z

Oops, ugh, bad rebase, fixed:

$ git diff-tree -U1 fe127ccdc0 bc9ffe4433
diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs
index df4cb27604..1f4e7c2415 100644
--- a/fuzz/src/chanmon_consistency.rs+++ b/fuzz/src/chanmon_consistency.rs@@ -393,3 +393,3 @@ impl SignerProvider for KeyProvider {
- Ok(TestChannelSigner::new_with_revoked(inner, state, false))+ Ok(TestChannelSigner::new_with_revoked(inner, state, false, false))
}
diff --git a/fuzz/src/full_stack.rs b/fuzz/src/full_stack.rs
index 88e9eadaf1..143f69f160 100644
--- a/fuzz/src/full_stack.rs+++ b/fuzz/src/full_stack.rs@@ -524,2 +524,3 @@ impl SignerProvider for KeyProvider {
false,
+ false,
)

At all times, the funder's balance should cover the commitment transaction fee, any non-zero-value anchors, and the fundee-selected channel reserve. Prior to this commit, we would allow the funder to dip into its reserve to pay for the two 330sat anchors. LDK sets reserves to at least 1000sat, so two 330 sat anchors would never overdraw this reserve. We now prevent any such dips, and ensure that the funder can pay for the complete sum of the transaction fee, the anchors, and the reserve. Substantial conflicts resulted in the `channel.rs` parts of this patch being rewriten. The `functional_tests.rs` changes also conflicted but were re-applied to the proper file.

TheBlueMatt · 2025-05-23T19:33:50Z

Also added a backport of #3796

TheBlueMatt · 2025-05-23T14:04:54Z

Backport of #3772, two commits from #3584, and #3790. The two commits from #3584 are kinda awkward (testing upgrade from 0.1 to 0.1) but it seems weirder to drop the test in a backport and it exposes a few more methods as public (which may be helpful for later upgrade tests from 0.1 to git/0.2/etc).

EDIT: Plus #3796

When we begin claiming a payment, we move the tracking of it from `claimable_payments` to `claiming_payments`. This ensures we only ever have one payment which is in the process of being claimed with a given payment hash at a time and lets us keep track of when all parts have been claimed with their `ChannelMonitor`s. However, on startup, we check that failing to move a payment from `claimable_payments` to `claiming_payments` implies that it is not present in `claiming_payments`. This is fine if the payment doesn't exist, but if the payment has already started being claimed, this will fail and we'll refuse to deserialize the `ChannelManager` (with a `debug_assert` failure in debug mode). Here we resolve this by checking if a payment is already being claimed before we attempt to initiate claiming and skip the failing check in that case.

ldk-reviews-bot · 2025-05-23T14:04:57Z

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

wpaulino · 2025-05-23T16:32:48Z

Should probably fix the fuzz build before merging

The signer we use in tests tracks the state of the channel and refuses to sign when the channel attempts an invalid state transition. In the next commit, however, we'll add an upgrade test which will fail these checks as the the state won't get copied from previous versions of LDK to this version. Thus, here, we add the ability to disable all state-based checks in the signer.

One major hole in our test coverage historically has been tests covering upgrades or downgrades across LDK versions. Luckily, these aren't particularly hard to write as cargo lets us depend on previous versions of the `lightning` crate directly, which we can use in tests. Here we add a simple initial test of upgrading from LDK 0.1 while there's a pending payment to be claimed.

In 93b4479 we fixed an issue which could cause a `ChannelMonitorUpdate` to get marked as blocked on itself, leading to an eventual force-closure. One potential side-effect of that issue, however, is that any further `ChannelMonitorUpdate`s to the same channel while it is blocked will not have any post-update actions processed (as there is a pending blocked `ChannelMonitorUpdate` sitting in the channel). This can leave a dangling `MonitorUpdateCompletionAction` sitting around even after the channel is closed. In 0.1, because `ChannelMonitorUpdate`s to closed channels were finally fully tracked, we started enforcing that any post-update completion action we had on startup corresponded to a peer entry, while at the same time no longer creating peer entries just because we had serialized one in the data we were loading (only creating them if we had channel(s) or a `ChannelMonitor`). This can cause some `ChannelManager` to no longer deserialize on 0.1 as we might have a left-over dangling `MonitorUpdateCompletionAction` and will no longer always have a peer entry just because of it. Here we fix this issue by specifically checking for dangling `MonitorUpdateCompletionAction::PaymentClaim` entries and dropping them if there is no corresponding channel or peer state entry. We only check for `PaymentClaimed` actions rather than allowing for any dangling actions as 93b4479 was only triggerable with MPP claims, so dangling `MonitorUpdateCompletionAction`s for forwarded payments should be exceedingly rare. This also adds an upgrade test to test a slightly convoluted version of this scenario. Trivial conflicts addressed in: * lightning/src/ln/channelmanager.rs

TheBlueMatt · 2025-05-23T17:59:43Z

Oops, ugh, bad rebase, fixed:

$ git diff-tree -U1 fe127ccdc0 bc9ffe4433
diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs
index df4cb27604..1f4e7c2415 100644
--- a/fuzz/src/chanmon_consistency.rs+++ b/fuzz/src/chanmon_consistency.rs@@ -393,3 +393,3 @@ impl SignerProvider for KeyProvider {
- Ok(TestChannelSigner::new_with_revoked(inner, state, false))+ Ok(TestChannelSigner::new_with_revoked(inner, state, false, false))
}
diff --git a/fuzz/src/full_stack.rs b/fuzz/src/full_stack.rs
index 88e9eadaf1..143f69f160 100644
--- a/fuzz/src/full_stack.rs+++ b/fuzz/src/full_stack.rs@@ -524,2 +524,3 @@ impl SignerProvider for KeyProvider {
false,
+ false,
)

At all times, the funder's balance should cover the commitment transaction fee, any non-zero-value anchors, and the fundee-selected channel reserve. Prior to this commit, we would allow the funder to dip into its reserve to pay for the two 330sat anchors. LDK sets reserves to at least 1000sat, so two 330 sat anchors would never overdraw this reserve. We now prevent any such dips, and ensure that the funder can pay for the complete sum of the transaction fee, the anchors, and the reserve. Substantial conflicts resulted in the `channel.rs` parts of this patch being rewriten. The `functional_tests.rs` changes also conflicted but were re-applied to the proper file.

TheBlueMatt · 2025-05-23T19:33:50Z

Also added a backport of #3796

TheBlueMatt · 2025-05-23T14:04:54Z

Backport of #3772, two commits from #3584, and #3790. The two commits from #3584 are kinda awkward (testing upgrade from 0.1 to 0.1) but it seems weirder to drop the test in a backport and it exposes a few more methods as public (which may be helpful for later upgrade tests from 0.1 to git/0.2/etc).

EDIT: Plus #3796

When we begin claiming a payment, we move the tracking of it from `claimable_payments` to `claiming_payments`. This ensures we only ever have one payment which is in the process of being claimed with a given payment hash at a time and lets us keep track of when all parts have been claimed with their `ChannelMonitor`s. However, on startup, we check that failing to move a payment from `claimable_payments` to `claiming_payments` implies that it is not present in `claiming_payments`. This is fine if the payment doesn't exist, but if the payment has already started being claimed, this will fail and we'll refuse to deserialize the `ChannelManager` (with a `debug_assert` failure in debug mode). Here we resolve this by checking if a payment is already being claimed before we attempt to initiate claiming and skip the failing check in that case.

ldk-reviews-bot · 2025-05-23T14:04:57Z

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

wpaulino · 2025-05-23T16:32:48Z

Should probably fix the fuzz build before merging

The signer we use in tests tracks the state of the channel and refuses to sign when the channel attempts an invalid state transition. In the next commit, however, we'll add an upgrade test which will fail these checks as the the state won't get copied from previous versions of LDK to this version. Thus, here, we add the ability to disable all state-based checks in the signer.

One major hole in our test coverage historically has been tests covering upgrades or downgrades across LDK versions. Luckily, these aren't particularly hard to write as cargo lets us depend on previous versions of the `lightning` crate directly, which we can use in tests. Here we add a simple initial test of upgrading from LDK 0.1 while there's a pending payment to be claimed.

In 93b4479 we fixed an issue which could cause a `ChannelMonitorUpdate` to get marked as blocked on itself, leading to an eventual force-closure. One potential side-effect of that issue, however, is that any further `ChannelMonitorUpdate`s to the same channel while it is blocked will not have any post-update actions processed (as there is a pending blocked `ChannelMonitorUpdate` sitting in the channel). This can leave a dangling `MonitorUpdateCompletionAction` sitting around even after the channel is closed. In 0.1, because `ChannelMonitorUpdate`s to closed channels were finally fully tracked, we started enforcing that any post-update completion action we had on startup corresponded to a peer entry, while at the same time no longer creating peer entries just because we had serialized one in the data we were loading (only creating them if we had channel(s) or a `ChannelMonitor`). This can cause some `ChannelManager` to no longer deserialize on 0.1 as we might have a left-over dangling `MonitorUpdateCompletionAction` and will no longer always have a peer entry just because of it. Here we fix this issue by specifically checking for dangling `MonitorUpdateCompletionAction::PaymentClaim` entries and dropping them if there is no corresponding channel or peer state entry. We only check for `PaymentClaimed` actions rather than allowing for any dangling actions as 93b4479 was only triggerable with MPP claims, so dangling `MonitorUpdateCompletionAction`s for forwarded payments should be exceedingly rare. This also adds an upgrade test to test a slightly convoluted version of this scenario. Trivial conflicts addressed in: * lightning/src/ln/channelmanager.rs

TheBlueMatt · 2025-05-23T17:59:43Z

Oops, ugh, bad rebase, fixed:

$ git diff-tree -U1 fe127ccdc0 bc9ffe4433
diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs
index df4cb27604..1f4e7c2415 100644
--- a/fuzz/src/chanmon_consistency.rs+++ b/fuzz/src/chanmon_consistency.rs@@ -393,3 +393,3 @@ impl SignerProvider for KeyProvider {
- Ok(TestChannelSigner::new_with_revoked(inner, state, false))+ Ok(TestChannelSigner::new_with_revoked(inner, state, false, false))
}
diff --git a/fuzz/src/full_stack.rs b/fuzz/src/full_stack.rs
index 88e9eadaf1..143f69f160 100644
--- a/fuzz/src/full_stack.rs+++ b/fuzz/src/full_stack.rs@@ -524,2 +524,3 @@ impl SignerProvider for KeyProvider {
false,
+ false,
)

At all times, the funder's balance should cover the commitment transaction fee, any non-zero-value anchors, and the fundee-selected channel reserve. Prior to this commit, we would allow the funder to dip into its reserve to pay for the two 330sat anchors. LDK sets reserves to at least 1000sat, so two 330 sat anchors would never overdraw this reserve. We now prevent any such dips, and ensure that the funder can pay for the complete sum of the transaction fee, the anchors, and the reserve. Substantial conflicts resulted in the `channel.rs` parts of this patch being rewriten. The `functional_tests.rs` changes also conflicted but were re-applied to the proper file.

TheBlueMatt · 2025-05-23T19:33:50Z

Also added a backport of #3796

TheBlueMatt · 2025-05-23T14:04:54Z

Backport of #3772, two commits from #3584, and #3790. The two commits from #3584 are kinda awkward (testing upgrade from 0.1 to 0.1) but it seems weirder to drop the test in a backport and it exposes a few more methods as public (which may be helpful for later upgrade tests from 0.1 to git/0.2/etc).

EDIT: Plus #3796

When we begin claiming a payment, we move the tracking of it from `claimable_payments` to `claiming_payments`. This ensures we only ever have one payment which is in the process of being claimed with a given payment hash at a time and lets us keep track of when all parts have been claimed with their `ChannelMonitor`s. However, on startup, we check that failing to move a payment from `claimable_payments` to `claiming_payments` implies that it is not present in `claiming_payments`. This is fine if the payment doesn't exist, but if the payment has already started being claimed, this will fail and we'll refuse to deserialize the `ChannelManager` (with a `debug_assert` failure in debug mode). Here we resolve this by checking if a payment is already being claimed before we attempt to initiate claiming and skip the failing check in that case.

ldk-reviews-bot · 2025-05-23T14:04:57Z

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

wpaulino · 2025-05-23T16:32:48Z

Should probably fix the fuzz build before merging

The signer we use in tests tracks the state of the channel and refuses to sign when the channel attempts an invalid state transition. In the next commit, however, we'll add an upgrade test which will fail these checks as the the state won't get copied from previous versions of LDK to this version. Thus, here, we add the ability to disable all state-based checks in the signer.

One major hole in our test coverage historically has been tests covering upgrades or downgrades across LDK versions. Luckily, these aren't particularly hard to write as cargo lets us depend on previous versions of the `lightning` crate directly, which we can use in tests. Here we add a simple initial test of upgrading from LDK 0.1 while there's a pending payment to be claimed.

In 93b4479 we fixed an issue which could cause a `ChannelMonitorUpdate` to get marked as blocked on itself, leading to an eventual force-closure. One potential side-effect of that issue, however, is that any further `ChannelMonitorUpdate`s to the same channel while it is blocked will not have any post-update actions processed (as there is a pending blocked `ChannelMonitorUpdate` sitting in the channel). This can leave a dangling `MonitorUpdateCompletionAction` sitting around even after the channel is closed. In 0.1, because `ChannelMonitorUpdate`s to closed channels were finally fully tracked, we started enforcing that any post-update completion action we had on startup corresponded to a peer entry, while at the same time no longer creating peer entries just because we had serialized one in the data we were loading (only creating them if we had channel(s) or a `ChannelMonitor`). This can cause some `ChannelManager` to no longer deserialize on 0.1 as we might have a left-over dangling `MonitorUpdateCompletionAction` and will no longer always have a peer entry just because of it. Here we fix this issue by specifically checking for dangling `MonitorUpdateCompletionAction::PaymentClaim` entries and dropping them if there is no corresponding channel or peer state entry. We only check for `PaymentClaimed` actions rather than allowing for any dangling actions as 93b4479 was only triggerable with MPP claims, so dangling `MonitorUpdateCompletionAction`s for forwarded payments should be exceedingly rare. This also adds an upgrade test to test a slightly convoluted version of this scenario. Trivial conflicts addressed in: * lightning/src/ln/channelmanager.rs

TheBlueMatt · 2025-05-23T17:59:43Z

Oops, ugh, bad rebase, fixed:

$ git diff-tree -U1 fe127ccdc0 bc9ffe4433
diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs
index df4cb27604..1f4e7c2415 100644
--- a/fuzz/src/chanmon_consistency.rs+++ b/fuzz/src/chanmon_consistency.rs@@ -393,3 +393,3 @@ impl SignerProvider for KeyProvider {
- Ok(TestChannelSigner::new_with_revoked(inner, state, false))+ Ok(TestChannelSigner::new_with_revoked(inner, state, false, false))
}
diff --git a/fuzz/src/full_stack.rs b/fuzz/src/full_stack.rs
index 88e9eadaf1..143f69f160 100644
--- a/fuzz/src/full_stack.rs+++ b/fuzz/src/full_stack.rs@@ -524,2 +524,3 @@ impl SignerProvider for KeyProvider {
false,
+ false,
)

At all times, the funder's balance should cover the commitment transaction fee, any non-zero-value anchors, and the fundee-selected channel reserve. Prior to this commit, we would allow the funder to dip into its reserve to pay for the two 330sat anchors. LDK sets reserves to at least 1000sat, so two 330 sat anchors would never overdraw this reserve. We now prevent any such dips, and ensure that the funder can pay for the complete sum of the transaction fee, the anchors, and the reserve. Substantial conflicts resulted in the `channel.rs` parts of this patch being rewriten. The `functional_tests.rs` changes also conflicted but were re-applied to the proper file.

TheBlueMatt · 2025-05-23T19:33:50Z

Also added a backport of #3796

TheBlueMatt · 2025-05-23T14:04:54Z

Backport of #3772, two commits from #3584, and #3790. The two commits from #3584 are kinda awkward (testing upgrade from 0.1 to 0.1) but it seems weirder to drop the test in a backport and it exposes a few more methods as public (which may be helpful for later upgrade tests from 0.1 to git/0.2/etc).

EDIT: Plus #3796

When we begin claiming a payment, we move the tracking of it from `claimable_payments` to `claiming_payments`. This ensures we only ever have one payment which is in the process of being claimed with a given payment hash at a time and lets us keep track of when all parts have been claimed with their `ChannelMonitor`s. However, on startup, we check that failing to move a payment from `claimable_payments` to `claiming_payments` implies that it is not present in `claiming_payments`. This is fine if the payment doesn't exist, but if the payment has already started being claimed, this will fail and we'll refuse to deserialize the `ChannelManager` (with a `debug_assert` failure in debug mode). Here we resolve this by checking if a payment is already being claimed before we attempt to initiate claiming and skip the failing check in that case.

ldk-reviews-bot · 2025-05-23T14:04:57Z

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

wpaulino · 2025-05-23T16:32:48Z

Should probably fix the fuzz build before merging

The signer we use in tests tracks the state of the channel and refuses to sign when the channel attempts an invalid state transition. In the next commit, however, we'll add an upgrade test which will fail these checks as the the state won't get copied from previous versions of LDK to this version. Thus, here, we add the ability to disable all state-based checks in the signer.

One major hole in our test coverage historically has been tests covering upgrades or downgrades across LDK versions. Luckily, these aren't particularly hard to write as cargo lets us depend on previous versions of the `lightning` crate directly, which we can use in tests. Here we add a simple initial test of upgrading from LDK 0.1 while there's a pending payment to be claimed.

In 93b4479 we fixed an issue which could cause a `ChannelMonitorUpdate` to get marked as blocked on itself, leading to an eventual force-closure. One potential side-effect of that issue, however, is that any further `ChannelMonitorUpdate`s to the same channel while it is blocked will not have any post-update actions processed (as there is a pending blocked `ChannelMonitorUpdate` sitting in the channel). This can leave a dangling `MonitorUpdateCompletionAction` sitting around even after the channel is closed. In 0.1, because `ChannelMonitorUpdate`s to closed channels were finally fully tracked, we started enforcing that any post-update completion action we had on startup corresponded to a peer entry, while at the same time no longer creating peer entries just because we had serialized one in the data we were loading (only creating them if we had channel(s) or a `ChannelMonitor`). This can cause some `ChannelManager` to no longer deserialize on 0.1 as we might have a left-over dangling `MonitorUpdateCompletionAction` and will no longer always have a peer entry just because of it. Here we fix this issue by specifically checking for dangling `MonitorUpdateCompletionAction::PaymentClaim` entries and dropping them if there is no corresponding channel or peer state entry. We only check for `PaymentClaimed` actions rather than allowing for any dangling actions as 93b4479 was only triggerable with MPP claims, so dangling `MonitorUpdateCompletionAction`s for forwarded payments should be exceedingly rare. This also adds an upgrade test to test a slightly convoluted version of this scenario. Trivial conflicts addressed in: * lightning/src/ln/channelmanager.rs

TheBlueMatt · 2025-05-23T17:59:43Z

Oops, ugh, bad rebase, fixed:

$ git diff-tree -U1 fe127ccdc0 bc9ffe4433
diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs
index df4cb27604..1f4e7c2415 100644
--- a/fuzz/src/chanmon_consistency.rs+++ b/fuzz/src/chanmon_consistency.rs@@ -393,3 +393,3 @@ impl SignerProvider for KeyProvider {
- Ok(TestChannelSigner::new_with_revoked(inner, state, false))+ Ok(TestChannelSigner::new_with_revoked(inner, state, false, false))
}
diff --git a/fuzz/src/full_stack.rs b/fuzz/src/full_stack.rs
index 88e9eadaf1..143f69f160 100644
--- a/fuzz/src/full_stack.rs+++ b/fuzz/src/full_stack.rs@@ -524,2 +524,3 @@ impl SignerProvider for KeyProvider {
false,
+ false,
)

At all times, the funder's balance should cover the commitment transaction fee, any non-zero-value anchors, and the fundee-selected channel reserve. Prior to this commit, we would allow the funder to dip into its reserve to pay for the two 330sat anchors. LDK sets reserves to at least 1000sat, so two 330 sat anchors would never overdraw this reserve. We now prevent any such dips, and ensure that the funder can pay for the complete sum of the transaction fee, the anchors, and the reserve. Substantial conflicts resulted in the `channel.rs` parts of this patch being rewriten. The `functional_tests.rs` changes also conflicted but were re-applied to the proper file.

TheBlueMatt · 2025-05-23T19:33:50Z

Also added a backport of #3796

TheBlueMatt · 2025-05-23T14:04:54Z

Backport of #3772, two commits from #3584, and #3790. The two commits from #3584 are kinda awkward (testing upgrade from 0.1 to 0.1) but it seems weirder to drop the test in a backport and it exposes a few more methods as public (which may be helpful for later upgrade tests from 0.1 to git/0.2/etc).

EDIT: Plus #3796

When we begin claiming a payment, we move the tracking of it from `claimable_payments` to `claiming_payments`. This ensures we only ever have one payment which is in the process of being claimed with a given payment hash at a time and lets us keep track of when all parts have been claimed with their `ChannelMonitor`s. However, on startup, we check that failing to move a payment from `claimable_payments` to `claiming_payments` implies that it is not present in `claiming_payments`. This is fine if the payment doesn't exist, but if the payment has already started being claimed, this will fail and we'll refuse to deserialize the `ChannelManager` (with a `debug_assert` failure in debug mode). Here we resolve this by checking if a payment is already being claimed before we attempt to initiate claiming and skip the failing check in that case.

ldk-reviews-bot · 2025-05-23T14:04:57Z

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

wpaulino · 2025-05-23T16:32:48Z

Should probably fix the fuzz build before merging

The signer we use in tests tracks the state of the channel and refuses to sign when the channel attempts an invalid state transition. In the next commit, however, we'll add an upgrade test which will fail these checks as the the state won't get copied from previous versions of LDK to this version. Thus, here, we add the ability to disable all state-based checks in the signer.

One major hole in our test coverage historically has been tests covering upgrades or downgrades across LDK versions. Luckily, these aren't particularly hard to write as cargo lets us depend on previous versions of the `lightning` crate directly, which we can use in tests. Here we add a simple initial test of upgrading from LDK 0.1 while there's a pending payment to be claimed.

In 93b4479 we fixed an issue which could cause a `ChannelMonitorUpdate` to get marked as blocked on itself, leading to an eventual force-closure. One potential side-effect of that issue, however, is that any further `ChannelMonitorUpdate`s to the same channel while it is blocked will not have any post-update actions processed (as there is a pending blocked `ChannelMonitorUpdate` sitting in the channel). This can leave a dangling `MonitorUpdateCompletionAction` sitting around even after the channel is closed. In 0.1, because `ChannelMonitorUpdate`s to closed channels were finally fully tracked, we started enforcing that any post-update completion action we had on startup corresponded to a peer entry, while at the same time no longer creating peer entries just because we had serialized one in the data we were loading (only creating them if we had channel(s) or a `ChannelMonitor`). This can cause some `ChannelManager` to no longer deserialize on 0.1 as we might have a left-over dangling `MonitorUpdateCompletionAction` and will no longer always have a peer entry just because of it. Here we fix this issue by specifically checking for dangling `MonitorUpdateCompletionAction::PaymentClaim` entries and dropping them if there is no corresponding channel or peer state entry. We only check for `PaymentClaimed` actions rather than allowing for any dangling actions as 93b4479 was only triggerable with MPP claims, so dangling `MonitorUpdateCompletionAction`s for forwarded payments should be exceedingly rare. This also adds an upgrade test to test a slightly convoluted version of this scenario. Trivial conflicts addressed in: * lightning/src/ln/channelmanager.rs

TheBlueMatt · 2025-05-23T17:59:43Z

Oops, ugh, bad rebase, fixed:

$ git diff-tree -U1 fe127ccdc0 bc9ffe4433
diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs
index df4cb27604..1f4e7c2415 100644
--- a/fuzz/src/chanmon_consistency.rs+++ b/fuzz/src/chanmon_consistency.rs@@ -393,3 +393,3 @@ impl SignerProvider for KeyProvider {
- Ok(TestChannelSigner::new_with_revoked(inner, state, false))+ Ok(TestChannelSigner::new_with_revoked(inner, state, false, false))
}
diff --git a/fuzz/src/full_stack.rs b/fuzz/src/full_stack.rs
index 88e9eadaf1..143f69f160 100644
--- a/fuzz/src/full_stack.rs+++ b/fuzz/src/full_stack.rs@@ -524,2 +524,3 @@ impl SignerProvider for KeyProvider {
false,
+ false,
)

At all times, the funder's balance should cover the commitment transaction fee, any non-zero-value anchors, and the fundee-selected channel reserve. Prior to this commit, we would allow the funder to dip into its reserve to pay for the two 330sat anchors. LDK sets reserves to at least 1000sat, so two 330 sat anchors would never overdraw this reserve. We now prevent any such dips, and ensure that the funder can pay for the complete sum of the transaction fee, the anchors, and the reserve. Substantial conflicts resulted in the `channel.rs` parts of this patch being rewriten. The `functional_tests.rs` changes also conflicted but were re-applied to the proper file.

TheBlueMatt · 2025-05-23T19:33:50Z

Also added a backport of #3796

TheBlueMatt · 2025-05-23T14:04:54Z

Backport of #3772, two commits from #3584, and #3790. The two commits from #3584 are kinda awkward (testing upgrade from 0.1 to 0.1) but it seems weirder to drop the test in a backport and it exposes a few more methods as public (which may be helpful for later upgrade tests from 0.1 to git/0.2/etc).

EDIT: Plus #3796

When we begin claiming a payment, we move the tracking of it from `claimable_payments` to `claiming_payments`. This ensures we only ever have one payment which is in the process of being claimed with a given payment hash at a time and lets us keep track of when all parts have been claimed with their `ChannelMonitor`s. However, on startup, we check that failing to move a payment from `claimable_payments` to `claiming_payments` implies that it is not present in `claiming_payments`. This is fine if the payment doesn't exist, but if the payment has already started being claimed, this will fail and we'll refuse to deserialize the `ChannelManager` (with a `debug_assert` failure in debug mode). Here we resolve this by checking if a payment is already being claimed before we attempt to initiate claiming and skip the failing check in that case.

ldk-reviews-bot · 2025-05-23T14:04:57Z

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

wpaulino · 2025-05-23T16:32:48Z

Should probably fix the fuzz build before merging

The signer we use in tests tracks the state of the channel and refuses to sign when the channel attempts an invalid state transition. In the next commit, however, we'll add an upgrade test which will fail these checks as the the state won't get copied from previous versions of LDK to this version. Thus, here, we add the ability to disable all state-based checks in the signer.

One major hole in our test coverage historically has been tests covering upgrades or downgrades across LDK versions. Luckily, these aren't particularly hard to write as cargo lets us depend on previous versions of the `lightning` crate directly, which we can use in tests. Here we add a simple initial test of upgrading from LDK 0.1 while there's a pending payment to be claimed.

In 93b4479 we fixed an issue which could cause a `ChannelMonitorUpdate` to get marked as blocked on itself, leading to an eventual force-closure. One potential side-effect of that issue, however, is that any further `ChannelMonitorUpdate`s to the same channel while it is blocked will not have any post-update actions processed (as there is a pending blocked `ChannelMonitorUpdate` sitting in the channel). This can leave a dangling `MonitorUpdateCompletionAction` sitting around even after the channel is closed. In 0.1, because `ChannelMonitorUpdate`s to closed channels were finally fully tracked, we started enforcing that any post-update completion action we had on startup corresponded to a peer entry, while at the same time no longer creating peer entries just because we had serialized one in the data we were loading (only creating them if we had channel(s) or a `ChannelMonitor`). This can cause some `ChannelManager` to no longer deserialize on 0.1 as we might have a left-over dangling `MonitorUpdateCompletionAction` and will no longer always have a peer entry just because of it. Here we fix this issue by specifically checking for dangling `MonitorUpdateCompletionAction::PaymentClaim` entries and dropping them if there is no corresponding channel or peer state entry. We only check for `PaymentClaimed` actions rather than allowing for any dangling actions as 93b4479 was only triggerable with MPP claims, so dangling `MonitorUpdateCompletionAction`s for forwarded payments should be exceedingly rare. This also adds an upgrade test to test a slightly convoluted version of this scenario. Trivial conflicts addressed in: * lightning/src/ln/channelmanager.rs

TheBlueMatt · 2025-05-23T17:59:43Z

Oops, ugh, bad rebase, fixed:

$ git diff-tree -U1 fe127ccdc0 bc9ffe4433
diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs
index df4cb27604..1f4e7c2415 100644
--- a/fuzz/src/chanmon_consistency.rs+++ b/fuzz/src/chanmon_consistency.rs@@ -393,3 +393,3 @@ impl SignerProvider for KeyProvider {
- Ok(TestChannelSigner::new_with_revoked(inner, state, false))+ Ok(TestChannelSigner::new_with_revoked(inner, state, false, false))
}
diff --git a/fuzz/src/full_stack.rs b/fuzz/src/full_stack.rs
index 88e9eadaf1..143f69f160 100644
--- a/fuzz/src/full_stack.rs+++ b/fuzz/src/full_stack.rs@@ -524,2 +524,3 @@ impl SignerProvider for KeyProvider {
false,
+ false,
)

At all times, the funder's balance should cover the commitment transaction fee, any non-zero-value anchors, and the fundee-selected channel reserve. Prior to this commit, we would allow the funder to dip into its reserve to pay for the two 330sat anchors. LDK sets reserves to at least 1000sat, so two 330 sat anchors would never overdraw this reserve. We now prevent any such dips, and ensure that the funder can pay for the complete sum of the transaction fee, the anchors, and the reserve. Substantial conflicts resulted in the `channel.rs` parts of this patch being rewriten. The `functional_tests.rs` changes also conflicted but were re-applied to the proper file.

TheBlueMatt · 2025-05-23T19:33:50Z

Also added a backport of #3796

TheBlueMatt · 2025-05-23T14:04:54Z

Backport of #3772, two commits from #3584, and #3790. The two commits from #3584 are kinda awkward (testing upgrade from 0.1 to 0.1) but it seems weirder to drop the test in a backport and it exposes a few more methods as public (which may be helpful for later upgrade tests from 0.1 to git/0.2/etc).

EDIT: Plus #3796

When we begin claiming a payment, we move the tracking of it from `claimable_payments` to `claiming_payments`. This ensures we only ever have one payment which is in the process of being claimed with a given payment hash at a time and lets us keep track of when all parts have been claimed with their `ChannelMonitor`s. However, on startup, we check that failing to move a payment from `claimable_payments` to `claiming_payments` implies that it is not present in `claiming_payments`. This is fine if the payment doesn't exist, but if the payment has already started being claimed, this will fail and we'll refuse to deserialize the `ChannelManager` (with a `debug_assert` failure in debug mode). Here we resolve this by checking if a payment is already being claimed before we attempt to initiate claiming and skip the failing check in that case.

ldk-reviews-bot · 2025-05-23T14:04:57Z

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

wpaulino · 2025-05-23T16:32:48Z

Should probably fix the fuzz build before merging

The signer we use in tests tracks the state of the channel and refuses to sign when the channel attempts an invalid state transition. In the next commit, however, we'll add an upgrade test which will fail these checks as the the state won't get copied from previous versions of LDK to this version. Thus, here, we add the ability to disable all state-based checks in the signer.

One major hole in our test coverage historically has been tests covering upgrades or downgrades across LDK versions. Luckily, these aren't particularly hard to write as cargo lets us depend on previous versions of the `lightning` crate directly, which we can use in tests. Here we add a simple initial test of upgrading from LDK 0.1 while there's a pending payment to be claimed.

In 93b4479 we fixed an issue which could cause a `ChannelMonitorUpdate` to get marked as blocked on itself, leading to an eventual force-closure. One potential side-effect of that issue, however, is that any further `ChannelMonitorUpdate`s to the same channel while it is blocked will not have any post-update actions processed (as there is a pending blocked `ChannelMonitorUpdate` sitting in the channel). This can leave a dangling `MonitorUpdateCompletionAction` sitting around even after the channel is closed. In 0.1, because `ChannelMonitorUpdate`s to closed channels were finally fully tracked, we started enforcing that any post-update completion action we had on startup corresponded to a peer entry, while at the same time no longer creating peer entries just because we had serialized one in the data we were loading (only creating them if we had channel(s) or a `ChannelMonitor`). This can cause some `ChannelManager` to no longer deserialize on 0.1 as we might have a left-over dangling `MonitorUpdateCompletionAction` and will no longer always have a peer entry just because of it. Here we fix this issue by specifically checking for dangling `MonitorUpdateCompletionAction::PaymentClaim` entries and dropping them if there is no corresponding channel or peer state entry. We only check for `PaymentClaimed` actions rather than allowing for any dangling actions as 93b4479 was only triggerable with MPP claims, so dangling `MonitorUpdateCompletionAction`s for forwarded payments should be exceedingly rare. This also adds an upgrade test to test a slightly convoluted version of this scenario. Trivial conflicts addressed in: * lightning/src/ln/channelmanager.rs

TheBlueMatt · 2025-05-23T17:59:43Z

Oops, ugh, bad rebase, fixed:

$ git diff-tree -U1 fe127ccdc0 bc9ffe4433
diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs
index df4cb27604..1f4e7c2415 100644
--- a/fuzz/src/chanmon_consistency.rs+++ b/fuzz/src/chanmon_consistency.rs@@ -393,3 +393,3 @@ impl SignerProvider for KeyProvider {
- Ok(TestChannelSigner::new_with_revoked(inner, state, false))+ Ok(TestChannelSigner::new_with_revoked(inner, state, false, false))
}
diff --git a/fuzz/src/full_stack.rs b/fuzz/src/full_stack.rs
index 88e9eadaf1..143f69f160 100644
--- a/fuzz/src/full_stack.rs+++ b/fuzz/src/full_stack.rs@@ -524,2 +524,3 @@ impl SignerProvider for KeyProvider {
false,
+ false,
)

At all times, the funder's balance should cover the commitment transaction fee, any non-zero-value anchors, and the fundee-selected channel reserve. Prior to this commit, we would allow the funder to dip into its reserve to pay for the two 330sat anchors. LDK sets reserves to at least 1000sat, so two 330 sat anchors would never overdraw this reserve. We now prevent any such dips, and ensure that the funder can pay for the complete sum of the transaction fee, the anchors, and the reserve. Substantial conflicts resulted in the `channel.rs` parts of this patch being rewriten. The `functional_tests.rs` changes also conflicted but were re-applied to the proper file.

TheBlueMatt · 2025-05-23T19:33:50Z

Also added a backport of #3796

This was referenced May 23, 2025
Do not fail to load ChannelManager when we see claiming payments #3772
Merged
Read ChannelManager even if we have no-peer post-update actions #3790
Merged
Add a simple test of upgrading from LDK 0.1 and correct in_flight_monitor_updates on upgrade #3584
Merged

ldk-reviews-bot requested a review from valentinewallace May 23, 2025 14:15