Do not fail to load `ChannelManager` when we see claiming payments by TheBlueMatt · Pull Request #3772 · lightningdevkit/rust-lightning

TheBlueMatt · 2025-05-12T02:03:28Z

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

However, on startup, we check that failing to move a payment from claimable_payments to claiming_payments we check 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-12T02:03:30Z

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

MaxFangX · 2025-05-12T02:33:30Z

Thanks for implementing this so quickly!

valentinewallace · 2025-05-12T16:16:44Z

+
 let mut channels_without_preimage = payment_claim.mpp_parts.iter()
 .map(|htlc_info| (htlc_info.counterparty_node_id, htlc_info.funding_txo, htlc_info.channel_id))
 .collect::<Vec<_>>();


To confirm, the failure occurred at the debug_assert "Entry was added in begin_claiming_payment" ~20 lines down, right?

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.

TheBlueMatt · 2025-05-12T19:18:32Z

Tweaked the commit message

TheBlueMatt · 2025-05-23T14:07:26Z

Backported in #3794

TheBlueMatt · 2025-05-12T02:03:28Z

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

However, on startup, we check that failing to move a payment from claimable_payments to claiming_payments we check 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-12T02:03:30Z

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

MaxFangX · 2025-05-12T02:33:30Z

Thanks for implementing this so quickly!

valentinewallace · 2025-05-12T16:16:44Z

+
 let mut channels_without_preimage = payment_claim.mpp_parts.iter()
 .map(|htlc_info| (htlc_info.counterparty_node_id, htlc_info.funding_txo, htlc_info.channel_id))
 .collect::<Vec<_>>();


To confirm, the failure occurred at the debug_assert "Entry was added in begin_claiming_payment" ~20 lines down, right?

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.

TheBlueMatt · 2025-05-12T19:18:32Z

Tweaked the commit message

TheBlueMatt · 2025-05-23T14:07:26Z

Backported in #3794

TheBlueMatt · 2025-05-12T02:03:28Z

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

However, on startup, we check that failing to move a payment from claimable_payments to claiming_payments we check 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-12T02:03:30Z

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

MaxFangX · 2025-05-12T02:33:30Z

Thanks for implementing this so quickly!

valentinewallace · 2025-05-12T16:16:44Z

+
 let mut channels_without_preimage = payment_claim.mpp_parts.iter()
 .map(|htlc_info| (htlc_info.counterparty_node_id, htlc_info.funding_txo, htlc_info.channel_id))
 .collect::<Vec<_>>();


To confirm, the failure occurred at the debug_assert "Entry was added in begin_claiming_payment" ~20 lines down, right?

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.

TheBlueMatt · 2025-05-12T19:18:32Z

Tweaked the commit message

TheBlueMatt · 2025-05-23T14:07:26Z

Backported in #3794

TheBlueMatt · 2025-05-12T02:03:28Z

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

However, on startup, we check that failing to move a payment from claimable_payments to claiming_payments we check 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-12T02:03:30Z

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

MaxFangX · 2025-05-12T02:33:30Z

Thanks for implementing this so quickly!

valentinewallace · 2025-05-12T16:16:44Z

+
 let mut channels_without_preimage = payment_claim.mpp_parts.iter()
 .map(|htlc_info| (htlc_info.counterparty_node_id, htlc_info.funding_txo, htlc_info.channel_id))
 .collect::<Vec<_>>();


To confirm, the failure occurred at the debug_assert "Entry was added in begin_claiming_payment" ~20 lines down, right?

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.

TheBlueMatt · 2025-05-12T19:18:32Z

Tweaked the commit message

TheBlueMatt · 2025-05-23T14:07:26Z

Backported in #3794

TheBlueMatt · 2025-05-12T02:03:28Z

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

However, on startup, we check that failing to move a payment from claimable_payments to claiming_payments we check 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-12T02:03:30Z

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

MaxFangX · 2025-05-12T02:33:30Z

Thanks for implementing this so quickly!

valentinewallace · 2025-05-12T16:16:44Z

+
 let mut channels_without_preimage = payment_claim.mpp_parts.iter()
 .map(|htlc_info| (htlc_info.counterparty_node_id, htlc_info.funding_txo, htlc_info.channel_id))
 .collect::<Vec<_>>();


