Retain Electrum Filterregistrations acrossstop/start`` - #1025
Conversation
|
I've assigned @tnull as a reviewer! |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Thanks for looking into this!
| Self::Stopped { pending_registered_txs, .. } => { | ||
| pending_registered_txs.push((*txid, script_pubkey.to_owned())) | ||
| }, | ||
| self.registered_txs.insert(*txid, script_pubkey.to_owned()); |
There was a problem hiding this comment.
When we do this, we should deduplicate the entries to make sure we're not re-registering all the entries every time.
Same below
There was a problem hiding this comment.
When we do this, we should deduplicate the entries to make sure we're not re-registering all the entries every time.
Same below
These dedupe by construction, the inventory is a HashMap<Txid, ScriptBuf> / HashSet rather than the Vecs it replaces, so start replays each entry exactly once. (The hunk cuts off right at the insert, so the forwarding branch below it may not have been visible.)
If you meant pruning entries that are no longer needed, I don't think we can from here: whether a tx is buried or an output spent lives in lightning-transaction-sync's private SyncState, which is dropped with the client on stop.
So we do re-register confirmed entries on each start, the same thing ChannelMonitor::load_outputs_to_watch does on every process restart. ChainSource::registered_txids has the same property today.
Let me know if you meant something else.
There was a problem hiding this comment.
These dedupe by construction, the inventory is a HashMap<Txid, ScriptBuf> / HashSet rather than the Vecs it replaces, so start replays each entry exactly once. (The hunk cuts off right at the insert, so the forwarding branch below it may not have been visible.)
Ah, lol, duh. Sorry, brain wasn't working right there.
If you meant pruning entries that are no longer needed, I don't think we can from here: whether a tx is buried or an output spent lives in lightning-transaction-sync's private SyncState, which is dropped with the client on stop.
So we do re-register confirmed entries on each start, the same thing ChannelMonitor::load_outputs_to_watch does on every process restart. ChainSource::registered_txids has the same property today.
Yeah, real pruning would likely require https://git.rust-bitcoin.org/lightningdevkit/rust-lightning/issues/1664 - long standing issue.
`ElectrumRuntimeStatus::stop` reset itself to `Stopped` with empty pending registration vectors, dropping the `ElectrumRuntimeClient` and, with it, the `ElectrumSyncClient` that owns the registered transactions and outputs. `start` then drained the pending vectors, so nothing survived even a single cycle. As `ChannelMonitor`s only register their watched transactions and outputs while being loaded in `Builder::build`, a `stop`/`start` cycle that doesn't rebuild the node left the chain source with no registrations at all, i.e., the node would no longer learn about confirmations or spends of, e.g., its funding outputs. Note we can't simply re-register from `ChannelMonitor::load_outputs_to_watch` on `start` either, as, e.g., the `OutputSweeper` also registers outputs it wants to see spent. Here we therefore keep a canonical, deduplicated registration inventory in `ElectrumRuntimeStatus` that is maintained whether we're started or not: `register_tx`/`register_output` always record the entry and additionally forward it to the client if one is live, `start` replays the inventory to the fresh client without consuming it, and `stop` merely drops the client. This also restores parity with the Esplora chain source, whose `tx_sync` is long-lived and hence never loses its registrations. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Open a channel, leave the funding transaction unconfirmed, then repeatedly stop and start the node before confirming it. The node can only emit `ChannelReady` if its chain source replayed the `Filter` registrations on every `start`, so this fails without the preceding fix (the node times out waiting for the event) and passes with it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
8805233 to
c9bd85e
Compare
3 participants
Fixes #1005.
Problem
ElectrumRuntimeStatus::stop reset itself via *self = Self::new(), which dropped the ElectrumRuntimeClient and the pending-registration vectors. Since the client owns the ElectrumSyncClient, which in turn owns its own copy of the
registered transactions and outputs, all Filter state was gone. start then drain(..)ed the pending vectors, so registrations didn't survive even a single cycle.
This matters because ChannelMonitors only register their watched transactions and outputs while being loaded in Builder::build. A stop/start cycle that doesn't rebuild the node therefore leaves the chain source with no
registrations at all, and the node stops learning about confirmations and spends of, e.g., its funding outputs.
As discussed on the issue, replaying ChannelMonitor::load_outputs_to_watch on start wouldn't be sufficient either: the OutputSweeper also registers outputs it wants to see spent, and exposes no way to reload them.
The Esplora chain source isn't affected, as its tx_sync is long-lived and never torn down — so this also restores parity between the two backends.
Fix
ElectrumRuntimeStatus becomes a struct holding an Option<Arc> alongside a canonical, deduplicated registration inventory (HashMap<Txid, ScriptBuf> and HashSet) that is maintained regardless of whether we're currently started:
Note the inventory intentionally retains entries for already-confirmed transactions. That mirrors what already happens on every process restart via monitor load, and matches the existing behaviour of ChainSource::registered_txids, which is likewise never pruned.
client() keeps its existing Option<Arc> signature, so all call sites are untouched.
Testing
Adds electrum_registrations_survive_chain_source_restart: it opens a channel, leaves the funding transaction unconfirmed, then stops and starts the node repeatedly before confirming it. The node can only emit ChannelReady if the chain source replayed its registrations on every start — the repetition specifically covers the non-draining behaviour.
Verified the test fails on current main (node times out after 60s waiting for ChannelReady) and passes with this fix. The other Electrum-backed tests still pass.
This change was developed with the assistance of Claude Code. The design, review, and testing decisions are my own.