`OutputSweeper`: Delay pruning until monitors have likely been archived by tnull · Pull Request #3559 · lightningdevkit/rust-lightning

tnull · 2025-01-24T10:25:54Z

Previously, we would prune tracked descriptors once we see a spend hit
ANTI_REORG_DELAY = 6 confirmations. However, this could lead to a
scenario where lingering ChannelMonitors waiting to be archived would
still regenerate and replay Event::SpendableOutputs, i.e., we would
re-add the same (now unspendable due to be actually being already spent)
outputs again after having intially pruned them.

Here, we therefore keep the tracked descriptors around for longer, in
particular at least ARCHIVAL_DELAY_BLOCKS + ANTI_REORG_DELAY = 4038
confirmations, at which point we assume the lingering monitors to have
been likely archived, and it's 'safe' for us to also forget about the
descriptors.

arik-so · 2025-01-24T18:09:14Z

 use core::ops::Deref;

+/// The number of blocks we wait before we prune the tracked spendable outputs.
+pub const PRUNE_DELAY_BLOCKS: u32 = ARCHIVAL_DELAY_BLOCKS + ANTI_REORG_DELAY;


how likely is it that 4032 blocks was insufficient, but 4038 was?

Well, the idea here is that we start pruning after the monitors have been archived. For that to work we need to wait at least 4032 blocks, but of course we're not super sure how soon after the threshold has been met user will actually call archive_fully_resolved_monitors, so I added the reorg delay on top. Could be debatable one way or another.
FWIW, I alternatively considered introducing a new MonitorArchivalComplete event type, but given that we on purpose keep the archival a best-effort thing it seemed wrong to go this way.

pretty unlikely, but holding onto them doesn't really cost us anything, so might as well...

yeah, I would have though max(4032, 6) would have also worked, but given that it indeed costs nothing…

TheBlueMatt

LGTM, one comment.

.. previously we just used the 4032 magic number, here we put it in a `pub const` that is reusable elsewhere.

Previously, we would prune tracked descriptors once we see a spend hit `ANTI_REORG_DELAY = 6` confirmations. However, this could lead to a scenario where lingering `ChannelMonitor`s waiting to be archived would still regenerate and replay `Event::SpendableOutput`s, i.e., we would re-add the same (now unspendable due to be actually being already spent) outputs again after having intially pruned them. Here, we therefore keep the tracked descriptors around for longer, in particular at least `ARCHIVAL_DELAY_BLOCKS + ANTI_REORG_DELAY = 4038` confirmations, at which point we assume the lingering monitors to have been likely archived, and it's 'safe' for us to also forget about the descriptors.

TheBlueMatt · 2025-01-28T15:28:25Z

Backported in #3567

v0.1.1 - Jan 28, 2025 - "Onchain Matters" API Updates =========== * A `ChannelManager::send_payment_with_route` was (re-)added, with semantics similar to `ChannelManager::send_payment` (rather than like the pre-0.1 `send_payent_with_route`, lightningdevkit#3534). * `RawBolt11Invoice::{to,from}_raw` were added (lightningdevkit#3549). Bug Fixes ========= * HTLCs which were forwarded where the inbound edge times out within the next three blocks will have the inbound HTLC failed backwards irrespective of the status of the outbound HTLC. This avoids the peer force-closing the channel (and claiming the inbound edge HTLC on-chain) even if we have not yet managed to claim the outbound edge on chain (lightningdevkit#3556). * On restart, replay of `Event::SpendableOutput`s could have caused `OutputSweeper` to generate double-spending transactions, making it unable to claim any delayed claims. This was resolved by retaining old claims for more than four weeks after they are claimed on-chain to detect replays (lightningdevkit#3559). * Fixed the additional feerate we will pay each time we RBF on-chain claims to match the Bitcoin Core policy (1 sat/vB) instead of 16 sats/vB (lightningdevkit#3457). * Fixed a cased where a custom `Router` which returns an invalid `Route`, provided to `ChannelManager`, can result in an outbound payment remaining pending forever despite no HTLCs being pending (lightningdevkit#3531). Security ======== 0.1.1 fixes a denial-of-service vulnerability allowing channel counterparties to cause force-closure of unrelated channels. * If a malicious channel counterparty force-closes a channel, broadcasting a revoked commitment transaction while the channel at closure time included multiple non-dust forwarded outbound HTLCs with identical payment hashes and amounts, failure to fail the HTLCs backwards could cause the channels on which we recieved the corresponding inbound HTLCs to be force-closed. Note that we'll receive, at a minimum, the malicious counterparty's reserve value when they broadcast the stale commitment (lightningdevkit#3556). Thanks to Matt Morehouse for reporting this issue.

tnull · 2025-01-24T10:25:54Z

Previously, we would prune tracked descriptors once we see a spend hit
ANTI_REORG_DELAY = 6 confirmations. However, this could lead to a
scenario where lingering ChannelMonitors waiting to be archived would
still regenerate and replay Event::SpendableOutputs, i.e., we would
re-add the same (now unspendable due to be actually being already spent)
outputs again after having intially pruned them.

Here, we therefore keep the tracked descriptors around for longer, in
particular at least ARCHIVAL_DELAY_BLOCKS + ANTI_REORG_DELAY = 4038
confirmations, at which point we assume the lingering monitors to have
been likely archived, and it's 'safe' for us to also forget about the
descriptors.

arik-so · 2025-01-24T18:09:14Z

 use core::ops::Deref;

+/// The number of blocks we wait before we prune the tracked spendable outputs.
+pub const PRUNE_DELAY_BLOCKS: u32 = ARCHIVAL_DELAY_BLOCKS + ANTI_REORG_DELAY;


how likely is it that 4032 blocks was insufficient, but 4038 was?

Well, the idea here is that we start pruning after the monitors have been archived. For that to work we need to wait at least 4032 blocks, but of course we're not super sure how soon after the threshold has been met user will actually call archive_fully_resolved_monitors, so I added the reorg delay on top. Could be debatable one way or another.
FWIW, I alternatively considered introducing a new MonitorArchivalComplete event type, but given that we on purpose keep the archival a best-effort thing it seemed wrong to go this way.

pretty unlikely, but holding onto them doesn't really cost us anything, so might as well...

yeah, I would have though max(4032, 6) would have also worked, but given that it indeed costs nothing…

TheBlueMatt

LGTM, one comment.

.. previously we just used the 4032 magic number, here we put it in a `pub const` that is reusable elsewhere.

Previously, we would prune tracked descriptors once we see a spend hit `ANTI_REORG_DELAY = 6` confirmations. However, this could lead to a scenario where lingering `ChannelMonitor`s waiting to be archived would still regenerate and replay `Event::SpendableOutput`s, i.e., we would re-add the same (now unspendable due to be actually being already spent) outputs again after having intially pruned them. Here, we therefore keep the tracked descriptors around for longer, in particular at least `ARCHIVAL_DELAY_BLOCKS + ANTI_REORG_DELAY = 4038` confirmations, at which point we assume the lingering monitors to have been likely archived, and it's 'safe' for us to also forget about the descriptors.

TheBlueMatt · 2025-01-28T15:28:25Z

Backported in #3567

v0.1.1 - Jan 28, 2025 - "Onchain Matters" API Updates =========== * A `ChannelManager::send_payment_with_route` was (re-)added, with semantics similar to `ChannelManager::send_payment` (rather than like the pre-0.1 `send_payent_with_route`, lightningdevkit#3534). * `RawBolt11Invoice::{to,from}_raw` were added (lightningdevkit#3549). Bug Fixes ========= * HTLCs which were forwarded where the inbound edge times out within the next three blocks will have the inbound HTLC failed backwards irrespective of the status of the outbound HTLC. This avoids the peer force-closing the channel (and claiming the inbound edge HTLC on-chain) even if we have not yet managed to claim the outbound edge on chain (lightningdevkit#3556). * On restart, replay of `Event::SpendableOutput`s could have caused `OutputSweeper` to generate double-spending transactions, making it unable to claim any delayed claims. This was resolved by retaining old claims for more than four weeks after they are claimed on-chain to detect replays (lightningdevkit#3559). * Fixed the additional feerate we will pay each time we RBF on-chain claims to match the Bitcoin Core policy (1 sat/vB) instead of 16 sats/vB (lightningdevkit#3457). * Fixed a cased where a custom `Router` which returns an invalid `Route`, provided to `ChannelManager`, can result in an outbound payment remaining pending forever despite no HTLCs being pending (lightningdevkit#3531). Security ======== 0.1.1 fixes a denial-of-service vulnerability allowing channel counterparties to cause force-closure of unrelated channels. * If a malicious channel counterparty force-closes a channel, broadcasting a revoked commitment transaction while the channel at closure time included multiple non-dust forwarded outbound HTLCs with identical payment hashes and amounts, failure to fail the HTLCs backwards could cause the channels on which we recieved the corresponding inbound HTLCs to be force-closed. Note that we'll receive, at a minimum, the malicious counterparty's reserve value when they broadcast the stale commitment (lightningdevkit#3556). Thanks to Matt Morehouse for reporting this issue.

tnull · 2025-01-24T10:25:54Z

Previously, we would prune tracked descriptors once we see a spend hit
ANTI_REORG_DELAY = 6 confirmations. However, this could lead to a
scenario where lingering ChannelMonitors waiting to be archived would
still regenerate and replay Event::SpendableOutputs, i.e., we would
re-add the same (now unspendable due to be actually being already spent)
outputs again after having intially pruned them.

Here, we therefore keep the tracked descriptors around for longer, in
particular at least ARCHIVAL_DELAY_BLOCKS + ANTI_REORG_DELAY = 4038
confirmations, at which point we assume the lingering monitors to have
been likely archived, and it's 'safe' for us to also forget about the
descriptors.

arik-so · 2025-01-24T18:09:14Z

 use core::ops::Deref;

+/// The number of blocks we wait before we prune the tracked spendable outputs.
+pub const PRUNE_DELAY_BLOCKS: u32 = ARCHIVAL_DELAY_BLOCKS + ANTI_REORG_DELAY;


how likely is it that 4032 blocks was insufficient, but 4038 was?

Well, the idea here is that we start pruning after the monitors have been archived. For that to work we need to wait at least 4032 blocks, but of course we're not super sure how soon after the threshold has been met user will actually call archive_fully_resolved_monitors, so I added the reorg delay on top. Could be debatable one way or another.
FWIW, I alternatively considered introducing a new MonitorArchivalComplete event type, but given that we on purpose keep the archival a best-effort thing it seemed wrong to go this way.

pretty unlikely, but holding onto them doesn't really cost us anything, so might as well...

yeah, I would have though max(4032, 6) would have also worked, but given that it indeed costs nothing…

TheBlueMatt

LGTM, one comment.

.. previously we just used the 4032 magic number, here we put it in a `pub const` that is reusable elsewhere.

Previously, we would prune tracked descriptors once we see a spend hit `ANTI_REORG_DELAY = 6` confirmations. However, this could lead to a scenario where lingering `ChannelMonitor`s waiting to be archived would still regenerate and replay `Event::SpendableOutput`s, i.e., we would re-add the same (now unspendable due to be actually being already spent) outputs again after having intially pruned them. Here, we therefore keep the tracked descriptors around for longer, in particular at least `ARCHIVAL_DELAY_BLOCKS + ANTI_REORG_DELAY = 4038` confirmations, at which point we assume the lingering monitors to have been likely archived, and it's 'safe' for us to also forget about the descriptors.

TheBlueMatt · 2025-01-28T15:28:25Z

Backported in #3567

v0.1.1 - Jan 28, 2025 - "Onchain Matters" API Updates =========== * A `ChannelManager::send_payment_with_route` was (re-)added, with semantics similar to `ChannelManager::send_payment` (rather than like the pre-0.1 `send_payent_with_route`, lightningdevkit#3534). * `RawBolt11Invoice::{to,from}_raw` were added (lightningdevkit#3549). Bug Fixes ========= * HTLCs which were forwarded where the inbound edge times out within the next three blocks will have the inbound HTLC failed backwards irrespective of the status of the outbound HTLC. This avoids the peer force-closing the channel (and claiming the inbound edge HTLC on-chain) even if we have not yet managed to claim the outbound edge on chain (lightningdevkit#3556). * On restart, replay of `Event::SpendableOutput`s could have caused `OutputSweeper` to generate double-spending transactions, making it unable to claim any delayed claims. This was resolved by retaining old claims for more than four weeks after they are claimed on-chain to detect replays (lightningdevkit#3559). * Fixed the additional feerate we will pay each time we RBF on-chain claims to match the Bitcoin Core policy (1 sat/vB) instead of 16 sats/vB (lightningdevkit#3457). * Fixed a cased where a custom `Router` which returns an invalid `Route`, provided to `ChannelManager`, can result in an outbound payment remaining pending forever despite no HTLCs being pending (lightningdevkit#3531). Security ======== 0.1.1 fixes a denial-of-service vulnerability allowing channel counterparties to cause force-closure of unrelated channels. * If a malicious channel counterparty force-closes a channel, broadcasting a revoked commitment transaction while the channel at closure time included multiple non-dust forwarded outbound HTLCs with identical payment hashes and amounts, failure to fail the HTLCs backwards could cause the channels on which we recieved the corresponding inbound HTLCs to be force-closed. Note that we'll receive, at a minimum, the malicious counterparty's reserve value when they broadcast the stale commitment (lightningdevkit#3556). Thanks to Matt Morehouse for reporting this issue.

tnull · 2025-01-24T10:25:54Z

Previously, we would prune tracked descriptors once we see a spend hit
ANTI_REORG_DELAY = 6 confirmations. However, this could lead to a
scenario where lingering ChannelMonitors waiting to be archived would
still regenerate and replay Event::SpendableOutputs, i.e., we would
re-add the same (now unspendable due to be actually being already spent)
outputs again after having intially pruned them.

Here, we therefore keep the tracked descriptors around for longer, in
particular at least ARCHIVAL_DELAY_BLOCKS + ANTI_REORG_DELAY = 4038
confirmations, at which point we assume the lingering monitors to have
been likely archived, and it's 'safe' for us to also forget about the
descriptors.

arik-so · 2025-01-24T18:09:14Z

 use core::ops::Deref;

+/// The number of blocks we wait before we prune the tracked spendable outputs.
+pub const PRUNE_DELAY_BLOCKS: u32 = ARCHIVAL_DELAY_BLOCKS + ANTI_REORG_DELAY;


how likely is it that 4032 blocks was insufficient, but 4038 was?

Well, the idea here is that we start pruning after the monitors have been archived. For that to work we need to wait at least 4032 blocks, but of course we're not super sure how soon after the threshold has been met user will actually call archive_fully_resolved_monitors, so I added the reorg delay on top. Could be debatable one way or another.
FWIW, I alternatively considered introducing a new MonitorArchivalComplete event type, but given that we on purpose keep the archival a best-effort thing it seemed wrong to go this way.

pretty unlikely, but holding onto them doesn't really cost us anything, so might as well...

yeah, I would have though max(4032, 6) would have also worked, but given that it indeed costs nothing…

TheBlueMatt

LGTM, one comment.

.. previously we just used the 4032 magic number, here we put it in a `pub const` that is reusable elsewhere.

Previously, we would prune tracked descriptors once we see a spend hit `ANTI_REORG_DELAY = 6` confirmations. However, this could lead to a scenario where lingering `ChannelMonitor`s waiting to be archived would still regenerate and replay `Event::SpendableOutput`s, i.e., we would re-add the same (now unspendable due to be actually being already spent) outputs again after having intially pruned them. Here, we therefore keep the tracked descriptors around for longer, in particular at least `ARCHIVAL_DELAY_BLOCKS + ANTI_REORG_DELAY = 4038` confirmations, at which point we assume the lingering monitors to have been likely archived, and it's 'safe' for us to also forget about the descriptors.

TheBlueMatt · 2025-01-28T15:28:25Z

Backported in #3567

v0.1.1 - Jan 28, 2025 - "Onchain Matters" API Updates =========== * A `ChannelManager::send_payment_with_route` was (re-)added, with semantics similar to `ChannelManager::send_payment` (rather than like the pre-0.1 `send_payent_with_route`, lightningdevkit#3534). * `RawBolt11Invoice::{to,from}_raw` were added (lightningdevkit#3549). Bug Fixes ========= * HTLCs which were forwarded where the inbound edge times out within the next three blocks will have the inbound HTLC failed backwards irrespective of the status of the outbound HTLC. This avoids the peer force-closing the channel (and claiming the inbound edge HTLC on-chain) even if we have not yet managed to claim the outbound edge on chain (lightningdevkit#3556). * On restart, replay of `Event::SpendableOutput`s could have caused `OutputSweeper` to generate double-spending transactions, making it unable to claim any delayed claims. This was resolved by retaining old claims for more than four weeks after they are claimed on-chain to detect replays (lightningdevkit#3559). * Fixed the additional feerate we will pay each time we RBF on-chain claims to match the Bitcoin Core policy (1 sat/vB) instead of 16 sats/vB (lightningdevkit#3457). * Fixed a cased where a custom `Router` which returns an invalid `Route`, provided to `ChannelManager`, can result in an outbound payment remaining pending forever despite no HTLCs being pending (lightningdevkit#3531). Security ======== 0.1.1 fixes a denial-of-service vulnerability allowing channel counterparties to cause force-closure of unrelated channels. * If a malicious channel counterparty force-closes a channel, broadcasting a revoked commitment transaction while the channel at closure time included multiple non-dust forwarded outbound HTLCs with identical payment hashes and amounts, failure to fail the HTLCs backwards could cause the channels on which we recieved the corresponding inbound HTLCs to be force-closed. Note that we'll receive, at a minimum, the malicious counterparty's reserve value when they broadcast the stale commitment (lightningdevkit#3556). Thanks to Matt Morehouse for reporting this issue.

tnull · 2025-01-24T10:25:54Z

Previously, we would prune tracked descriptors once we see a spend hit
ANTI_REORG_DELAY = 6 confirmations. However, this could lead to a
scenario where lingering ChannelMonitors waiting to be archived would
still regenerate and replay Event::SpendableOutputs, i.e., we would
re-add the same (now unspendable due to be actually being already spent)
outputs again after having intially pruned them.

Here, we therefore keep the tracked descriptors around for longer, in
particular at least ARCHIVAL_DELAY_BLOCKS + ANTI_REORG_DELAY = 4038
confirmations, at which point we assume the lingering monitors to have
been likely archived, and it's 'safe' for us to also forget about the
descriptors.

arik-so · 2025-01-24T18:09:14Z

 use core::ops::Deref;

+/// The number of blocks we wait before we prune the tracked spendable outputs.
+pub const PRUNE_DELAY_BLOCKS: u32 = ARCHIVAL_DELAY_BLOCKS + ANTI_REORG_DELAY;


how likely is it that 4032 blocks was insufficient, but 4038 was?

Well, the idea here is that we start pruning after the monitors have been archived. For that to work we need to wait at least 4032 blocks, but of course we're not super sure how soon after the threshold has been met user will actually call archive_fully_resolved_monitors, so I added the reorg delay on top. Could be debatable one way or another.
FWIW, I alternatively considered introducing a new MonitorArchivalComplete event type, but given that we on purpose keep the archival a best-effort thing it seemed wrong to go this way.

pretty unlikely, but holding onto them doesn't really cost us anything, so might as well...

yeah, I would have though max(4032, 6) would have also worked, but given that it indeed costs nothing…

TheBlueMatt

LGTM, one comment.

.. previously we just used the 4032 magic number, here we put it in a `pub const` that is reusable elsewhere.

Previously, we would prune tracked descriptors once we see a spend hit `ANTI_REORG_DELAY = 6` confirmations. However, this could lead to a scenario where lingering `ChannelMonitor`s waiting to be archived would still regenerate and replay `Event::SpendableOutput`s, i.e., we would re-add the same (now unspendable due to be actually being already spent) outputs again after having intially pruned them. Here, we therefore keep the tracked descriptors around for longer, in particular at least `ARCHIVAL_DELAY_BLOCKS + ANTI_REORG_DELAY = 4038` confirmations, at which point we assume the lingering monitors to have been likely archived, and it's 'safe' for us to also forget about the descriptors.

TheBlueMatt · 2025-01-28T15:28:25Z

Backported in #3567

v0.1.1 - Jan 28, 2025 - "Onchain Matters" API Updates =========== * A `ChannelManager::send_payment_with_route` was (re-)added, with semantics similar to `ChannelManager::send_payment` (rather than like the pre-0.1 `send_payent_with_route`, lightningdevkit#3534). * `RawBolt11Invoice::{to,from}_raw` were added (lightningdevkit#3549). Bug Fixes ========= * HTLCs which were forwarded where the inbound edge times out within the next three blocks will have the inbound HTLC failed backwards irrespective of the status of the outbound HTLC. This avoids the peer force-closing the channel (and claiming the inbound edge HTLC on-chain) even if we have not yet managed to claim the outbound edge on chain (lightningdevkit#3556). * On restart, replay of `Event::SpendableOutput`s could have caused `OutputSweeper` to generate double-spending transactions, making it unable to claim any delayed claims. This was resolved by retaining old claims for more than four weeks after they are claimed on-chain to detect replays (lightningdevkit#3559). * Fixed the additional feerate we will pay each time we RBF on-chain claims to match the Bitcoin Core policy (1 sat/vB) instead of 16 sats/vB (lightningdevkit#3457). * Fixed a cased where a custom `Router` which returns an invalid `Route`, provided to `ChannelManager`, can result in an outbound payment remaining pending forever despite no HTLCs being pending (lightningdevkit#3531). Security ======== 0.1.1 fixes a denial-of-service vulnerability allowing channel counterparties to cause force-closure of unrelated channels. * If a malicious channel counterparty force-closes a channel, broadcasting a revoked commitment transaction while the channel at closure time included multiple non-dust forwarded outbound HTLCs with identical payment hashes and amounts, failure to fail the HTLCs backwards could cause the channels on which we recieved the corresponding inbound HTLCs to be force-closed. Note that we'll receive, at a minimum, the malicious counterparty's reserve value when they broadcast the stale commitment (lightningdevkit#3556). Thanks to Matt Morehouse for reporting this issue.

tnull · 2025-01-24T10:25:54Z

Previously, we would prune tracked descriptors once we see a spend hit
ANTI_REORG_DELAY = 6 confirmations. However, this could lead to a
scenario where lingering ChannelMonitors waiting to be archived would
still regenerate and replay Event::SpendableOutputs, i.e., we would
re-add the same (now unspendable due to be actually being already spent)
outputs again after having intially pruned them.

Here, we therefore keep the tracked descriptors around for longer, in
particular at least ARCHIVAL_DELAY_BLOCKS + ANTI_REORG_DELAY = 4038
confirmations, at which point we assume the lingering monitors to have
been likely archived, and it's 'safe' for us to also forget about the
descriptors.

arik-so · 2025-01-24T18:09:14Z

 use core::ops::Deref;

+/// The number of blocks we wait before we prune the tracked spendable outputs.
+pub const PRUNE_DELAY_BLOCKS: u32 = ARCHIVAL_DELAY_BLOCKS + ANTI_REORG_DELAY;


how likely is it that 4032 blocks was insufficient, but 4038 was?

Well, the idea here is that we start pruning after the monitors have been archived. For that to work we need to wait at least 4032 blocks, but of course we're not super sure how soon after the threshold has been met user will actually call archive_fully_resolved_monitors, so I added the reorg delay on top. Could be debatable one way or another.
FWIW, I alternatively considered introducing a new MonitorArchivalComplete event type, but given that we on purpose keep the archival a best-effort thing it seemed wrong to go this way.

pretty unlikely, but holding onto them doesn't really cost us anything, so might as well...

yeah, I would have though max(4032, 6) would have also worked, but given that it indeed costs nothing…

TheBlueMatt

LGTM, one comment.

.. previously we just used the 4032 magic number, here we put it in a `pub const` that is reusable elsewhere.

Previously, we would prune tracked descriptors once we see a spend hit `ANTI_REORG_DELAY = 6` confirmations. However, this could lead to a scenario where lingering `ChannelMonitor`s waiting to be archived would still regenerate and replay `Event::SpendableOutput`s, i.e., we would re-add the same (now unspendable due to be actually being already spent) outputs again after having intially pruned them. Here, we therefore keep the tracked descriptors around for longer, in particular at least `ARCHIVAL_DELAY_BLOCKS + ANTI_REORG_DELAY = 4038` confirmations, at which point we assume the lingering monitors to have been likely archived, and it's 'safe' for us to also forget about the descriptors.

TheBlueMatt · 2025-01-28T15:28:25Z

Backported in #3567

v0.1.1 - Jan 28, 2025 - "Onchain Matters" API Updates =========== * A `ChannelManager::send_payment_with_route` was (re-)added, with semantics similar to `ChannelManager::send_payment` (rather than like the pre-0.1 `send_payent_with_route`, lightningdevkit#3534). * `RawBolt11Invoice::{to,from}_raw` were added (lightningdevkit#3549). Bug Fixes ========= * HTLCs which were forwarded where the inbound edge times out within the next three blocks will have the inbound HTLC failed backwards irrespective of the status of the outbound HTLC. This avoids the peer force-closing the channel (and claiming the inbound edge HTLC on-chain) even if we have not yet managed to claim the outbound edge on chain (lightningdevkit#3556). * On restart, replay of `Event::SpendableOutput`s could have caused `OutputSweeper` to generate double-spending transactions, making it unable to claim any delayed claims. This was resolved by retaining old claims for more than four weeks after they are claimed on-chain to detect replays (lightningdevkit#3559). * Fixed the additional feerate we will pay each time we RBF on-chain claims to match the Bitcoin Core policy (1 sat/vB) instead of 16 sats/vB (lightningdevkit#3457). * Fixed a cased where a custom `Router` which returns an invalid `Route`, provided to `ChannelManager`, can result in an outbound payment remaining pending forever despite no HTLCs being pending (lightningdevkit#3531). Security ======== 0.1.1 fixes a denial-of-service vulnerability allowing channel counterparties to cause force-closure of unrelated channels. * If a malicious channel counterparty force-closes a channel, broadcasting a revoked commitment transaction while the channel at closure time included multiple non-dust forwarded outbound HTLCs with identical payment hashes and amounts, failure to fail the HTLCs backwards could cause the channels on which we recieved the corresponding inbound HTLCs to be force-closed. Note that we'll receive, at a minimum, the malicious counterparty's reserve value when they broadcast the stale commitment (lightningdevkit#3556). Thanks to Matt Morehouse for reporting this issue.

tnull · 2025-01-24T10:25:54Z

Previously, we would prune tracked descriptors once we see a spend hit
ANTI_REORG_DELAY = 6 confirmations. However, this could lead to a
scenario where lingering ChannelMonitors waiting to be archived would
still regenerate and replay Event::SpendableOutputs, i.e., we would
re-add the same (now unspendable due to be actually being already spent)
outputs again after having intially pruned them.

Here, we therefore keep the tracked descriptors around for longer, in
particular at least ARCHIVAL_DELAY_BLOCKS + ANTI_REORG_DELAY = 4038
confirmations, at which point we assume the lingering monitors to have
been likely archived, and it's 'safe' for us to also forget about the
descriptors.

arik-so · 2025-01-24T18:09:14Z

 use core::ops::Deref;

+/// The number of blocks we wait before we prune the tracked spendable outputs.
+pub const PRUNE_DELAY_BLOCKS: u32 = ARCHIVAL_DELAY_BLOCKS + ANTI_REORG_DELAY;


how likely is it that 4032 blocks was insufficient, but 4038 was?

Well, the idea here is that we start pruning after the monitors have been archived. For that to work we need to wait at least 4032 blocks, but of course we're not super sure how soon after the threshold has been met user will actually call archive_fully_resolved_monitors, so I added the reorg delay on top. Could be debatable one way or another.
FWIW, I alternatively considered introducing a new MonitorArchivalComplete event type, but given that we on purpose keep the archival a best-effort thing it seemed wrong to go this way.

pretty unlikely, but holding onto them doesn't really cost us anything, so might as well...

yeah, I would have though max(4032, 6) would have also worked, but given that it indeed costs nothing…

TheBlueMatt

LGTM, one comment.

.. previously we just used the 4032 magic number, here we put it in a `pub const` that is reusable elsewhere.

Previously, we would prune tracked descriptors once we see a spend hit `ANTI_REORG_DELAY = 6` confirmations. However, this could lead to a scenario where lingering `ChannelMonitor`s waiting to be archived would still regenerate and replay `Event::SpendableOutput`s, i.e., we would re-add the same (now unspendable due to be actually being already spent) outputs again after having intially pruned them. Here, we therefore keep the tracked descriptors around for longer, in particular at least `ARCHIVAL_DELAY_BLOCKS + ANTI_REORG_DELAY = 4038` confirmations, at which point we assume the lingering monitors to have been likely archived, and it's 'safe' for us to also forget about the descriptors.

TheBlueMatt · 2025-01-28T15:28:25Z

Backported in #3567

v0.1.1 - Jan 28, 2025 - "Onchain Matters" API Updates =========== * A `ChannelManager::send_payment_with_route` was (re-)added, with semantics similar to `ChannelManager::send_payment` (rather than like the pre-0.1 `send_payent_with_route`, lightningdevkit#3534). * `RawBolt11Invoice::{to,from}_raw` were added (lightningdevkit#3549). Bug Fixes ========= * HTLCs which were forwarded where the inbound edge times out within the next three blocks will have the inbound HTLC failed backwards irrespective of the status of the outbound HTLC. This avoids the peer force-closing the channel (and claiming the inbound edge HTLC on-chain) even if we have not yet managed to claim the outbound edge on chain (lightningdevkit#3556). * On restart, replay of `Event::SpendableOutput`s could have caused `OutputSweeper` to generate double-spending transactions, making it unable to claim any delayed claims. This was resolved by retaining old claims for more than four weeks after they are claimed on-chain to detect replays (lightningdevkit#3559). * Fixed the additional feerate we will pay each time we RBF on-chain claims to match the Bitcoin Core policy (1 sat/vB) instead of 16 sats/vB (lightningdevkit#3457). * Fixed a cased where a custom `Router` which returns an invalid `Route`, provided to `ChannelManager`, can result in an outbound payment remaining pending forever despite no HTLCs being pending (lightningdevkit#3531). Security ======== 0.1.1 fixes a denial-of-service vulnerability allowing channel counterparties to cause force-closure of unrelated channels. * If a malicious channel counterparty force-closes a channel, broadcasting a revoked commitment transaction while the channel at closure time included multiple non-dust forwarded outbound HTLCs with identical payment hashes and amounts, failure to fail the HTLCs backwards could cause the channels on which we recieved the corresponding inbound HTLCs to be force-closed. Note that we'll receive, at a minimum, the malicious counterparty's reserve value when they broadcast the stale commitment (lightningdevkit#3556). Thanks to Matt Morehouse for reporting this issue.

tnull · 2025-01-24T10:25:54Z

Previously, we would prune tracked descriptors once we see a spend hit
ANTI_REORG_DELAY = 6 confirmations. However, this could lead to a
scenario where lingering ChannelMonitors waiting to be archived would
still regenerate and replay Event::SpendableOutputs, i.e., we would
re-add the same (now unspendable due to be actually being already spent)
outputs again after having intially pruned them.

Here, we therefore keep the tracked descriptors around for longer, in
particular at least ARCHIVAL_DELAY_BLOCKS + ANTI_REORG_DELAY = 4038
confirmations, at which point we assume the lingering monitors to have
been likely archived, and it's 'safe' for us to also forget about the
descriptors.

arik-so · 2025-01-24T18:09:14Z

 use core::ops::Deref;

+/// The number of blocks we wait before we prune the tracked spendable outputs.
+pub const PRUNE_DELAY_BLOCKS: u32 = ARCHIVAL_DELAY_BLOCKS + ANTI_REORG_DELAY;


how likely is it that 4032 blocks was insufficient, but 4038 was?

Well, the idea here is that we start pruning after the monitors have been archived. For that to work we need to wait at least 4032 blocks, but of course we're not super sure how soon after the threshold has been met user will actually call archive_fully_resolved_monitors, so I added the reorg delay on top. Could be debatable one way or another.
FWIW, I alternatively considered introducing a new MonitorArchivalComplete event type, but given that we on purpose keep the archival a best-effort thing it seemed wrong to go this way.

pretty unlikely, but holding onto them doesn't really cost us anything, so might as well...

yeah, I would have though max(4032, 6) would have also worked, but given that it indeed costs nothing…

TheBlueMatt

LGTM, one comment.

.. previously we just used the 4032 magic number, here we put it in a `pub const` that is reusable elsewhere.

Previously, we would prune tracked descriptors once we see a spend hit `ANTI_REORG_DELAY = 6` confirmations. However, this could lead to a scenario where lingering `ChannelMonitor`s waiting to be archived would still regenerate and replay `Event::SpendableOutput`s, i.e., we would re-add the same (now unspendable due to be actually being already spent) outputs again after having intially pruned them. Here, we therefore keep the tracked descriptors around for longer, in particular at least `ARCHIVAL_DELAY_BLOCKS + ANTI_REORG_DELAY = 4038` confirmations, at which point we assume the lingering monitors to have been likely archived, and it's 'safe' for us to also forget about the descriptors.

TheBlueMatt · 2025-01-28T15:28:25Z

Backported in #3567

v0.1.1 - Jan 28, 2025 - "Onchain Matters" API Updates =========== * A `ChannelManager::send_payment_with_route` was (re-)added, with semantics similar to `ChannelManager::send_payment` (rather than like the pre-0.1 `send_payent_with_route`, lightningdevkit#3534). * `RawBolt11Invoice::{to,from}_raw` were added (lightningdevkit#3549). Bug Fixes ========= * HTLCs which were forwarded where the inbound edge times out within the next three blocks will have the inbound HTLC failed backwards irrespective of the status of the outbound HTLC. This avoids the peer force-closing the channel (and claiming the inbound edge HTLC on-chain) even if we have not yet managed to claim the outbound edge on chain (lightningdevkit#3556). * On restart, replay of `Event::SpendableOutput`s could have caused `OutputSweeper` to generate double-spending transactions, making it unable to claim any delayed claims. This was resolved by retaining old claims for more than four weeks after they are claimed on-chain to detect replays (lightningdevkit#3559). * Fixed the additional feerate we will pay each time we RBF on-chain claims to match the Bitcoin Core policy (1 sat/vB) instead of 16 sats/vB (lightningdevkit#3457). * Fixed a cased where a custom `Router` which returns an invalid `Route`, provided to `ChannelManager`, can result in an outbound payment remaining pending forever despite no HTLCs being pending (lightningdevkit#3531). Security ======== 0.1.1 fixes a denial-of-service vulnerability allowing channel counterparties to cause force-closure of unrelated channels. * If a malicious channel counterparty force-closes a channel, broadcasting a revoked commitment transaction while the channel at closure time included multiple non-dust forwarded outbound HTLCs with identical payment hashes and amounts, failure to fail the HTLCs backwards could cause the channels on which we recieved the corresponding inbound HTLCs to be force-closed. Note that we'll receive, at a minimum, the malicious counterparty's reserve value when they broadcast the stale commitment (lightningdevkit#3556). Thanks to Matt Morehouse for reporting this issue.

Introduce SpendableOutputDescriptor::outpoint accessor
c03c8a4

tnull requested a review from TheBlueMatt January 24, 2025 10:25

arik-so reviewed Jan 24, 2025
View reviewed changes

TheBlueMatt reviewed Jan 24, 2025
View reviewed changes

Comment threadlightning/src/util/sweep.rs Outdated

TheBlueMatt added the backport 0.1 label Jan 24, 2025

tnull added 2 commits January 27, 2025 09:56

Prefactor: Make monior archival delay a pub const
d8b50f0
.. previously we just used the 4032 magic number, here we put it in a `pub const` that is reusable elsewhere.

tnull force-pushed the 2025-01-sweeper-improvements branch from 0aa813f to 84412ccCompare January 27, 2025 08:56

TheBlueMatt approved these changes Jan 27, 2025
View reviewed changes

arik-so approved these changes Jan 27, 2025
View reviewed changes

Comment threadlightning/src/chain/channelmonitor.rs

tnull merged commit 79267d3 into lightningdevkit:mainJan 27, 2025

TheBlueMatt mentioned this pull request Jan 28, 2025
0.1.1 Backports #3567
Merged

TheBlueMatt removed the backport 0.1 label Jan 28, 2025

Introduce SpendableOutputDescriptor::outpoint accessor
c03c8a4

tnull requested a review from TheBlueMatt January 24, 2025 10:25

arik-so reviewed Jan 24, 2025
View reviewed changes

TheBlueMatt reviewed Jan 24, 2025
View reviewed changes

Comment threadlightning/src/util/sweep.rs Outdated

TheBlueMatt added the backport 0.1 label Jan 24, 2025

tnull added 2 commits January 27, 2025 09:56

Prefactor: Make monior archival delay a pub const
d8b50f0
.. previously we just used the 4032 magic number, here we put it in a `pub const` that is reusable elsewhere.

tnull force-pushed the 2025-01-sweeper-improvements branch from 0aa813f to 84412ccCompare January 27, 2025 08:56

TheBlueMatt approved these changes Jan 27, 2025
View reviewed changes

arik-so approved these changes Jan 27, 2025
View reviewed changes

Comment threadlightning/src/chain/channelmonitor.rs

tnull merged commit 79267d3 into lightningdevkit:mainJan 27, 2025

TheBlueMatt mentioned this pull request Jan 28, 2025
0.1.1 Backports #3567
Merged

TheBlueMatt removed the backport 0.1 label Jan 28, 2025

Introduce SpendableOutputDescriptor::outpoint accessor
c03c8a4

tnull requested a review from TheBlueMatt January 24, 2025 10:25

arik-so reviewed Jan 24, 2025
View reviewed changes

TheBlueMatt reviewed Jan 24, 2025
View reviewed changes

Comment threadlightning/src/util/sweep.rs Outdated

TheBlueMatt added the backport 0.1 label Jan 24, 2025

tnull added 2 commits January 27, 2025 09:56

Prefactor: Make monior archival delay a pub const
d8b50f0
.. previously we just used the 4032 magic number, here we put it in a `pub const` that is reusable elsewhere.

tnull force-pushed the 2025-01-sweeper-improvements branch from 0aa813f to 84412ccCompare January 27, 2025 08:56

TheBlueMatt approved these changes Jan 27, 2025
View reviewed changes

arik-so approved these changes Jan 27, 2025
View reviewed changes

Comment threadlightning/src/chain/channelmonitor.rs

tnull merged commit 79267d3 into lightningdevkit:mainJan 27, 2025

TheBlueMatt mentioned this pull request Jan 28, 2025
0.1.1 Backports #3567
Merged

TheBlueMatt removed the backport 0.1 label Jan 28, 2025

Introduce SpendableOutputDescriptor::outpoint accessor
c03c8a4

tnull requested a review from TheBlueMatt January 24, 2025 10:25

arik-so reviewed Jan 24, 2025
View reviewed changes

TheBlueMatt reviewed Jan 24, 2025
View reviewed changes

Comment threadlightning/src/util/sweep.rs Outdated

TheBlueMatt added the backport 0.1 label Jan 24, 2025

tnull added 2 commits January 27, 2025 09:56

Prefactor: Make monior archival delay a pub const
d8b50f0
.. previously we just used the 4032 magic number, here we put it in a `pub const` that is reusable elsewhere.

tnull force-pushed the 2025-01-sweeper-improvements branch from 0aa813f to 84412ccCompare January 27, 2025 08:56

TheBlueMatt approved these changes Jan 27, 2025
View reviewed changes

arik-so approved these changes Jan 27, 2025
View reviewed changes

Comment threadlightning/src/chain/channelmonitor.rs

tnull merged commit 79267d3 into lightningdevkit:mainJan 27, 2025

TheBlueMatt mentioned this pull request Jan 28, 2025
0.1.1 Backports #3567
Merged

TheBlueMatt removed the backport 0.1 label Jan 28, 2025

Introduce SpendableOutputDescriptor::outpoint accessor
c03c8a4

tnull requested a review from TheBlueMatt January 24, 2025 10:25

arik-so reviewed Jan 24, 2025
View reviewed changes

TheBlueMatt reviewed Jan 24, 2025
View reviewed changes

Comment threadlightning/src/util/sweep.rs Outdated

TheBlueMatt added the backport 0.1 label Jan 24, 2025

tnull added 2 commits January 27, 2025 09:56

Prefactor: Make monior archival delay a pub const
d8b50f0
.. previously we just used the 4032 magic number, here we put it in a `pub const` that is reusable elsewhere.

tnull force-pushed the 2025-01-sweeper-improvements branch from 0aa813f to 84412ccCompare January 27, 2025 08:56

TheBlueMatt approved these changes Jan 27, 2025
View reviewed changes

arik-so approved these changes Jan 27, 2025
View reviewed changes

Comment threadlightning/src/chain/channelmonitor.rs

tnull merged commit 79267d3 into lightningdevkit:mainJan 27, 2025

TheBlueMatt mentioned this pull request Jan 28, 2025
0.1.1 Backports #3567
Merged

TheBlueMatt removed the backport 0.1 label Jan 28, 2025

Introduce SpendableOutputDescriptor::outpoint accessor
c03c8a4

tnull requested a review from TheBlueMatt January 24, 2025 10:25

arik-so reviewed Jan 24, 2025
View reviewed changes

TheBlueMatt reviewed Jan 24, 2025
View reviewed changes

Comment threadlightning/src/util/sweep.rs Outdated

TheBlueMatt added the backport 0.1 label Jan 24, 2025

tnull added 2 commits January 27, 2025 09:56

Prefactor: Make monior archival delay a pub const
d8b50f0
.. previously we just used the 4032 magic number, here we put it in a `pub const` that is reusable elsewhere.

tnull force-pushed the 2025-01-sweeper-improvements branch from 0aa813f to 84412ccCompare January 27, 2025 08:56

TheBlueMatt approved these changes Jan 27, 2025
View reviewed changes

arik-so approved these changes Jan 27, 2025
View reviewed changes

Comment threadlightning/src/chain/channelmonitor.rs

tnull merged commit 79267d3 into lightningdevkit:mainJan 27, 2025

TheBlueMatt mentioned this pull request Jan 28, 2025
0.1.1 Backports #3567
Merged

TheBlueMatt removed the backport 0.1 label Jan 28, 2025

Introduce SpendableOutputDescriptor::outpoint accessor
c03c8a4

tnull requested a review from TheBlueMatt January 24, 2025 10:25

arik-so reviewed Jan 24, 2025
View reviewed changes

TheBlueMatt reviewed Jan 24, 2025
View reviewed changes

Comment threadlightning/src/util/sweep.rs Outdated

TheBlueMatt added the backport 0.1 label Jan 24, 2025

tnull added 2 commits January 27, 2025 09:56

Prefactor: Make monior archival delay a pub const
d8b50f0
.. previously we just used the 4032 magic number, here we put it in a `pub const` that is reusable elsewhere.

tnull force-pushed the 2025-01-sweeper-improvements branch from 0aa813f to 84412ccCompare January 27, 2025 08:56

TheBlueMatt approved these changes Jan 27, 2025
View reviewed changes

arik-so approved these changes Jan 27, 2025
View reviewed changes

Comment threadlightning/src/chain/channelmonitor.rs

tnull merged commit 79267d3 into lightningdevkit:mainJan 27, 2025

TheBlueMatt mentioned this pull request Jan 28, 2025
0.1.1 Backports #3567
Merged

TheBlueMatt removed the backport 0.1 label Jan 28, 2025

Introduce SpendableOutputDescriptor::outpoint accessor
c03c8a4

tnull requested a review from TheBlueMatt January 24, 2025 10:25

arik-so reviewed Jan 24, 2025
View reviewed changes

TheBlueMatt reviewed Jan 24, 2025
View reviewed changes

Comment threadlightning/src/util/sweep.rs Outdated

TheBlueMatt added the backport 0.1 label Jan 24, 2025

tnull added 2 commits January 27, 2025 09:56

Prefactor: Make monior archival delay a pub const
d8b50f0
.. previously we just used the 4032 magic number, here we put it in a `pub const` that is reusable elsewhere.

tnull force-pushed the 2025-01-sweeper-improvements branch from 0aa813f to 84412ccCompare January 27, 2025 08:56

TheBlueMatt approved these changes Jan 27, 2025
View reviewed changes

arik-so approved these changes Jan 27, 2025
View reviewed changes

Comment threadlightning/src/chain/channelmonitor.rs

tnull merged commit 79267d3 into lightningdevkit:mainJan 27, 2025

TheBlueMatt mentioned this pull request Jan 28, 2025
0.1.1 Backports #3567
Merged

TheBlueMatt removed the backport 0.1 label Jan 28, 2025

Uh oh!

Conversation

tnull commented Jan 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented Jan 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

tnull commented Jan 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented Jan 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

tnull commented Jan 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented Jan 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!