To confirm, the failure occurred at the debug_assert "Entry was added in begin_claiming_payment" ~20 lines down, right?

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.

TheBlueMatt · 2025-05-12T19:18:32Z

Tweaked the commit message

TheBlueMatt · 2025-05-23T14:07:26Z

Backported in #3794

TheBlueMatt · 2025-05-12T02:03:28Z

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

However, on startup, we check that failing to move a payment from claimable_payments to claiming_payments we check 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-12T02:03:30Z

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

MaxFangX · 2025-05-12T02:33:30Z

Thanks for implementing this so quickly!

valentinewallace · 2025-05-12T16:16:44Z

+
 let mut channels_without_preimage = payment_claim.mpp_parts.iter()
 .map(|htlc_info| (htlc_info.counterparty_node_id, htlc_info.funding_txo, htlc_info.channel_id))
 .collect::<Vec<_>>();


To confirm, the failure occurred at the debug_assert "Entry was added in begin_claiming_payment" ~20 lines down, right?

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.

TheBlueMatt · 2025-05-12T19:18:32Z

Tweaked the commit message

TheBlueMatt · 2025-05-23T14:07:26Z

Backported in #3794

TheBlueMatt · 2025-05-12T02:03:28Z

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

However, on startup, we check that failing to move a payment from claimable_payments to claiming_payments we check 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-12T02:03:30Z

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

MaxFangX · 2025-05-12T02:33:30Z

Thanks for implementing this so quickly!

valentinewallace · 2025-05-12T16:16:44Z

+
 let mut channels_without_preimage = payment_claim.mpp_parts.iter()
 .map(|htlc_info| (htlc_info.counterparty_node_id, htlc_info.funding_txo, htlc_info.channel_id))
 .collect::<Vec<_>>();


To confirm, the failure occurred at the debug_assert "Entry was added in begin_claiming_payment" ~20 lines down, right?

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.

TheBlueMatt · 2025-05-12T19:18:32Z

Tweaked the commit message

TheBlueMatt · 2025-05-23T14:07:26Z

Backported in #3794

TheBlueMatt · 2025-05-12T02:03:28Z

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

However, on startup, we check that failing to move a payment from claimable_payments to claiming_payments we check 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-12T02:03:30Z

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

MaxFangX · 2025-05-12T02:33:30Z

Thanks for implementing this so quickly!

valentinewallace · 2025-05-12T16:16:44Z

+
 let mut channels_without_preimage = payment_claim.mpp_parts.iter()
 .map(|htlc_info| (htlc_info.counterparty_node_id, htlc_info.funding_txo, htlc_info.channel_id))
 .collect::<Vec<_>>();


To confirm, the failure occurred at the debug_assert "Entry was added in begin_claiming_payment" ~20 lines down, right?

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.

TheBlueMatt · 2025-05-12T19:18:32Z

Tweaked the commit message

TheBlueMatt · 2025-05-23T14:07:26Z

Backported in #3794

TheBlueMatt added this to the 0.1.4 milestone May 12, 2025

TheBlueMatt force-pushed the 2025-05-chanman-double-restart-claim-fail branch from bc1a7d0 to d24c7a3Compare May 12, 2025 02:06

ldk-reviews-bot requested a review from wpaulino May 12, 2025 02:14

TheBlueMatt force-pushed the 2025-05-chanman-double-restart-claim-fail branch from d24c7a3 to 47cccb7Compare May 12, 2025 02:19

TheBlueMatt force-pushed the 2025-05-chanman-double-restart-claim-fail branch from 47cccb7 to ce09f6fCompare May 12, 2025 12:24

valentinewallace reviewed May 12, 2025
View reviewed changes

TheBlueMatt force-pushed the 2025-05-chanman-double-restart-claim-fail branch from ce09f6f to 21f829bCompare May 12, 2025 19:18

wpaulino approved these changes May 12, 2025
View reviewed changes

TheBlueMatt requested a review from valentinewallace May 12, 2025 20:43

valentinewallace approved these changes May 13, 2025
View reviewed changes

valentinewallace merged commit db1681b into lightningdevkit:mainMay 13, 2025

TheBlueMatt added the backport 0.1 label May 13, 2025

TheBlueMatt mentioned this pull request May 23, 2025
[0.1] Backports for 0.1.4 #3794
Merged

TheBlueMatt removed the backport 0.1 label May 23, 2025