Skip to content

OutputSweeper: Delay pruning until monitors have likely been archived #3559

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 27, 2025
Merged

OutputSweeper: Delay pruning until monitors have likely been archived #3559

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c03c8a4
Introduce `SpendableOutputDescriptor::outpoint` accessor
tnull Jan 24, 2025
d8b50f0
Prefactor: Make monior archival delay a `pub const`
tnull Jan 24, 2025
84412cc
`OutputSweeper`: Delay pruning until monitors have likely been archived
tnull Jan 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions lightning-background-processor/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1099,7 +1099,7 @@ mod tests {
SCORER_PERSISTENCE_SECONDARY_NAMESPACE,
};
use lightning::util::ser::Writeable;
use lightning::util::sweep::{OutputSpendStatus, OutputSweeper};
use lightning::util::sweep::{OutputSpendStatus, OutputSweeper, PRUNE_DELAY_BLOCKS};
use lightning::util::test_utils;
use lightning::{get_event, get_event_msg};
use lightning_persister::fs_store::FilesystemStore;
Expand Down Expand Up @@ -2282,8 +2282,8 @@ mod tests {
}

// Check we stop tracking the spendable outputs when one of the txs reaches
// ANTI_REORG_DELAY confirmations.
confirm_transaction_depth(&mut nodes[0], &sweep_tx_0, ANTI_REORG_DELAY);
// PRUNE_DELAY_BLOCKS confirmations.
confirm_transaction_depth(&mut nodes[0], &sweep_tx_0, PRUNE_DELAY_BLOCKS);
assert_eq!(nodes[0].sweeper.tracked_spendable_outputs().len(), 0);

