Refactor `Confirm` interface around per-block changes

[tnull]

Currently, Confirm allows to call best_block_updated/transactions_confirmed/transaction_unconfirmed to be called independently, and even without prescribing concrete ordering requirements for the former two.

This can lead to indeterministic behavior/races. For example, we got a user report of a newly opened channel being force-closed with ClosureReason::FundingTimeOut, if the node would restart past the 2016 blocks timeout described in BOLT 2, because lightning-transaction-sync calls best_block_updated before transactions_confirmed.

While we could fix this particular race as a one-off, the real fix is a refactoring of Confirm to only take a single best_block_updated (or apply_block_changes) method that would take an UpdatedBlock object describing all the changes that happend as part of a particular block. We should then internally make sure that these changes are applied exactly in the order they need to be.

[EthanYuan]

Thank you so much for the detailed follow-up and for opening an issue. It's great to see our analysis was on the right track.

We agree with your cautious approach. As you pointed out, simply reordering the calls in EsploraSyncClient might have unforeseen consequences, and we understand that a proper refactoring of the Confirm interface is the most robust long-term solution.

We will be following issue #3867 closely for updates.

Thanks for the quick follow-up and the clear explanation.

[rose2221]

Can i work on this?

[TheBlueMatt]

I'm not really sure what we can do here from an interface perspective if we want to keep things - if we haven't seen a funding transaction confirm and its been 2016 blocks, we need to FC. The only way to avoid this issue is to have transactions_confirmed calls come before best_block_updated calls (and remove implicit best_block_updated calls, which exist in some place but not everywhere, we could double-check that they won't cause issues, at least here I think they do not exist). ISTM this would be a fine change, though.

While we could fix this particular race as a one-off, the real fix is a refactoring of Confirm to only take a single best_block_updated (or apply_block_changes) method that would take an UpdatedBlock object describing all the changes that happend as part of a particular block. We should then internally make sure that these changes are applied exactly in the order they need to be.

What would the difference be between the Confirm interface and Listen, at that point?

[tnull]

Can i work on this?

Ah, sorry to say that this task might be a bit too intricate for is a first time contributor. I’ll probably pick it up myself.

[tnull]

The only way to avoid this issue is to have transactions_confirmed calls come before best_block_updated calls

Right.

What would the difference be between the Confirm interface and Listen, at that point?

The difference would be that we'd still not require to connect/disconnect blocks in a row. Essentially, we'd keep all properties of Confirm but would enforce the ordering requirements internally, while providing a failsafe API. Note we currently require to call transactions_unconfirmed/transactions_confirmed in chain order anyways. Rather than leaving it to the user to call them in the right order, we can create a BlockEvent API or whatever. I think we discussed this previously somewhere in the context of reducing ChannelMonitor persistence, as there we also couldn't lean on any guarantees in which order transactions_confirmed/best_block_updated would be called.

[TheBlueMatt]

The difference would be that we'd still not require to connect/disconnect blocks in a row. Essentially, we'd keep all properties of Confirm but would enforce the ordering requirements internally, while providing a failsafe API. Note we currently require to call transactions_unconfirmed/transactions_confirmed in chain order anyways. Rather than leaving it to the user to call them in the right order, we can create a BlockEvent API or whatever.

Yea, I vaguely recall that. More safety would definitely be nice, not 100% sure I see exactly what you're thinking but definitely happy to see where this lands.