Prune monitors with zero claimable balances upon persisting

[wpaulino]

Currently, users can decide to prune their ChannelMonitors if a call to get_claimable_balances returns empty. This is only safe to do so in certain scenarios though, as we can also return empty if the channel is either still unconfirmed or it has confirmed but we don't have a to_self balance (like in the case of an inbound channel) and no HTLCs are present in our latest commitment transaction.

[TheBlueMatt]

Hmm, so I think its safe as long as (a) the channel is closed (ie channelmanager doesnt know about it) and (b) the balances are empty, at least what's what we've been telling users.

We should, however, actually make use of this in our Persister stuff.

[TheBlueMatt]

I guess actually with the exception of 0confs where the funding may not be confirmed but the channel closed? Is there a bug there?

[wpaulino]

Actually, it looks like we always push a ClaimableOnChannelClose balance (even if it's actually zero, which it can be on an inbound channel we've never received a payment on) when we haven't seen a spend onchain, so it should be safe to prune once there aren't any balances left (even 0-confs), assuming that behavior doesn't change (though it probably should?).

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

Line 1837 in 0e8da58

res.push(Balance::ClaimableOnChannelClose {

[TheBlueMatt]

Nice okay, good to know. Yea, I don't think we need to change that, I mean its dumb that we'll hang around to a monitor until our peer claims their money, but its easier than running the risk of removing a monitor too early before the channel is closed?

So I think this issue should just be re-titled to refer to actually using this in the persister?

[wpaulino]

So I think this issue should just be re-titled to refer to actually using this in the persister?

Yeah. We'll still need a way to clean up timed out funding attempts, otherwise we'll always hold on to their ChannelMonitors. Though I guess that's a separate issue.

[TheBlueMatt]

Note that async monitor updates will also throw a wrinkle into this - #2167 (comment)

[wpaulino]

There's also another case pointed out here: when a revoked commitment confirms and the counterparty claims all of the outputs, we still keep the claimable balances around preventing us from pruning.

[TheBlueMatt]

To summarize, aside from the one async monitor update issue linked above (ie only an issue for those returning ChannelMonitorUpdateStatus::InProgress from a persist method), the only known issues are cases where we may fail to have an empty balance set even though we have no claimable balances. ie it is otherwise considered safe to remove monitors after get_claimable_balaces returns an empty set. For safety, I'd recommend waiting until two weeks or a month after get_claimable_balances is returning an empty set before pruning.

[TheBlueMatt]

Closing as completed in #2964. Tracking the one async issue mentioned in #3052