if !std::thread::panicking() {
Expand Down
16 changes: 10 additions & 6 deletions lightning/src/chain/channelmonitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,10 @@ pub(crate) const LATENCY_GRACE_PERIOD_BLOCKS: u32 = 3;
// solved by a previous claim tx. What we want to avoid is reorg evicting our claim tx and us not
// keep bumping another claim tx to solve the outpoint.
pub const ANTI_REORG_DELAY: u32 = 6;
/// Number of blocks we wait before assuming a [`ChannelMonitor`] to be fully resolved and
/// considering it be safely archived.
// 4032 blocks are roughly four weeks
pub const ARCHIVAL_DELAY_BLOCKS: u32 = 4032;
/// Number of blocks before confirmation at which we fail back an un-relayed HTLC or at which we
/// refuse to accept a new HTLC.
///
Expand Down Expand Up @@ -2015,10 +2019,11 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
///
/// This function returns a tuple of two booleans, the first indicating whether the monitor is
/// fully resolved, and the second whether the monitor needs persistence to ensure it is
/// reliably marked as resolved within 4032 blocks.
/// reliably marked as resolved within [`ARCHIVAL_DELAY_BLOCKS`] blocks.
///
/// The first boolean is true only if [`Self::get_claimable_balances`] has been empty for at least
/// 4032 blocks as an additional protection against any bugs resulting in spuriously empty balance sets.
/// The first boolean is true only if [`Self::get_claimable_balances`] has been empty for at
/// least [`ARCHIVAL_DELAY_BLOCKS`] blocks as an additional protection against any bugs
/// resulting in spuriously empty balance sets.
pub fn check_and_update_full_resolution_status<L: Logger>(&self, logger: &L) -> (bool, bool) {
let mut is_all_funds_claimed = self.get_claimable_balances().is_empty();
let current_height = self.current_best_block().height;
Expand All @@ -2034,11 +2039,10 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
// once processed, implies the preimage exists in the corresponding inbound channel.
let preimages_not_needed_elsewhere = inner.pending_monitor_events.is_empty();

const BLOCKS_THRESHOLD: u32 = 4032; // ~four weeks
match (inner.balances_empty_height, is_all_funds_claimed, preimages_not_needed_elsewhere) {
(Some(balances_empty_height), true, true) => {
// Claimed all funds, check if reached the blocks threshold.
(current_height >= balances_empty_height + BLOCKS_THRESHOLD, false)
(current_height >= balances_empty_height + ARCHIVAL_DELAY_BLOCKS, false)
},
(Some(_), false, _)|(Some(_), _, false) => {
// previously assumed we claimed all funds, but we have new funds to claim or
Expand All @@ -2058,7 +2062,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
// None. It is set to the current block height.
log_debug!(logger,
"ChannelMonitor funded at {} is now fully resolved. It will become archivable in {} blocks",
inner.get_funding_txo().0, BLOCKS_THRESHOLD);
inner.get_funding_txo().0, ARCHIVAL_DELAY_BLOCKS);
inner.balances_empty_height = Some(current_height);
(false, true)
},
Expand Down
18 changes: 9 additions & 9 deletions lightning/src/ln/monitor_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
//! Further functional tests which test blockchain reorganizations.

use crate::sign::{ecdsa::EcdsaChannelSigner, OutputSpender, SpendableOutputDescriptor};
use crate::chain::channelmonitor::{ANTI_REORG_DELAY, LATENCY_GRACE_PERIOD_BLOCKS, Balance, BalanceSource, ChannelMonitorUpdateStep};
use crate::chain::channelmonitor::{ANTI_REORG_DELAY, ARCHIVAL_DELAY_BLOCKS,LATENCY_GRACE_PERIOD_BLOCKS, Balance, BalanceSource, ChannelMonitorUpdateStep};
use crate::chain::transaction::OutPoint;
use crate::chain::chaininterface::{ConfirmationTarget, LowerBoundedFeeEstimator, compute_feerate_sat_per_1000_weight};
use crate::events::bump_transaction::{BumpTransactionEvent, WalletSource};
Expand Down Expand Up @@ -246,31 +246,31 @@ fn archive_fully_resolved_monitors() {

// At this point, both nodes have no more `Balance`s, but nodes[0]'s `ChannelMonitor` still
// hasn't had the `MonitorEvent` that contains the preimage claimed by the `ChannelManager`.
// Thus, calling `archive_fully_resolved_channel_monitors` and waiting 4032 blocks will not
// result in the `ChannelMonitor` being archived.
// Thus, calling `archive_fully_resolved_channel_monitors` and waiting `ARCHIVAL_DELAY_BLOCKS`
// blocks will not result in the `ChannelMonitor` being archived.
nodes[0].chain_monitor.chain_monitor.archive_fully_resolved_channel_monitors();
assert_eq!(nodes[0].chain_monitor.chain_monitor.list_monitors().len(), 1);
connect_blocks(&nodes[0], 4032);
connect_blocks(&nodes[0], ARCHIVAL_DELAY_BLOCKS);
nodes[0].chain_monitor.chain_monitor.archive_fully_resolved_channel_monitors();
assert_eq!(nodes[0].chain_monitor.chain_monitor.list_monitors().len(), 1);

// ...however, nodes[1]'s `ChannelMonitor` is ready to be archived, and will be in exactly 4032
// blocks.
// ...however, nodes[1]'s `ChannelMonitor` is ready to be archived, and will be in exactly
// `ARCHIVAL_DELAY_BLOCKS` blocks.
nodes[1].chain_monitor.chain_monitor.archive_fully_resolved_channel_monitors();
assert_eq!(nodes[1].chain_monitor.chain_monitor.list_monitors().len(), 1);
connect_blocks(&nodes[1], 4031);
connect_blocks(&nodes[1], ARCHIVAL_DELAY_BLOCKS - 1);
nodes[1].chain_monitor.chain_monitor.archive_fully_resolved_channel_monitors();
assert_eq!(nodes[1].chain_monitor.chain_monitor.list_monitors().len(), 1);
connect_blocks(&nodes[1], 1);
nodes[1].chain_monitor.chain_monitor.archive_fully_resolved_channel_monitors();
assert_eq!(nodes[1].chain_monitor.chain_monitor.list_monitors().len(), 0);

// Finally, we process the pending `MonitorEvent` from nodes[0], allowing the `ChannelMonitor`
// to be archived 4032 blocks later.
// to be archived `ARCHIVAL_DELAY_BLOCKS` blocks later.
expect_payment_sent(&nodes[0], payment_preimage, None, true, false);
nodes[0].chain_monitor.chain_monitor.archive_fully_resolved_channel_monitors();
assert_eq!(nodes[0].chain_monitor.chain_monitor.list_monitors().len(), 1);
connect_blocks(&nodes[0], 4031);
connect_blocks(&nodes[0], ARCHIVAL_DELAY_BLOCKS - 1);
nodes[0].chain_monitor.chain_monitor.archive_fully_resolved_channel_monitors();
assert_eq!(nodes[0].chain_monitor.chain_monitor.list_monitors().len(), 1);
connect_blocks(&nodes[0], 1);
Expand Down
9 changes: 9 additions & 0 deletions lightning/src/sign/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -538,6 +538,15 @@ impl SpendableOutputDescriptor {
};
Ok((psbt, expected_max_weight))
}

/// Returns the outpoint of the spendable output.
pub fn outpoint(&self) -> OutPoint {
match self {
Self::StaticOutput { outpoint, .. } => *outpoint,
Self::StaticPaymentOutput(descriptor) => descriptor.outpoint,
Self::DelayedPaymentOutput(descriptor) => descriptor.outpoint,
}
}
}

/// The parameters required to derive a channel signer via [`SignerProvider`].
Expand Down
23 changes: 13 additions & 10 deletions lightning/src/util/sweep.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
//! sweeping them.

use crate::chain::chaininterface::{BroadcasterInterface, ConfirmationTarget, FeeEstimator};
use crate::chain::channelmonitor::ANTI_REORG_DELAY;
use crate::chain::channelmonitor::{ANTI_REORG_DELAY, ARCHIVAL_DELAY_BLOCKS};
use crate::chain::{self, BestBlock, Confirm, Filter, Listen, WatchedOutput};
use crate::io;
use crate::ln::msgs::DecodeError;
Expand All @@ -32,6 +32,9 @@ use bitcoin::{BlockHash, Transaction, Txid};

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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/// The state of a spendable output currently tracked by an [`OutputSweeper`].
#[derive(Clone, Debug, PartialEq, Eq)]
pub struct TrackedSpendableOutput {
Expand Down Expand Up @@ -71,13 +74,7 @@ impl TrackedSpendableOutput {

/// Returns whether the output is spent in the given transaction.
pub fn is_spent_in(&self, tx: &Transaction) -> bool {
let prev_outpoint = match &self.descriptor {
SpendableOutputDescriptor::StaticOutput { outpoint, .. } => *outpoint,
SpendableOutputDescriptor::DelayedPaymentOutput(output) => output.outpoint,
SpendableOutputDescriptor::StaticPaymentOutput(output) => output.outpoint,
}
.into_bitcoin_outpoint();

let prev_outpoint = self.descriptor.outpoint().into_bitcoin_outpoint();
tx.input.iter().any(|input| input.previous_output == prev_outpoint)
}
}
Expand Down Expand Up @@ -107,7 +104,11 @@ pub enum OutputSpendStatus {
latest_spending_tx: Transaction,
},
/// A transaction spending the output has been confirmed on-chain but will be tracked until it
/// reaches [`ANTI_REORG_DELAY`] confirmations.
/// reaches at least [`PRUNE_DELAY_BLOCKS`] confirmations to ensure [`Event::SpendableOutputs`]
/// stemming from lingering [`ChannelMonitor`]s can safely be replayed.
///
/// [`Event::SpendableOutputs`]: crate::events::Event::SpendableOutputs
/// [`ChannelMonitor`]: crate::chain::channelmonitor::ChannelMonitor
PendingThresholdConfirmations {
/// The hash of the chain tip when we first broadcast a transaction spending this output.
first_broadcast_hash: BlockHash,
Expand Down Expand Up @@ -530,7 +531,9 @@ where
// Prune all outputs that have sufficient depth by now.
sweeper_state.outputs.retain(|o| {
if let Some(confirmation_height) = o.status.confirmation_height() {
if cur_height >= confirmation_height + ANTI_REORG_DELAY - 1 {
// We wait at least `PRUNE_DELAY_BLOCKS` as before that
// `Event::SpendableOutputs` from lingering monitors might get replayed.
if cur_height >= confirmation_height + PRUNE_DELAY_BLOCKS - 1 {
log_debug!(self.logger,
"Pruning swept output as sufficiently confirmed via spend in transaction {:?}. Pruned descriptor: {:?}",
o.status.latest_spending_tx().map(|t| t.compute_txid()), o.descriptor
Expand Down
Loading