[Memory Optimization] Persist counterparty_claimable_outpoints

[G8XSU]

Issue: #3049

Problem

After every commitment is signed, counterparty_claimable_outpoints keeps on growing without bounds within a channel_monitor, with a new hashmap entry for each commitment_tx.

It poses two problems mainly:

1. Increased memory footprint, since all the active channel_monitors are stored in-memory.
2. Increased channel_monitor on-disk size, where these are currently stored.

We don’t want to keep storing outpoints that will only be used for revoked_tx/funding_spend, instead we would like to store them in cold storage and read them only when required.

Ideal outcome: After doing this, Ldk's memory footprint should be drastically decreased owing to removed in-memory hashmap entries of counterparty_claimable_outpoints

Solution

Right now,

counterparty_claimable_outpoints: HashMap<Txid, Vec<(HTLCOutputInCommitment, Option<Box<HTLCSource>>)>>

stores two things mainly,

1. List of htlc outputs per commitment_tx
2. and their corresponding htlcSource, which identifies htlc route and contains some information regarding ongoing payments using PaymentId that can be used to track pending/inflight payments.

HTLC Sources are also used to reconstruct pending_outbound_payments in channel_manager.

For commitment_tx’s htlc_outputs:

Most of the times, it is being used either for current_counterparty_commitment_txid or prev_counterparty_commitment_txid for which we need to keep it in-memory because it is not just for revoked_tx/funding_spend, those are for current ongoing commitment_signed exchanges.

Otherwise, it is used for revoked_tx/funding_spend.

So, for older commitment_tx outpoints, which are only useful for revoked_tx/funding_spend, we will persist them separately out of channel_monitor by using a trigger [here](

rust-lightning/lightning/src/chain/channelmonitor.rs

Lines 2545 to 2547 in d1ac071

	for &mut (_, ref mut source_opt) in self.counterparty_claimable_outpoints.get_mut(&txid).unwrap() {
	*source_opt = None;
	}

) and persist some kind of newly introducedClaimInfo against a commitment_tx which can be retrieved later.

Both read and write of ClaimInfo should be done in an async fashion, as we don’t need to block everything to handle this.

Motivation: This is inline with us providing a async interface for channel_monitor persistence. If we don’t do async then we will be blocked on persistence for every commitment_signed which is sub-optimal.

Async Interface options

As mentioned earlier, both read and write of ClaimInfo should be done using in an async fashion. And currently we don’t have a Rust-async KVStore interface, which, as I understand, we avoid due to bindings support in LDK.

So there are couple of options for Async interface:

1. Public trait, similar to Persist trait

   Similar to how we handle async channel_monitor persistence for both new/updated channel_monitors, we could define a new interface for ClaimInfo or re-use existing Persist interface for it. Client will need to return something likeInProgress and notify us via a function call just the current channel_monitor flow.

   Reading/writing ClaimInfo in Persist interface doesn’t fit very well within its scope, so we will have to create a new trait/interface.

   Cons: ChannelMonitor interface / InProgress handling and its custom async implementation is already a complex interface to understand for our users and introducing yet another similar interface can cause unnecessary pain with one more custom async interface. An events based interface seems more intuitive, re-uses existing events interface that clients are familiar with and can be handled out-of-band, slightly delayed arbitrarily.

2. Events based approach:

   Publish PersistClaimInfo event which can be handled async by clients, whenever they handle the event, we(chain_monitor) are notified via a function call and we remove in-memory entry for the claim from channel_monitor.

   Whenever we suspect a revoked_tx/funding_spend after seeing a matching tx in transactions_confirmed, we request ClaimInfo using RequestClaimInfo, and once we have it, we do rest of the handling for check_spend_counterparty_transaction or cancel_prev_commitment_claims

   Cons: These events are time sensitive, if RequestClaimInfo is not handled in time we will fail to broadcast justice_tx.

Preferred approach: Events, we can document that these events are time-sensitive, and the same applies for trait approach as well.

Caveats

• In get_claimable_balances, if funding_spend is confirmed/pending, we read counterparty_claimable_outpoints for funding_tx on every call to this function.
  This is slightly concerning for two reasons:

  1. We want to provide output of this function immediatly.
  2. It is user triggered and its occurrence might not be rare.

  As a workaround, once we have funding_spend_seen and ClaimInfo provided by user, we store it back again in hashmap. This solves it for times once we have ClaimInfo.

  But, we can't provide reliable get_claimable_balances in the time between funding_spend_seen and user providing us with ClaimInfo.

  get_claimable_balances would be slightly out-of-date until we have ClaimInfo provided to us, and that should be fine for now.

[G8XSU]

Based on above approach, adding tentative tasks to the issue:

• Minimize current set of reads to counterparty_claimable_outpoints by re-using existing get calls and minor refactors
• Introduce new event variants: PersistClaimInfo and RequestedClaimInfo (names tbd)
• Start publishing 2 new events, one in provide_secret and other in check_spend
• Add pub fn in chain-monitor that can be called after both of those events.
• Refactor check_spend_counterparty_transaction and cancel_prev_commitment_claims to act after we receive ClaimInfo
• Once funding_spent_seen and we receive ClaimInfo, Re-insert ClaimInfo in counterparty_claimable_outpoints, so that get_claimable_balances can account for it.

[wpaulino]

I favor option 2 for retrieving this data back. Even if we could use async/await with option 1, we still need to handle failures (temporary read fail for whatever reason), and we're also blocking the processing pipeline for new blocks on each monitor if we choose to await the read.

As for the issue with get_claimable_balances, we certainly need fetch the ClaimInfo back to generate the claims, but we might not need them to expose their balances. I had a look and noticed we still use the different types of HTLC balances (MaybeTimeout, ContentiousClaimable, MaybePreimage, etc.) for revoked HTLCs, rather than just using the existing CounterpartyRevokedOutputClaimable. Even if an HTLC is claimed via the timeout/preimage, we still have another chance to claim it via the revocation path. Also, it seems like all the data we would need to generate the balances we could just obtain from the confirmed funding transaction itself (assuming we also store the transaction and not just the txid)?

[G8XSU]

I favor option 2 for retrieving this data back

Yes, Option-2 is the approach i prefer as well. Matt also confirmed offline, so will be moving forward with this.

we're also blocking the processing pipeline for new blocks on each monitor if we choose to await the read.

For rust-async yes since we can't make whole stack async for now, but note that in option-1 as well we rely on user returning InProgress on write/read and later doing a function call.(similar to Persist)

we certainly need fetch the ClaimInfo back to generate the claims, but we might not need them to expose their balances.

Yeah makes sense, thanks for bringing it up.
Regarding different types on HTLC balances for revoked Htlcs and using the info from the confirmed funding transaction itself, i will look into understanding this in detail once i get to the related task.

[TheBlueMatt]

Would this remain true after #2618?

[wpaulino]

My understanding is that #2618 is only critical to get right while the channel is still operating. In the scenario being discussed here, we've already seen a revoked commitment from the counterparty confirm and we should just expose all of outputs (including HTLCs) as ours via CounterpartyRevokedOutputClaimable.

[TheBlueMatt]

Mm, right, good point, once its all funds from a stale revoked commitment tx its all our money, or at least will be once we have the ClaimInfo.