Skip to content

fix(electrum): serve server.features independently of peer discovery - #265

Open
HusseinAdeiza wants to merge 1 commit into
Blockstream:new-indexfrom
HusseinAdeiza:fix/electrum-server-features-no-discovery
Open

HusseinAdeiza wants to merge 1 commit into
Blockstream:new-indexfrom
HusseinAdeiza:fix/electrum-server-features-no-discovery

Conversation

@HusseinAdeiza

@HusseinAdeiza HusseinAdeiza commented Oct 6, 2026 •

Copy link
Copy Markdown

Closes #229

server.features was tied to peer discovery two ways: the dispatch arm is #[cfg(feature = "electrum-discovery")], and even inside gated builds the handler unwrapped the discovery manager and errored ("discovery is disabled", -32603) whenever the server was started without --electrum-public-hosts. Electrum >= 4.7.0 calls server.features on connect, so a default-feature electrs build with no public hosts is simply unreachable from the wallet (spesmilo/electrum#10281, reported in the issue).

ServerFeatures itself never needed discovery: the struct is already ungated, and all the data (hosts, version, genesis, protocol range) comes from the config. This builds the features once in RPC::start, hands every connection an Arc<ServerFeatures>, and registers the method in every build. Discovery stays opt-in, exactly as before: it is still only started when public hosts are set, and the manager gets the same features value. The electrum_public_hosts config field loses its cfg gate so non-discovery builds can still report real hosts (parsed as None there, since the CLI flag lives in the gated block).

This mirrors the fix mempool/electrs carries in PR 150 (merged upstream there on 2026-06-03).

Tests: new test_electrum_server_features_without_public_hosts in tests/electrum.rs runs a raw TCP server.features against the default harness config (hosts = None), same pattern as test_electrum_raw. On current master that request fails, -32603 inside gated builds or unknown-method with --no-default-features; with the patch it returns the features object.

cargo check --all-targets passes with default features and with --no-default-features. The new integration test needs the electrsd harness to fetch bitcoind, which is heavy to provision locally; it will run in CI here, and I have the same command green locally if it finishes before review.

@EddieHouston

Copy link
Copy Markdown
Collaborator

Thanks for opening this PR. This looks like a faithful port of mempool/electrs#150.

One edge case shared with that implementation: genesis_hash(config.network_type) returns an all-zero placeholder for Liquid testnet and regtest. The helper’s comment assumes it is only used for discovery, but this change exposes it through server.features in every build.

Could we use the indexed block hash at height 0 instead? That would also handle configurable Elements regtest genesis blocks. A test asserting the actual genesis hash would catch this; the new test currently only checks that it’s a string and is disabled for Liquid.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

server.features should not be tied to peer discovery (returns -32603 when --electrum-public-hosts unset)

2 participants