Add `archive_fully_resolved_monitors` to `ChainMonitor` #2964

jbesraa · 2024-03-24T05:58:54Z

An attempt to resolve #2884

lightning/src/chain/channelmonitor.rs

TheBlueMatt · 2024-03-26T17:01:47Z

lightning/src/chain/chainmonitor.rs

+			}
+		}
+		for funding_outpoint in to_remove {
+			monitors.remove(&funding_outpoint);


We want to do this at the storage layer too, but IMO we should do it by moving the monitor into a new pruned_monitors namespace, not deleting.

TheBlueMatt · 2024-03-26T17:02:30Z

lightning/src/chain/chainmonitor.rs

+		let mut monitors = self.monitors.write().unwrap();
+		let mut to_remove = Vec::new();
+		for (funding_outpoint, monitor_holder) in monitors.iter() {
+			if monitor_holder.monitor.is_stale() {


I'm not convinced we want to unilaterally remove everything. We've generally recommended users prune only after both the monitor has no claimable balances (ie is_stale()) and some time has passed. We could track that internally in the ChannelMonitor (and maybe should) but if we don't we should have users specify the list of monitors to prune manually.

jbesraa · 2024-03-27T16:24:00Z

lightning/src/util/persist.rs

@@ -58,6 +58,11 @@ pub const CHANNEL_MONITOR_PERSISTENCE_SECONDARY_NAMESPACE: &str = "";
 /// The primary namespace under which [`ChannelMonitorUpdate`]s will be persisted.
 pub const CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE: &str = "monitor_updates";

+/// The primary namespace under which [`ChannelMonitor`]s will be persisted.
+pub const PRUNED_CHANNEL_MONITOR_PERSISTENCE_PRIMARY_NAMESPACE: &str = "pruned_monitors";


@TheBlueMatt In which part of the code do you think I should use those namespaces? should I add a separate function or persist into this namespace before the remove?

jbesraa · 2024-03-27T16:25:56Z

lightning/src/util/test_utils.rs

@@ -501,6 +501,10 @@ impl<Signer: sign::ecdsa::WriteableEcdsaChannelSigner> chainmonitor::Persist<Sig
 		}
 		res
 	}
+
+	fn prune_persisted_channel(&self, _funding_txo: OutPoint) -> bool {
+		false


If I do self.persister.prune.. here I get some type errors.. didnt have time today to look into it any further

jbesraa · 2024-03-27T16:27:36Z

lightning/src/chain/chainmonitor.rs

+		let monitors_b = binding.first().unwrap();
+		let outpoint = monitors_b.0.clone();
+		dbg!(&outpoint);
+		// nodes[1].chain_monitor().unwrap().chain_monitor.prune_stale_channel_monitors(vec![outpoint]); // lock order problem


the test is still not there yet, I was hoping to at least run the prune_stale_channel_monitors function from here but got into a lock order problem

TheBlueMatt · 2024-03-27T19:01:36Z

lightning/src/chain/chainmonitor.rs

+	///
+	/// This is called when a channel is stale, ie we dont have balance to claim and its
+	/// closed.
+	fn prune_persisted_channel(&self, channel_funding_outpoint: OutPoint) -> bool;


IMO lets call this "archive" rather than prune?

Also, why bother with a return value?

TheBlueMatt · 2024-03-27T19:02:19Z

lightning/src/chain/chainmonitor.rs

+
+	/// Prune a channel's data.
+	///
+	/// This is called when a channel is stale, ie we dont have balance to claim and its


We should have a whole discussion of safety here and probably say something about how you really want to archive to hedge against LDK bugs but generally this is when a monitor is completely used up.

TheBlueMatt · 2024-03-27T19:02:44Z

lightning/src/chain/chainmonitor.rs

+	/// Prune a channel's data.
+	///
+	/// This is called when a channel is stale, ie we dont have balance to claim and its
+	/// closed.


Something about how all balances have converted to ClaimableOutput events?

TheBlueMatt · 2024-03-27T19:04:50Z

lightning/src/chain/channelmonitor.rs

@@ -1855,6 +1855,10 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitor<Signer> {
 		spendable_outputs
 	}

+	pub(crate) fn is_stale(&self) -> bool {
+		self.get_claimable_balances().is_empty()


Please also persist the block height at which we hit this here, then only return true if its old. Ie the first time this hits empty we should update a field that just says "oh, we first hit empty at height X", persist that new field, then every time this is called after that we debug_assert that the claimable balances remain empty, and then when the height X reaches, say, two weeks old, go ahead and return true.

TheBlueMatt · 2024-03-28T20:52:51Z

This needs a small rebase now. Will you have time to work on this today or tomorrow? We'd like to cut an RC of the next release tomorrow and would like to include this. If not, no big deal, but I may take it over.

lightning/src/chain/chainmonitor.rs

jbesraa · 2024-03-28T21:47:51Z

lightning/src/chain/chainmonitor.rs

+		for funding_txo in to_archive {
+			let channel_monitor = monitors.get(&funding_txo);
+			if let Some(channel_monitor) = channel_monitor {
+				if channel_monitor.monitor.is_stale() 


this if is safe because rust goes through it LTR, so the channel will be archived only if its stale, and it will be removed only if it was archived

jbesraa · 2024-03-28T21:49:57Z

lightning/src/util/persist.rs

@@ -536,6 +576,46 @@ where
 			}
 		}
 	}
+	/// Read an archived channel monitor.
+	fn read_archived_channel_monitor(&self, archived_monitor_name: &MonitorName) -> Result<(BlockHash, ChannelMonitor<<SP::Target as SignerProvider>::EcdsaSigner>), io::Error>  { 


this is actually not used, leftover from some experiments

Yea, I don't think we need to bother having this here currently. It may be useful for some users if we screw up but they can also just move things from one key to another to unarchive them.

jbesraa · 2024-03-28T21:51:50Z

This needs a small rebase now. Will you have time to work on this today or tomorrow? We'd like to cut an RC of the next release tomorrow and would like to include this. If not, no big deal, but I may take it over.

pushed new commits & rebased. I think the main thing that is missing now is testing. happy to complete this tomorrow.

codecov-commenter · 2024-03-28T21:58:28Z

Codecov Report

Attention: Patch coverage is 68.98734% with 49 lines in your changes are missing coverage. Please review.

Project coverage is 90.03%. Comparing base (1d2a27d) to head (4b55043).
Report is 45 commits behind head on main.

Files	Patch %	Lines
lightning/src/util/persist.rs	31.91%	31 Missing and 1 partial ⚠️
lightning/src/chain/channelmonitor.rs	64.51%	11 Missing ⚠️
lightning/src/util/test_utils.rs	69.23%	4 Missing ⚠️
lightning/src/chain/chainmonitor.rs	95.00%	1 Missing ⚠️
lightning/src/ln/monitor_tests.rs	97.87%	0 Missing and 1 partial ⚠️

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2964      +/-   ##
==========================================
+ Coverage   89.40%   90.03%   +0.62%     
==========================================
  Files         117      118       +1     
  Lines       96016   105242    +9226     
  Branches    96016   105242    +9226     
==========================================
+ Hits        85842    94753    +8911     
- Misses       7957     8297     +340     
+ Partials     2217     2192      -25

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

TheBlueMatt

Thanks for being willing to work on this on short notice!

TheBlueMatt · 2024-03-28T22:26:20Z

lightning/src/chain/channelmonitor.rs

@@ -934,6 +934,9 @@ pub(crate) struct ChannelMonitorImpl<Signer: WriteableEcdsaChannelSigner> {
 	/// Ordering of tuple data: (their_per_commitment_point, feerate_per_kw, to_broadcaster_sats,
 	/// to_countersignatory_sats)
 	initial_counterparty_commitment_info: Option<(PublicKey, u32, u64, u64)>,
+
+	/// The latest block height we've seen at the time of checking for stale channels.


Suggested change

/// The latest block height we've seen at the time of checking for stale channels.

/// The first block height at which we had no remaining claimable balances.

TheBlueMatt · 2024-03-28T22:26:50Z

lightning/src/chain/channelmonitor.rs

@@ -934,6 +934,9 @@ pub(crate) struct ChannelMonitorImpl<Signer: WriteableEcdsaChannelSigner> {
 	/// Ordering of tuple data: (their_per_commitment_point, feerate_per_kw, to_broadcaster_sats,
 	/// to_countersignatory_sats)
 	initial_counterparty_commitment_info: Option<(PublicKey, u32, u64, u64)>,
+
+	/// The latest block height we've seen at the time of checking for stale channels.
+	latest_stale_tip: Option<u32>,


Not sure the right name for this var, really.

Suggested change

latest_stale_tip: Option<u32>,

blanaces_empty_height: Option<u32>,

TheBlueMatt · 2024-03-28T22:27:41Z

lightning/src/chain/channelmonitor.rs

+	fn latest_stale_tip(&self) -> Option<u32> {
+		let inner = self.inner.lock().unwrap();
+		inner.latest_stale_tip
+	}
+
+	fn set_latest_stale_tip(&self, latest_stale_tip: Option<u32>) {
+		let mut inner = self.inner.lock().unwrap();
+		inner.latest_stale_tip = latest_stale_tip;
+	}


We don't need getters and setters for an internal field.

TheBlueMatt · 2024-03-28T22:28:37Z

lightning/src/chain/channelmonitor.rs

+			let two_weeks = 2016;
+			let is_below_threshold = self.current_best_block().height > latest_stale_tip + two_weeks;
+			is_below_threshold && self.get_claimable_balances().is_empty()
+		} else {
+			let best_block = self.current_best_block();
+			self.set_latest_stale_tip(Some(best_block.height));
+			false


Not quite sure what's going on here. The goal is to ensure that we've had get_claimable_balance empty for at least two weeks. Thus, we should (a) fetch get_claimable_balances, if its empty (1) set the field if its unset, or (2) if the field is set, return true if its two weeks old and (b) if the field is set make sure get_claimable_balances returned an empty set via debug_assertion.

TheBlueMatt · 2024-03-28T22:29:50Z

lightning/src/util/persist.rs

@@ -536,6 +576,46 @@ where
 			}
 		}
 	}
+	/// Read an archived channel monitor.
+	fn read_archived_channel_monitor(&self, archived_monitor_name: &MonitorName) -> Result<(BlockHash, ChannelMonitor<<SP::Target as SignerProvider>::EcdsaSigner>), io::Error>  { 


Yea, I don't think we need to bother having this here currently. It may be useful for some users if we screw up but they can also just move things from one key to another to unarchive them.

jbesraa · 2024-03-29T13:26:27Z

@TheBlueMatt I did cleanup the commits and completed the requested changes. I spent some time trying to test the archive_stale_channel_monitors function without success(error reference: https://github.com/lightningdevkit/rust-lightning/actions/runs/8481391972/job/23238659966?pr=2964#step:7:5206). There could be something wrong with the way I implemented the Persist trait for the TestPersistor.

Unfortunately, I have to drop off in a bit and wont have more time for this today. If you need this to be done today, feel free to take over, otherwise will continue tomorrow morning(EET).

TheBlueMatt

A few comments. Tomorrow is probably okay given we have a lot of other PRs to land too but if we make good progress today I'll push some updates here.

TheBlueMatt · 2024-03-29T14:57:58Z

lightning/src/chain/channelmonitor.rs

+	/// Checks if the monitor is stale, meaning it has not been updated with a new block data in a
+	/// substantial amount of time and thus has no outputs to watch, ie


This seems to imply that a monitor is "stale" if the user has forgotten to feed it block data for a while. Maybe instead of calling it stale we go with something like fully_resolved and reword the docs here to just talk about how all outputs have been claimed and get_claimable_balances is empty?

TheBlueMatt · 2024-03-29T14:58:33Z

lightning/src/chain/channelmonitor.rs

+	/// The first time this method is called it will save the current known height of the monitor
+	/// if all funds are claimed. Otherwise, if we yet to claim all funds, it will return false. If all funds
+	/// are claimed, it will return true if the monitor has not been updated with new block data in
+	/// a substantial amount of time referred as `threshold` and `balances_empty_height` is set.


I think we can just rephrase this whole paragraph as "will only return true once get_claimable_balances has been empty for at least two weeks.

TheBlueMatt · 2024-03-29T15:01:34Z

lightning/src/chain/channelmonitor.rs

+			// This if can be ture at least in the second time this method is called. we check if
+			// the monitor has not been updated with new block data to watch new ouputs in a
+			// substantial amount(2016 blocks) of time meaning the channel is probably closed and
+			// this monitor is not expected to be able to claim any funds on chain.


This comment seems to imply we think that we actually expect get_claimable_balances to gain new entries after going empty, but we strongly do not. Can you rephrase it to highlight that we expect this to never happen, but we handle the case here to ensure if there's a bug we don't spuriously prune a monitor.

TheBlueMatt · 2024-03-29T15:02:18Z

lightning/src/chain/channelmonitor.rs

+			// the monitor has not been updated with new block data to watch new ouputs in a
+			// substantial amount(2016 blocks) of time meaning the channel is probably closed and
+			// this monitor is not expected to be able to claim any funds on chain.
+			debug_assert!(is_all_funds_claimed, "Trying to check if monitor is stale before all funds are claimed. Aborting.");


This is great, but in release builds we should also handle the is_all_funds_claimed case - if we need an if is_all_funds_claimed { inner.balances_empty_height = None; balances_empty_height = None; }.

TheBlueMatt · 2024-03-29T15:02:39Z

lightning/src/chain/channelmonitor.rs

+		let is_all_funds_claimed = self.get_claimable_balances().is_empty();
+		let current_height = self.current_best_block().height;
+		let inner = self.inner.lock().unwrap();
+		let blanaces_empty_height = inner.blanaces_empty_height;


nit: I'm not sure there's any value in copying this into a local variable, it just means more stuff flying around.

TheBlueMatt · 2024-03-29T15:02:56Z

lightning/src/chain/channelmonitor.rs

+			// current height and start counting from there.
+			// This is to to make sure we don't consider the monitor stale on the first call to
+			// `is_stale` after all funds are claimed.
+			let mut inner = inner;


Please drop this and just make the top inner mut if you need it.

TheBlueMatt · 2024-03-29T15:03:30Z

lightning/src/util/persist.rs

+			&monitor 
+		) {
+			Ok(()) => {}
+			Err(_e) => return chain::ChannelMonitorUpdateStatus::UnrecoverableError // TODO: Should we return UnrecoverableError here?


Yea, that's correct.

TheBlueMatt · 2024-03-29T15:05:06Z

Note that CI is still failing.

valentinewallace

No major feedback, thanks for tackling this :)

lightning/src/chain/chainmonitor.rs

lightning/src/chain/channelmonitor.rs

lightning/src/chain/chainmonitor.rs

lightning/src/ln/monitor_tests.rs

jbesraa · 2024-04-16T17:00:17Z

Thanks @valentinewallace
Addressed your comments

lightning/src/util/persist.rs

valentinewallace · 2024-04-16T18:26:29Z

You have several instances of trailing whitespace in the top commit, git show should show you where it is.

lightning/src/chain/chainmonitor.rs

lightning/src/chain/channelmonitor.rs

lightning/src/chain/chainmonitor.rs

G8XSU · 2024-04-16T19:45:46Z

lightning/src/chain/channelmonitor.rs

+			(None, true) => {
+				// Claimed all funds but `balances_empty_height` is None. It is set to the
+				// current block height.
+				inner.balances_empty_height = Some(current_height);


should also log here.

we should add a log "Will archive fully resolved ChannelMonitor for funding txo {}, after 4032 blocks"
idea being it is well before the actual archive and can serve as warning.

Sorry I missed this one somehow.

lightning/src/chain/chainmonitor.rs

lightning/src/chain/channelmonitor.rs

G8XSU · 2024-04-16T20:41:54Z

lightning/src/chain/chainmonitor.rs

+	///
+	/// Archiving the data in a backup location (rather than deleting it fully) is useful for
+	/// hedging against data loss in case of unexpected failure.
+	fn archive_persisted_channel(&self, channel_funding_outpoint: OutPoint);


at multiple places i see error handling related to "got an error -> ignore"
should we consider returning a Result<(), io::Error> from here and ignore error where archive_persisted_channel is being used?

it could also be helpful for users implementing their own Persist trait, if we ignore it on our own.

Not sure why LDK swallowing a user-provided error is better than it being visible to the user that LDK will do nothing with their error.

currently, users will be questioning "what to do with error", while implementing this interface.
it would have been nice in the sense that we take the decision for them or handle the error if we can.

imo, a user facing interface should only be non-fallible if there is no way we can handle it.

anyway, not holding this PR for this, it is ok if we leave it like this.

imo, a user facing interface should only be non-fallible if there is no way we can handle it.

I think I take a bit of a different view - in this case we're not "handling it", but rather we're ignoring it - adding an error type would be confusing as it would imply to the user that we'll do something with the error and that they need to correctly propagate errors, whereas that's not actually true - the error will be ignored and if they have an error they should consider whether it is fatal for themselves.

G8XSU · 2024-04-16T20:43:46Z

lightning/src/ln/monitor_tests.rs

+	let (_, _, chan_id, funding_tx) =
+		create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 1_000_000);
+
+	nodes[0].node.close_channel(&chan_id, &nodes[1].node.get_our_node_id()).unwrap();


this test-case starts with a closed channel, is it worth it to attempt archiving during other states, and ensure we don't archive?
(could be helpful in case of catching a regression)

There are a handful of other tests in this file that at least test that get_claimable_balance is correct in all the states we could enumerate, so it should at least be safe there, we could add more assertions that the monitor isn't prunable in those tests later.

Checks if the monitor is fully resolved. Resolved monitor is one that has claimed all of its ouputs and balacnes. This function returns true only if `get_claimable_balances` has been empty for at least 2016 blocks.

TheBlueMatt · 2024-04-18T15:55:29Z

Went ahead and addressed the latest round of feedback and squashed - this is now the only remaining PR for the 0.0.123 RC

lightning/src/chain/chainmonitor.rs

lightning/src/util/persist.rs

Archives fully resolved channel monitors by adding them to a backup location and removing them from the primary storage & the monitor set. This is useful for pruning fully resolved monitors from the monitor set and primary storage so they are not reloaded on every new new block connection. We also add a new function, `archive_persisted_channel` to the `Persist` trait that writes the monitor to an archive storage and removes it from the primary storage.

TheBlueMatt · 2024-04-18T17:12:44Z

Addressed @valentinewallace's feedback.

valentinewallace

LGTM if @G8XSU is happy.

G8XSU

Lgtm !
Minor nits.

G8XSU · 2024-04-18T17:21:34Z

lightning/src/chain/channelmonitor.rs

+	/// its outputs and balances (i.e. [`Self::get_claimable_balances`] returns an empty set).
+	///
+	/// This function returns true only if [`Self::get_claimable_balances`] has been empty for at least
+	/// 2016 blocks as an additional protection against any bugs resulting in spuriously empty balance sets.


nit: doc update to 4032 blocks

G8XSU · 2024-04-18T18:20:37Z

lightning/src/chain/channelmonitor.rs

+			(None, true) => {
+				// Claimed all funds but `balances_empty_height` is None. It is set to the
+				// current block height.
+				inner.balances_empty_height = Some(current_height);


we should add a log "Will archive fully resolved ChannelMonitor for funding txo {}, after 4032 blocks"
idea being it is well before the actual archive and can serve as warning.

G8XSU · 2024-04-18T18:22:52Z

lightning/src/chain/chainmonitor.rs

+	///
+	/// Archiving the data in a backup location (rather than deleting it fully) is useful for
+	/// hedging against data loss in case of unexpected failure.
+	fn archive_persisted_channel(&self, channel_funding_outpoint: OutPoint);


currently, users will be questioning "what to do with error", while implementing this interface.
it would have been nice in the sense that we take the decision for them or handle the error if we can.

imo, a user facing interface should only be non-fallible if there is no way we can handle it.

anyway, not holding this PR for this, it is ok if we leave it like this.

TheBlueMatt · 2024-04-18T21:39:40Z

Merging since this has ACKs and then we can cut the release, will open a followup in a bit.

#2964 Followups

v0.0.123 - May 08, 2024 - "BOLT12 Dust Sweeping" API Updates =========== * To reduce risk of force-closures and improve HTLC reliability the default dust exposure limit has been increased to `MaxDustHTLCExposure::FeeRateMultiplier(10_000)`. Users with existing channels might want to consider using `ChannelManager::update_channel_config` to apply the new default (lightningdevkit#3045). * `ChainMonitor::archive_fully_resolved_channel_monitors` is now provided to remove from memory `ChannelMonitor`s that have been fully resolved on-chain and are now not needed. It uses the new `Persist::archive_persisted_channel` to inform the storage layer that such a monitor should be archived (lightningdevkit#2964). * An `OutputSweeper` is now provided which will automatically sweep `SpendableOutputDescriptor`s, retrying until the sweep confirms (lightningdevkit#2825). * After initiating an outbound channel, a peer disconnection no longer results in immediate channel closure. Rather, if the peer is reconnected before the channel times out LDK will automatically retry opening it (lightningdevkit#2725). * `PaymentPurpose` now has separate variants for BOLT12 payments, which include fields from the `invoice_request` as well as the `OfferId` (lightningdevkit#2970). * `ChannelDetails` now includes a list of in-flight HTLCs (lightningdevkit#2442). * `Event::PaymentForwarded` now includes `skimmed_fee_msat` (lightningdevkit#2858). * The `hashbrown` dependency has been upgraded and the use of `ahash` as the no-std hash table hash function has been removed. As a consequence, LDK's `Hash{Map,Set}`s no longer feature several constructors when LDK is built with no-std; see the `util::hash_tables` module instead. On platforms that `getrandom` supports, setting the `possiblyrandom/getrandom` feature flag will ensure hash tables are resistant to HashDoS attacks, though the `possiblyrandom` crate should detect most common platforms (lightningdevkit#2810, lightningdevkit#2891). * `ChannelMonitor`-originated requests to the `ChannelSigner` can now fail and be retried using `ChannelMonitor::signer_unblocked` (lightningdevkit#2816). * `SpendableOutputDescriptor::to_psbt_input` now includes the `witness_script` where available as well as new proprietary data which can be used to re-derive some spending keys from the base key (lightningdevkit#2761, lightningdevkit#3004). * `OutPoint::to_channel_id` has been removed in favor of `ChannelId::v1_from_funding_outpoint` in preparation for v2 channels with a different `ChannelId` derivation scheme (lightningdevkit#2797). * `PeerManager::get_peer_node_ids` has been replaced with `list_peers` and `peer_by_node_id`, which provide more details (lightningdevkit#2905). * `Bolt11Invoice::get_payee_pub_key` is now provided (lightningdevkit#2909). * `Default[Message]Router` now take an `entropy_source` argument (lightningdevkit#2847). * `ClosureReason::HTLCsTimedOut` has been separated out from `ClosureReason::HolderForceClosed` as it is the most common case (lightningdevkit#2887). * `ClosureReason::CooperativeClosure` is now split into `{Counterparty,Locally}Initiated` variants (lightningdevkit#2863). * `Event::ChannelPending::channel_type` is now provided (lightningdevkit#2872). * `PaymentForwarded::{prev,next}_user_channel_id` are now provided (lightningdevkit#2924). * Channel init messages have been refactored towards V2 channels (lightningdevkit#2871). * `BumpTransactionEvent` now contains the channel and counterparty (lightningdevkit#2873). * `util::scid_utils` is now public, with some trivial utilities to examine short channel ids (lightningdevkit#2694). * `DirectedChannelInfo::{source,target}` are now public (lightningdevkit#2870). * Bounds in `lightning-background-processor` were simplified by using `AChannelManager` (lightningdevkit#2963). * The `Persist` impl for `KVStore` no longer requires `Sized`, allowing for the use of `dyn KVStore` as `Persist` (lightningdevkit#2883, lightningdevkit#2976). * `From<PaymentPreimage>` is now implemented for `PaymentHash` (lightningdevkit#2918). * `NodeId::from_slice` is now provided (lightningdevkit#2942). * `ChannelManager` deserialization may now fail with `DangerousValue` when LDK's persistence API was violated (lightningdevkit#2974). Bug Fixes ========= * Excess fees on counterparty commitment transactions are now included in the dust exposure calculation. This lines behavior up with some cases where transaction fees can be burnt, making them effectively dust exposure (lightningdevkit#3045). * `Future`s used as an `std::...::Future` could grow in size unbounded if it was never woken. For those not using async persistence and using the async `lightning-background-processor`, this could cause a memory leak in the `ChainMonitor` (lightningdevkit#2894). * Inbound channel requests that fail in `ChannelManager::accept_inbound_channel` would previously have stalled from the peer's perspective as no `error` message was sent (lightningdevkit#2953). * Blinded path construction has been tuned to select paths more likely to succeed, improving BOLT12 payment reliability (lightningdevkit#2911, lightningdevkit#2912). * After a reorg, `lightning-transaction-sync` could have failed to follow a transaction that LDK needed information about (lightningdevkit#2946). * `RecipientOnionFields`' `custom_tlvs` are now propagated to recipients when paying with blinded paths (lightningdevkit#2975). * `Event::ChannelClosed` is now properly generated and peers are properly notified for all channels that as a part of a batch channel open fail to be funded (lightningdevkit#3029). * In cases where user event processing is substantially delayed such that we complete multiple round-trips with our peers before a `PaymentSent` event is handled and then restart without persisting the `ChannelManager` after having persisted a `ChannelMonitor[Update]`, on startup we may have `Err`d trying to deserialize the `ChannelManager` (lightningdevkit#3021). * If a peer has relatively high latency, `PeerManager` may have failed to establish a connection (lightningdevkit#2993). * `ChannelUpdate` messages broadcasted for our own channel closures are now slightly more robust (lightningdevkit#2731). * Deserializing malformed BOLT11 invoices may have resulted in an integer overflow panic in debug builds (lightningdevkit#3032). * In exceedingly rare cases (no cases of this are known), LDK may have created an invalid serialization for a `ChannelManager` (lightningdevkit#2998). * Message processing latency handling BOLT12 payments has been reduced (lightningdevkit#2881). * Latency in processing `Event::SpendableOutputs` may be reduced (lightningdevkit#3033). Node Compatibility ================== * LDK's blinded paths were inconsistent with other implementations in several ways, which have been addressed (lightningdevkit#2856, lightningdevkit#2936, lightningdevkit#2945). * LDK's messaging blinded paths now support the latest features which some nodes may begin relying on soon (lightningdevkit#2961). * LDK's BOLT12 structs have been updated to support some last-minute changes to the spec (lightningdevkit#3017, lightningdevkit#3018). * CLN v24.02 requires the `gossip_queries` feature for all peers, however LDK by default does not set it for those not using a `P2PGossipSync` (e.g. those using RGS). This change was reverted in CLN v24.02.2 however for now LDK always sets the `gossip_queries` feature. This change is expected to be reverted in a future LDK release (lightningdevkit#2959). Security ======== 0.0.123 fixes a denial-of-service vulnerability which we believe to be reachable from untrusted input when parsing invalid BOLT11 invoices containing non-ASCII characters. * BOLT11 invoices with non-ASCII characters in the human-readable-part may cause an out-of-bounds read attempt leading to a panic (lightningdevkit#3054). Note that all BOLT11 invoices containing non-ASCII characters are invalid. In total, this release features 150 files changed, 19307 insertions, 6306 deletions in 360 commits since 0.0.121 from 17 authors, in alphabetical order: * Arik Sosman * Duncan Dean * Elias Rohrer * Evan Feenstra * Jeffrey Czyz * Keyue Bao * Matt Corallo * Orbital * Sergi Delgado Segura * Valentine Wallace * Willem Van Lint * Wilmer Paulino * benthecarman * jbesraa * olegkubrakov * optout * shaavan

jbesraa commented Mar 24, 2024

View reviewed changes

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed Mar 26, 2024

View reviewed changes

jbesraa force-pushed the prune-stale-chanmonitor branch from b8b4199 to 3282321 Compare March 27, 2024 16:20

jbesraa commented Mar 27, 2024

View reviewed changes

TheBlueMatt reviewed Mar 27, 2024

View reviewed changes

TheBlueMatt added this to the 0.0.122 milestone Mar 28, 2024

jbesraa force-pushed the prune-stale-chanmonitor branch from 6059f76 to 9660f60 Compare March 28, 2024 21:43

jbesraa commented Mar 28, 2024

View reviewed changes

lightning/src/chain/chainmonitor.rs Outdated Show resolved Hide resolved

jbesraa commented Mar 28, 2024

View reviewed changes

TheBlueMatt reviewed Mar 28, 2024

View reviewed changes

jbesraa force-pushed the prune-stale-chanmonitor branch 8 times, most recently from acf2c77 to 6d58544 Compare March 29, 2024 13:17

jbesraa changed the title ~~Add prune_stale_channel_monitors to ChainMonitor~~ Add archive_stale_channel_monitors to ChainMonitor Mar 29, 2024

TheBlueMatt reviewed Mar 29, 2024

View reviewed changes

jbesraa force-pushed the prune-stale-chanmonitor branch 2 times, most recently from 4a2a20c to 36dbada Compare April 1, 2024 11:30

jbesraa force-pushed the prune-stale-chanmonitor branch 3 times, most recently from 513557a to f7a735d Compare April 12, 2024 20:46

jbesraa changed the title ~~Add archive_stale_channel_monitors to ChainMonitor~~ Add archive_fully_resolved_monitors to ChainMonitor Apr 12, 2024

TheBlueMatt added the Seeking Code Review label Apr 15, 2024

valentinewallace reviewed Apr 16, 2024

View reviewed changes

lightning/src/chain/chainmonitor.rs Show resolved Hide resolved

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

lightning/src/chain/chainmonitor.rs Outdated Show resolved Hide resolved

lightning/src/ln/monitor_tests.rs Show resolved Hide resolved

valentinewallace removed the Seeking Code Review label Apr 16, 2024

valentinewallace assigned valentinewallace and TheBlueMatt Apr 16, 2024

jbesraa force-pushed the prune-stale-chanmonitor branch 2 times, most recently from abb104e to 20151e3 Compare April 16, 2024 16:55

valentinewallace reviewed Apr 16, 2024

View reviewed changes

lightning/src/util/persist.rs Outdated Show resolved Hide resolved

lightning/src/util/persist.rs Outdated Show resolved Hide resolved

lightning/src/util/persist.rs Outdated Show resolved Hide resolved

G8XSU reviewed Apr 16, 2024

View reviewed changes

Add ChannelMonitor::is_fully_resolved function

9a1649c

Checks if the monitor is fully resolved. Resolved monitor is one that has claimed all of its ouputs and balacnes. This function returns true only if `get_claimable_balances` has been empty for at least 2016 blocks.

TheBlueMatt force-pushed the prune-stale-chanmonitor branch from 20151e3 to 7f40ba3 Compare April 18, 2024 15:54

valentinewallace reviewed Apr 18, 2024

View reviewed changes

lightning/src/chain/chainmonitor.rs Outdated Show resolved Hide resolved

lightning/src/util/persist.rs Outdated Show resolved Hide resolved

TheBlueMatt force-pushed the prune-stale-chanmonitor branch from 7f40ba3 to 4b55043 Compare April 18, 2024 17:12

valentinewallace approved these changes Apr 18, 2024

View reviewed changes

G8XSU approved these changes Apr 18, 2024

View reviewed changes

TheBlueMatt merged commit 195e666 into lightningdevkit:main Apr 18, 2024
15 of 16 checks passed

TheBlueMatt mentioned this pull request Apr 18, 2024

#2964 Followups #3003

Merged

G8XSU added a commit that referenced this pull request Apr 25, 2024

Merge pull request #3003 from TheBlueMatt/2024-04-2964-followups

8701b1b

#2964 Followups

TheBlueMatt mentioned this pull request Oct 13, 2024

Prune monitors with zero claimable balances upon persisting #2236

Closed

	/// The latest block height we've seen at the time of checking for stale channels.
	/// The first block height at which we had no remaining claimable balances.

	latest_stale_tip: Option<u32>,
	blanaces_empty_height: Option<u32>,

		/// Checks if the monitor is stale, meaning it has not been updated with a new block data in a
		/// substantial amount of time and thus has no outputs to watch, ie

Add archive_fully_resolved_monitors to ChainMonitor #2964

Add archive_fully_resolved_monitors to ChainMonitor #2964

Conversation

jbesraa commented Mar 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Mar 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbesraa commented Mar 28, 2024

codecov-commenter commented Mar 28, 2024 • edited Loading

Codecov Report

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbesraa commented Mar 29, 2024

TheBlueMatt left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Mar 29, 2024

valentinewallace left a comment

Choose a reason for hiding this comment

jbesraa commented Apr 16, 2024

valentinewallace commented Apr 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Apr 18, 2024

TheBlueMatt commented Apr 18, 2024

valentinewallace left a comment

Choose a reason for hiding this comment

G8XSU left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Apr 18, 2024

Add `archive_fully_resolved_monitors` to `ChainMonitor` #2964

Add `archive_fully_resolved_monitors` to `ChainMonitor` #2964

codecov-commenter commented Mar 28, 2024 •

edited

Loading

TheBlueMatt left a comment •

edited

Loading