Skip to content

Electrum chain source stop() discards registered Filter entries #1005

Description

@tnull

Discovered in #992 (but pre-existing)

Claude:

The new failure-path chain_source.stop() silently discards Electrum Filter registrations, breaking a failed-start → retry cycle for nodes with existing channels.

During Builder::build, channel monitors are loaded (src/builder.rs:1987) and register their watched txs/outputs with the chain source's Filter impl. For Electrum, these are queued in ElectrumRuntimeStatus::Stopped { pending_registered_txs, pending_registered_outputs } until start() applies
them. But ElectrumRuntimeStatus::stop() does *self = Self::new() (src/chain/electrum.rs:456), wiping both the running client and any pending registrations, and nothing re-registers them afterward — the outer ChainSource::registered_txids set survives but is never re-applied (and doesn't
carry scripts/outputs).

Concrete scenario: a node with existing channels calls start(), the fee-rate update or 0FC validation fails transiently (network blip) → the new error path calls chain_source.stop() → the app retries start(), which now succeeds — but the fresh ElectrumRuntimeClient's tx-sync watches nothing.
Confirmations/spends of existing funding outputs (e.g. a counterparty force-close) go undetected until a full process restart.

To be fair, a normal stop() → start() cycle already has this property on main, so it may be a known limitation — but pre-PR the failed-start path left the registrations intact (at the cost of the leak this PR fixes), and a failed first start with a retry loop is a much more common pattern
than a deliberate stop/restart. I'd suggest having ElectrumRuntimeStatus::stop() preserve the registration set (transition back to Stopped carrying the known registrations) rather than resetting to empty — that would fix both this path and the pre-existing stop/start case.

The long-term fix would still be to get rid of the start/stop mechanism entirely (see bitcoindevkit/rust-electrum-client#166), but as a hotfix we probably should make sure we trigger re-registration after restart (via load_outputs_to_watch

Activity

  1. added this to the 0.8 milestone on Jul 27, 2026
  2. Jolah1 commented on Jul 27, 2026

    @Jolah1
    Contributor

    I would like to work on this.
    Replaying only ChannelMonitor::load_outputs_to_watch appears incomplete because OutputSweeper also registers watched outputs and has no public method to reload all of them. My proposed fix is to keep a canonical, deduplicated registration inventory in ElectrumRuntimeStatus across both Started and Stopped: every register_tx/register_output updates the inventory, registrations are also forwarded to the live client when started, start() replays the inventory without draining it, and stop() drops only the client.
    I’ll add coverage for registrations made before and after start and for repeated stop/start cycles. Does that direction sound right?

  3. Jolah1 commented on Aug 3, 2026

    @Jolah1
    Contributor

    Hi @tnull, following up on this. since #992 now calls chain_source.stop() on every startup failure. As #992 doesn’t change the Electrum registration lifecycle, this issue appears independently actionable and would also make its failed- start retry path safe.

    should I proceed with the persistent, deduplicated registration inventory described above, based on main? I’d cover registrations made before and after start(), normal stop/start cycles, and failed - start retry behavior where practical.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions