feat(core): allow serialization of SyncResponse

[evanlinjin]

Description

Adds serde::Serialize / serde::Deserialize for SyncResponse, enabling air-gapped workflows: a connected (online) device can sync, serialize the SyncResponse, and hand it to a disconnected (offline) device which deserializes and applies it independently. This is the goal of #1954, implemented without recursion.

SyncResponse is made up of a TxUpdate and an Option<CheckPoint>, so both gain serde support:

• TxUpdate — a plain #[derive(Serialize, Deserialize)]. Its fields are flat collections, so there is nothing self-referential to recurse over. An Ord bound is added on A for deserialization so that anchors: BTreeSet<(A, Txid)> can be rebuilt.

• CheckPoint — this is the interesting one. A CheckPoint is a reference-counted linked list (prev + skip pointers), so a derived implementation would recurse one stack frame per checkpoint and overflow the stack on long chains — the exact hazard that already forced a hand-written Drop (example_bitcoind_rpc_polling sync command ends with stack overflow #1634). It is therefore implemented by hand:

  • Serialize: walk the chain iteratively via CheckPoint::iter() and emit a flat sequence of (height, data) pairs.
  • Deserialize: collect the sequence and rebuild with CheckPoint::from_blocks(..), which re-derives the skip/index topology deterministically.

  The skip/index topology is intentionally not serialized — it is reconstructed on load, keeping the encoding minimal and both directions iterative.

Notes to the reviewers

• This is an alternative take on core: allow serialization of SyncResponse #1954 that addresses the "serializing CheckPoints is a bit odd since it is a linked list" concern raised in review: the linked list is flattened to a (height, data) sequence and rebuilt, so neither serialization nor deserialization recurses.
• checkpoint_serde_is_not_recursive round-trips a 10k-checkpoint chain on a deliberately small (128 KiB) thread stack, mirroring the existing checkpoint_drop_is_not_recursive test — a recursive impl would overflow it.
• serde_json is added only as a dev-dependency of bdk_core, for the round-trip tests.
• Scope is intentionally limited to SyncResponse to match core: allow serialization of SyncResponse #1954. FullScanResponse can follow the exact same pattern as a trivial follow-up if desired.

Changelog notice

• Added serde::Serialize and serde::Deserialize implementations for SyncResponse, TxUpdate, and CheckPoint (under the serde feature). CheckPoint is (de)serialized iteratively as a flat list of (height, data) blocks to avoid recursing over its linked-list structure.

Checklists

All Submissions:

• I followed the contribution guidelines

New Features:

• I've added tests for the new feature
• I've added docs for the new feature

---

Generated by Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 98.55072% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.11%. Comparing base (47556ab) to head (b62c35c).
⚠️ Report is 19 commits behind head on master.

Files with missing lines	Patch %	Lines
crates/core/src/checkpoint.rs	96.29%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2232      +/-   ##
==========================================
+ Coverage   77.69%   79.11%   +1.42%     
==========================================
  Files          29       30       +1     
  Lines        5801     6047     +246     
  Branches      271      279       +8     
==========================================
+ Hits         4507     4784     +277     
+ Misses       1223     1187      -36     
- Partials       71       76       +5     

Flag	Coverage Δ
rust	79.11% <98.55%> (+1.42%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[oleonardolima]

cACK b62c35c

[oleonardolima]

nit: are we looking to have forward-compatibility in this deserialization process ? if that's the case, it's a good idea to add serde(default) for each field.

[Dmenec]

Quick (maybe naive) question... I thought an air-gapped device only signed transactions, why does it need the checkpoint/chain data too? What does it do with it on the offline side?

[thunderbiscuit]

Linking to our issue here because it's relevant: bitcoindevkit/bdk-ffi#970.

I was considering adding a serialization layer just for bindings so that other clients could be built on top of us without requiring we ship them in the main "bindings" library, so as not to bloat it unecessarily (at the moment all clients must ship into one, big dependency). This makes it less attractive to build and test alternative clients with the bindings (Floresta for example). I am not commenting on the PR itself, but rather just giving a thumbs up and saying "this feature is useful!" with some extra context.

Also, even if this only ends up in SyncRequest we can use workarounds to ensure clients do in fact perform full syncs even sort of "manually", so not a blocker there for those who would want to use it.

[evanlinjin]

Quick (maybe naive) question... I thought an air-gapped device only signed transactions, why does it need the checkpoint/chain data too? What does it do with it on the offline side?

Maybe "air-gapped" might not be the right terminology. Any hardware device that has no access to internet, and wishes to display a balance, UTXO set, transactions, etc. without an internet connection would benefit from this feature.

[tvpeter]

ACK b62c35c

nit: I noticed that all the serde tests uses D=BlockHash and since the default trait impl for BlockHash::prev_blockhash() is None, the validation for Deserialize inside the push (from_blocks) is not covered in the tests.

Aside that, since this PR is scoped to SyncResponse, I'll be happy to add same for FullScanResponse if would want it added.

Thank you.

[afurious]

But "well-linked" is only checked when D::prev_blockhash() returns Some(_). That's true for D = Header. It is not true for D = BlockHash — and BlockHash is the default type parameter for both CheckPoint and SyncResponse::chain_update, i.e. the type this PR's own tests exercise and the type most callers will actually serialize. ToBlockHash for BlockHash doesn't override prev_blockhash(), so it inherits the trait's default of None, and push/extend fall back to checking height ordering only.

Concrete consequence: an air-gapped device that receives a tampered or corrupted SyncResponse with D = BlockHash will happily deserialize a chain of blocks that are ascending in height but not actually connected to one another (no cryptographic linkage was ever present to check). This isn't a bug this PR introduced — it's an existing property of CheckPoint — but this PR is precisely the one whose stated purpose is "hand this data across a trust boundary to an offline device," which is exactly the scenario where an overpromising error message matters. Worth either:

• softening the message to something like "blocks must be non-empty and strictly ascending in height (and, for types with linkage data, correctly linked)", or
• adding a doc note on SyncResponse's serde impls stating that round-tripping preserves structure but does not add hash-chain integrity beyond what D itself provides.

Finding: the recursion-guard test passes, but not entirely for the reason its comment claims

checkpoint_serde_is_not_recursive mirrors checkpoint_drop_is_not_recursive in chain length (10,240 checkpoints) but uses a 4x larger stack (128 KiB vs. the drop test's 32 KiB). Worth knowing: serde_json has its own default recursion depth limit (128 nested containers) as a built-in anti-stack-overflow guard. If a future refactor accidentally reintroduced a naive recursive Serialize/Deserialize (one nested JSON array per checkpoint), serde_json would likely trip its own recursion-limit error and return a clean Err well before the process ever got close to exhausting a 128 KiB stack — so the test would still fail (via .expect() panicking on that Err), just not via the stack-overflow crash the comment describes. Net effect: the test is still a valid regression guard, it just has two independent safety nets rather than the one it advertises. Not a blocker, just worth knowing so nobody "fixes" the test by shrinking the stack further expecting a SIGSEGV that may never come.

Smaller observations

• Error info is discarded on deserialize failure. CheckPoint::from_blocks returns Result<Self, Option>, distinguishing "empty input" from "bad linkage, here's how far we got." The map_err(|_| ...) throws that distinction away in favor of one generic message. Fine for now, but if air-gapped sync ever needs debugging in the field, that partial-chain info is gone.
• No test for the deserialize error path. All three new tests (checkpoint_serde_round_trip, checkpoint_serde_is_not_recursive, sync_response_serde_round_trip, tx_update_serde_round_trip) are happy-path round trips. Given the feature's entire premise is "accept data that crossed an untrusted or physically insecure channel," a test asserting that a corrupted/out-of-order block sequence actually produces Err (not just that valid data round-trips) would directly validate the one line of defense this feature has.
• No single-node chain test. All round-trip tests start from genesis and push up to N more blocks; nobody round-trips a bare CheckPoint::new(..) with zero pushes (sequence length 1). Cheap to add, exercises the index == 0 edge of the size-hint math.
• Scope-limiting FullScanResponse is a reasonable call, explicitly flagged by the author as a following-PR candidate. No objection, just flagging it's the obvious next ask in review.