Retain Electrum Filterregistrations acrossstop/start`` - #1025
Open
Jolah1 wants to merge 2 commits into
Open
Conversation
`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>
|
I've assigned @tnull as a reviewer! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.