Skip to content

feat(core): allow serialization of SyncResponse#2232

Open
evanlinjin wants to merge 1 commit into
bitcoindevkit:masterfrom
evanlinjin:claude/happy-darwin-hkwqlc
Open

feat(core): allow serialization of SyncResponse#2232
evanlinjin wants to merge 1 commit into
bitcoindevkit:masterfrom
evanlinjin:claude/happy-darwin-hkwqlc

Conversation

@evanlinjin

Copy link
Copy Markdown
Member

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:

New Features:

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

Generated by Claude Code

Add `serde::Serialize`/`Deserialize` for `SyncResponse` so a connected
device can serialize a sync response and hand it to an air-gapped device
to apply independently.

`SyncResponse` holds a `TxUpdate` and an `Option<CheckPoint>`:

- `TxUpdate` gains a plain derive; its fields are flat collections so
  there is nothing self-referential to recurse over.
- `CheckPoint` is a reference-counted linked list, so a derived impl
  would recurse through `prev`/`skip` once per checkpoint and overflow
  the stack on long chains -- the same hazard that forced the
  hand-written `Drop` (bitcoindevkit#1634). It is instead serialized iteratively as a
  flat sequence of `(height, data)` pairs and rebuilt with
  `CheckPoint::from_blocks`, which re-derives the `skip`/`index`
  topology deterministically.

Tests cover round-tripping `CheckPoint`, `TxUpdate` and `SyncResponse`,
and assert (de)serialization is not recursive by round-tripping a long
chain on a deliberately small thread stack.

https://claude.ai/code/session_014BXMFRQP8qoGBghJcWzakG
@codecov

codecov Bot commented Jun 15, 2026

Copy link
Copy Markdown

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 10 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.

@evanlinjin evanlinjin self-assigned this Jun 15, 2026

@oleonardolima oleonardolima left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cACK b62c35c

Comment on lines +29 to 37
#[cfg_attr(
feature = "serde",
derive(serde::Serialize, serde::Deserialize),
serde(bound(
serialize = "A: serde::Serialize",
deserialize = "A: Ord + serde::Deserialize<'de>"
))
)]
#[non_exhaustive]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@oleonardolima oleonardolima moved this to Needs Review in BDK Chain Jun 17, 2026
@oleonardolima oleonardolima added this to the Chain 0.24.0 milestone Jun 17, 2026
@Dmenec

Dmenec commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Member

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.

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

Labels

None yet

Projects

Status: Needs Review

Development

Successfully merging this pull request may close these issues.

5 participants