feat: Add `Serialize` to `response` types by evanlinjin · Pull Request #20 · bitcoindevkit/electrum_streaming_client

evanlinjin · 2026-08-21T10:29:52Z

Description

Every type in src/response.rs derived serde::Deserialize only. That makes the responses decodable but not persistable.

The concrete motivation is downstream: bdk_electrum_streaming caches response::Tx values (a script's Electrum history) and now wants to persist that cache. Because response::Tx and friends are deserialize-only, it had to define a private mirror enum plus hand-written conversions in both directions purely to get the data back out. Deriving Serialize upstream removes that workaround entirely.

What this does

Adds serde::Serialize to every type in src/response.rs, plus PartialEq/Eq on the ones that lacked them so round trips can be asserted.

This is not just a derive change. Several fields decode through helpers in src/custom_serde.rs, and each one needed a matching serializer so the output is still valid Electrum wire data rather than the Rust type's own serde representation:

deserializer	new serializer	notes
`from_consensus_hex`	`to_consensus_hex`
`from_cancat_consensus_hex`	`to_cancat_consensus_hex`	re-concatenates before hex-encoding
`feerate_opt_from_btc_per_kb`	`feerate_opt_to_btc_per_kb`	`None` is written back as `-1.0`
`feerate_from_sat_per_byte`	`feerate_to_sat_per_byte`
`weight_from_vb`	`weight_to_vb`
`amount_from_btc`	`amount_to_btc`
`amount_from_sats`	`amount_to_sats`	integer sats, not `Amount`'s own serde
`amount_from_maybe_negative_sats`	`amount_to_maybe_negative_sats`
`all_inputs_confirmed_bool_from_height`	`all_inputs_confirmed_bool_to_height`	writes the `height` field as `0` / `-1`, never a bool

The existing #[serde(rename = ...)] attributes (hex, tx_hash) already apply to both directions, so field names are unchanged.

Tests

round_trip_responses: for every type, value → serde_json::to_value → deserialize → assert_eq! against the original.
round_trip_tx_preserves_variant: Tx is #[serde(untagged)], so this checks that both variants land back on themselves. They do — MempoolTx is tried first and only matches when fee is present, and ConfirmedTx catches the rest.
mempool_tx_serializes_height_not_bool: asserts the exact JSON shape, pinning height: 0 / height: -1. This is the case most likely to regress.

cargo test, cargo clippy --all-targets and cargo fmt --check all pass.

For the reviewer to decide

amount_from_maybe_negative_sats is lossy in a way that cannot be undone. GetBalanceResp::unconfirmed is documented as possibly negative, but the deserializer calls .unsigned_abs(), so the sign is discarded at decode time and Amount has no way to represent it. amount_to_maybe_negative_sats therefore always writes a non-negative number. A Rust-value round trip is lossless; a JSON→JSON round trip of a negative balance is not. Fixing that properly means changing unconfirmed to SignedAmount, which is a breaking change, so I left it alone and documented the asymmetry on the helper.
Float-backed conversions are approximate in both directions. EstimateFeeResp and FeePair go through f32; the tests use values that are exactly representable (0.001 BTC/kvB, 1 sat/vB), but arbitrary server-supplied rates may not survive a round trip bit-for-bit. This is pre-existing on the decode side; serializing just makes it visible in both directions.
Scope. I only touched src/response.rs as requested. The Deserialize-only types in src/protocol.rs and src/notification.rs are untouched — happy to extend if you want them too.

Checklists

All Submissions:

I've signed all my commits
I followed the contribution guidelines
I ran cargo test, cargo clippy --all-targets and cargo fmt --check before pushing

🤖 Generated with Claude Code

Every type in `response` derived `Deserialize` only, so a downstream crate that wanted to persist a cached response (e.g. a script's history of `response::Tx`) had to define a mirror type and hand-write the conversions just to get the data back out. Several fields decode through `custom_serde` helpers, so each one gains a symmetrically-named serializer that writes the Electrum wire representation back out rather than the Rust type's own serde: - `to_consensus_hex` / `to_cancat_consensus_hex` - `feerate_opt_to_btc_per_kb` (writes `-1.0` for `None`) - `feerate_to_sat_per_byte` - `weight_to_vb` - `amount_to_btc` / `amount_to_sats` / `amount_to_maybe_negative_sats` - `all_inputs_confirmed_bool_to_height` (writes `0` / `-1`, not a bool) `PartialEq`/`Eq` are derived on the types that lacked them so the round trip can be asserted in tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

noahjoeris

Have a look at PR #17 which removes the need for those helpers:

amount_from_btc / amount_to_btc
amount_from_sats / amount_to_sats
amount_from_maybe_negative_sats / amount_to_maybe_negative_sats

noahjoeris · 2026-08-21T12:39:11Z

    Ok(items)
 }

+pub fn to_cancat_consensus_hex<T, S>(values: &[T], serializer: S) -> Result<S::Ok, S::Error>


nit: this should be _concat_ right?

…ne owner 0a756eb chore: Bump `bdk_electrum_streaming` to `v0.6.0` (志宇) 90b4777 fix(bdk_electrum_streaming): Reject a transaction that is not the one asked for (志宇) 68d83da refactor(bdk_electrum_streaming)!: Give the chain and anchors one owner (志宇) 916f3d2 feat(bdk_electrum_streaming)!: Make the cache persistable (志宇) ffa9ffb fix(bdk_electrum_streaming)!: Make an anchor survive a reorg (志宇) 944dbf2 fix(bdk_electrum_streaming): Correct two SpkJob logging faults (志宇) 9f22bcd chore(bdk_electrum_streaming): Clear clippy across all targets (志宇) Pull request description: Fixes #12. Fixes #19. A reorg that moves a transaction into a different block of the **same height** leaves its Electrum script status untouched — the status is a hash over txid-height pairs — so the server never notifies the script hash. The anchor already delivered goes on pointing at a block that is no longer in the chain, nothing ever asks again, and the transaction stops being canonical for good. Repair has to be driven from the chain tip, which is the one thing that does report the eviction. Doing that showed the split was in the wrong place. Anchors lived in `SpkJob`, so every script resolved them against a tip none of them could move — hence the resolve-the-whole-set-each-pass rule — and the chain layer had to *synthesise script notifications* to drive its own reorg repair. When one layer fabricates the other's events to do its own work, the boundary is wrong. So `ConfirmationJob` replaces `ChainJob` and owns both the chain and the anchors, leaving `SpkJob` the history, the transactions in it and their prevouts. Header fetches batch into `blockchain.block.headers` instead of one `blockchain.block.header` per anchor. `State` stages a single `Update` that both job kinds write into and hands it over once *both* are finished, so a transaction is never published ahead of the chain that anchors it. `ConfirmationJob` finishing is not enough on its own: it keeps offering the update until the last script is done with it. It is held back only until every script has its *history*, not until the scripts finish: the job works from the heights those histories name, so a script still downloading its own transactions has already told it every block it needs. Waiting for the downloads serialised the header and proof fetches behind them for nothing. Anchor scope comes from the statuses `Subscriptions` holds rather than from whichever jobs happen to be live, so a reorg re-verifies every script and not just the last one to notify. `Cache` splits by who can reconstruct it. `Subscriptions` — per-script status and history — is persisted, because a status is a hash Electrum computes over the history it stands for that no wallet stores, and the server reports a history as it stands now, never again mentioning a transaction it dropped. `TxCache` — transactions, anchors, and which transactions paid a script — is not, because all of it is in the caller's wallet already; seeding it from wallet data is a caller's job, and a `TxCache` that starts empty is correct but re-downloads everything. `response::Tx` is `Deserialize`-only upstream, so histories round-trip through a private mirror until [bitcoindevkit/electrum_streaming_client#20](bitcoindevkit/electrum_streaming_client#20) lands. Two ways to settle on a chain the server has left are fixed too, and neither subsumes the other (#19). `ReqCoord::forget_job` drops the superseded job's in-flight requests, so the replacement's identical `request::Headers` is not deduplicated against an answer already on its way from the abandoned chain. And a header batch has to link up to the tip that was actually announced, so a batch answered from a chain we were never told about is abandoned rather than adopted — the request there is current, only the answer is not. The notification carries its own header, so that header is never re-fetched and instead sits at the top of the run the batch must join onto; the linkage walk is what makes a mismatch visible. Keying histories by status brought two faults of its own, both fixed here. A self-transfer with change pays two scripts the same transactions, so they share one status; dropping the outgoing status's history whenever one script moved on took it out from under the other, which then has no history and no notification coming, since its own status has not changed. The history is dropped only once no script answers to it. And a job waits for the history its notification's status stands for, so a server answering with a different one — it reorged since notifying, or the script's transactions dropped out entirely — left the job unable to find its own and asking again on every answer, unbounded and without backoff. `SpkJob::awaiting_history` compares the two, and the job is dropped rather than re-asked: the status the server actually holds is already on its way. ## Breaking changes (→ `0.6.0`) - `ChainJob` and `ChainJobOutcome` are gone, replaced by `ConfirmationJob`, `ConfirmationStage` and `ConfirmationProgress`. `JobId::Chain` is `JobId::Confirmation`, and `chain_job.rs` becomes `confirmation_job.rs`. - Named for what it owns rather than what it produces: it settles where transactions sit in the chain, and only two of an `Update`'s three parts come from it — `chain_update` and `tx_update.anchors`. The rest are the scripts', and `State` assembles them. - `State::advance` is `State::poll`. `SpkJob::poll` takes `&mut self` and returns `anyhow::Result<SpkProgress>`; `SpkJobStage` and `TxsJobStage` are replaced by `SpkStage` with an explicit `Done`. Additive: `SpkJob::awaiting_history`. - `SpkJob::poll` is fallible so that a server answering `blockchain.transaction.get` with a transaction whose outputs cannot reach an outpoint we know is spent brings the connection down rather than panicking the state thread. That `unimplemented!` is on `main`; it and the txid check below are the two fixes here to code this PR does not introduce. - `ConfirmationStage` gains `Idle`, so exhaustive patterns over it no longer compile. `Done` means the job resolved everything and still owes the caller an update, which it keeps offering until taken; `Idle` means nothing is owed — the update was taken, or the job was abandoned. One stage for both handed the same update over twice, and handed one over for a job that resolved nothing. - `ConfirmationJob::is_done` is now true only for `Done`, and `ConfirmationJob::set_idle` is added. `ConfirmationJob::reset` is removed — nothing called it. - `ConfirmationJob`'s setters are `set_tip`, `set_statuses` and `resolve_blocks` — the first two no-op on an unchanged value, and `set_statuses` is called with the same set on every poll, so `set_` says what they do. - `SpkHistories` is `Subscriptions`, keyed by status rather than script hash. `remove`/`insert`/`get`/`status` are `remove_spk`/`insert_spk`/`spk_history`/`spk_status`. Its height index (`spk_hashes_at_heights`, `prune`, `HEIGHT_INDEX_HORIZON`) is gone — nothing read it once repair stopped being driven through script jobs. - `Cache::{txs, anchors, spk_txids}` are now `Cache::tx_cache::{…}`, and a serialized `Cache` no longer carries them. - `Cache::failed_anchors` is removed, and nothing replaces it. A proof the server refuses or contradicts cancels the job rather than being recorded against our block — an error proves nothing about that block either way. A script notification now builds a `ConfirmationJob` as well as a tip does, so a disagreement below the reorg window recovers without waiting on a block. The trade is deliberate: one pair the server will not prove now withholds the whole update rather than publishing the rest without that anchor. - `JobRequest::GetHeader` is removed; nothing enqueues a single-header request any more. - `ReqCoord::pop` returns `Option<PoppedRequest>`; `ReqCoord::clear` is removed. Additive: `bump_chain_generation`, `forget_job`. - Two paths in the merkle-proof handler — a proof for a height not in our chain, and a block we hold with no header beside it — carry a `debug_assert!`. Both are reachable only if our own bookkeeping broke, so a debug build now panics where it used to log and continue. Release behaviour is unchanged. - A `blockchain.transaction.get` response whose transaction does not hash to the requested txid now errors the connection instead of being cached under the wrong id. Behavioural, not a signature change, but a server that was quietly getting away with it no longer will. Predates this branch. New dependency `serde`, already in the tree via `serde_json`. ## Testing `cargo test` — 24 deterministic (`tests/state.rs`), 5 live (`tests/env.rs`), 8 unit. Each fix was reverted in turn to confirm a test catches it. Two exceptions, stated rather than glossed: `ConfirmationStage::Done` restoring itself after `poll`'s `mem::take`, and the consistency guard parking in `Idle` rather than `Done`, are both reasoned rather than demonstrated. Reverting either produces redundant work rather than a wrong update, so no test catches them. Every one of the seven commits passes `cargo fmt --check`, `cargo check`, `cargo clippy --all-targets` and the full suite on its own. `reorg_to_same_height_block_refetches_anchor_live` confirms a tracked transaction, then re-mines it into a different block at the same height. **Fails on `main`**, passes here. `a_reorg_reanchors_every_script_not_just_the_last_to_notify` is the one the single-script test harness was structurally blind to: the server now serves any number of scripts, matching a transaction to a script by the outputs it pays. `a_history_that_cannot_match_the_job_is_not_re_asked` is capped at 50 requests so the unbounded re-ask fails the test rather than hanging it. `a_transaction_that_is_not_the_one_asked_for_is_rejected` answers a `blockchain.transaction.get` with a different, perfectly valid transaction and requires the connection to come down. `a_merkle_error_below_the_reorg_window_is_recovered_by_a_script_notification` refuses a proof at a height nothing will rewrite, so the tip never moves and the script notification is the whole of the recovery. It fails if a refused proof drops the job without something able to rebuild one. `confirmation_job_runs_ahead_but_the_update_waits_for_the_scripts` pins both halves of the gate: it answers a script's history but withholds its transaction, asserts the job reaches proof fetching anyway, then answers the proof and asserts nothing is published — and only once the transaction lands does one update arrive carrying both it and its anchor. Re-announcing the same tip afterwards must produce nothing. ## Known, not fixed here - One script stuck on its *history* withholds every other script's update. Inherent to staging a single `Update`; the trade this PR makes deliberately. Scripts stuck downloading transactions no longer block the confirmation job itself. - A single `(txid, height)` the server will not prove withholds the whole update, not just that anchor. The job is cancelled and rebuilt from the next notification rather than carrying a record of the refusal, so a server that refuses one proof indefinitely delivers nothing at all. Judged the server's fault to fix, and preferable to reporting a mined transaction as unconfirmed. - `TxCache` has no seeding constructor, so a reconnect refetches everything. Top commit has no ACKs. Tree-SHA512: ba07e4863767ce0f629306f52193972744206273db83d1694880015f941e780d28b77032f0c68b0e744ef36a4224c99a055b24070e9754fdf1e1754de63fc240

evanlinjin requested a review from oleonardolima as a code owner August 21, 2026 10:29

evanlinjin mentioned this pull request Aug 21, 2026

fix(bdk_electrum_streaming)!: Give the chain and anchors one owner evanlinjin/experiments#13

Merged

noahjoeris requested changes Aug 21, 2026

View reviewed changes

Navigation Menu

Uh oh!

feat: Add `Serialize` to `response` types - #20

feat: Add `Serialize` to `response` types#20
evanlinjin wants to merge 1 commit into
masterfrom
feat/serialize-response-types

evanlinjin commented Aug 21, 2026

Uh oh!

noahjoeris left a comment

Uh oh!

noahjoeris Aug 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

evanlinjin commented Aug 21, 2026

Description

What this does

Tests

For the reviewer to decide

Checklists

All Submissions:

Uh oh!

noahjoeris left a comment

Choose a reason for hiding this comment

Uh oh!

noahjoeris Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants