Skip to content

perf(rpc): stop storing the block header in every block record - #90

Open
rabbitson87 wants to merge 2 commits into
perf/rpc-block-record-headerfrom
perf/rpc-record-header-from-tree
Open

perf(rpc): stop storing the block header in every block record#90
rabbitson87 wants to merge 2 commits into
perf/rpc-block-record-headerfrom
perf/rpc-record-header-from-tree

Conversation

@rabbitson87

@rabbitson87 rabbitson87 commented Aug 21, 2026

Copy link
Copy Markdown
Member

Stacked on #86. Merge that first — this branch is based on it, and the diff
below is only the delta.

The node holds one BlockRecord per applied block for the life of the process
and nothing removes one, so the record's own footprint is the cost. The
BlockTree already holds a full bitcoin::block::Header on every node — the
record stored it a second time.

Revision per record at a mainnet tip
header as a hex String 264 B + 1 allocation
header as an inline [u8; 80] (#86) 168 B −88 MiB
header not stored at all 88 B −73.5 MiB

This is footprint, not time. size_of::<BlockRecord>() × record count,
pinned by a test — not an observed RSS drop, and it should not be quoted as one.
There is no benchmark and no harness in this workspace that attributes resident
memory to the block-record log.

Why this was a design question, not a refactor

#86 stopped short of removing the field, and the plan for it said why: two
fallbacks exist for "the tree does not know this hash"
Context::record_for_hash step 2 and the singleton in rest.rs — and their
comments describe it as a real state ("a block seen before a checkpoint
restore"). If it is real, dropping the field turns a working getblock /
getblockheader / REST answer into an empty one.

It is not reachable in a running node:

  • The header enters the tree before the record enters the log. apply_block
    inserts via applied_header_tip (apply.rs:2338-2343) and pushes the record
    afterwards (:2354), through the same Arc<RwLock<BlockTree>>.
  • The tree never drops a node. The only Slab operation is insert
    (chain/src/tree.rs:604); invalidate_subtree flips status and clears the
    height index, never the slab. The "pruned out of BlockTree" comment in
    undo_pruner.rs is about the redb column family, not this tree.
  • The log is not durable. state.rs:1061 builds it empty on every open, so a
    record cannot survive a restart into the state its own comment describes.
  • A restore rebuilds the tree from genesis, contiguously
    (checkpoint.rs:242-329 asserts node.height == height from index 0 up).
  • Context::add_block has no non-test callers, and every
    BlockRecord::from_block* outside applied_block_record is a test or bench.

Context::header_record was already the precedent — it builds a record whose
header comes from tree.node_by_hash(hash), an O(1) lookup.

What changed

Every constructor leaves header: None. header_record is the only producer, so
the tree is the single source of truth for what a block's header is.

record_for_hash step 1 resolves the tree node, then looks for a cached record
with the same hash and height. It returned that cached record verbatim, which
would now answer with no header at all — it splices the tree's header in instead.
No extra lock: header_record has already taken and released the tree guard,
and the header it produced outlives it.

Consumers are unchanged — all four already read through header_bytes() /
header_hex(), which #86 introduced.

The boxing is the saving, not a detail

Option<[u8; 80]> costs its full 80 bytes in every record even when None.
Emptying the log's records while leaving the array inline would have saved
nothing at all.

#86 considered and rejected Option<Box<[u8; 80]>> because it "lands at the same
168 bytes total while keeping the per-block allocation". That was correct while
every record carries a header. It stops being correct once none of them do, which
is exactly what this change makes true: the box is 8 bytes in the log and
allocates once per RPC answer instead of once per block.

Behaviour that changed

  • rest.rs's singleton fallback has no header to serve. Reaching it means
    the tree has no node for the hash, so there is none to be had; it yields an
    empty result. Code and comment left in place — removing an unreachable fallback
    is a separate claim from removing a stored field — and
    headers_for_a_record_the_tree_does_not_know_serve_nothing pins the outcome so
    it is recorded rather than silent.
  • Three getblock tests were built from a record alone. They now seed the
    tree too, through a seed_block helper that does what apply_block does. A
    record on its own was never a node's state; those fixtures were asking
    getblock to answer from half of it.

Mutation audit

Mutation Result
record_for_hash drops the header splice 4 tests failed
header_record does not read the header off the tree node 6 tests failed
applied_block_record stores the header again 1 test failed
from_block_bytes stores the header again 4 tests failed
the header is un-boxed 1 test failed

Baseline and restored green across all 14 targets.

Two of these are worth naming. applied_block_record storing the header again
is caught by applied_block_record_matches_rpc_constructors
, which predates
this change: it asserts the node's builder and the rpc constructors agree, and
now that both must agree on no header it guards the memory claim on the node
side without having been written for it.

Un-boxing is caught only by the size_of assertion, which is the point of
having one — it compiles, passes every behavioural test, and silently hands 80
bytes per block back to the heap.

Verification

  • cargo test -p bitcoin-rs-rpc -p bitcoin-rs-node --no-default-features --features bitcoin-rs-node/fjall --no-fail-fast — 14/14 targets green
  • cargo fmt --check — clean
  • cargo clippy ... -- -D warnings — clean

Not in this change

  • record_for_hash step 2 and the rest.rs fallback stay. The evidence says
    they are unreachable in production; acting on that is its own claim.
  • BlockTree's unbounded Slab. Nothing removes a node, and the tree is now
    the single source of every header — which raises the value of that candidate
    rather than lowering it.

Full write-up: docs/benchmarks/block-record-footprint.md.


Second commit: the ordering is enforced, not just argued (e9502b6)

Everything above rests on one ordering — the header is in the tree before the
record is in the log — and nothing checked it. In Bitcoin Core that state is not
representable at all: CBlockIndex holds the header and the payload facts in
one structure, so there is no such thing as a record without an index entry. Here
they are two structures held in step by code discipline.

The push site now asserts it:

debug_assert!(
    handles.block_tree.read().node_by_hash(block_hash).is_some(),
    "block {} is entering the record log with no block-tree node; \
     its header would be unrecoverable",
    block_hash.to_string_be()
);

The tree lock is free there — applied_header_tip releases its write guard
before returning — and the check is one hash-table lookup, compiled out of
release builds.

It is not a lone test. Moving the record push above the tree insert — exactly
the mistake it defends against — fails 52 node tests, each on this assertion
naming the block that would have lost its header. Every node test that applies a
block now exercises the invariant.

One part of the argument needed checking, not assuming

A reorg calls invalidate_subtree. Had lookup filtered on node status, an
invalidated block's header would have become unreachable while its record was
still in the log — a hole straight through the safety argument.

lookup (chain/src/tree.rs:146-154) matches on hash alone, and by_hash is
only ever insert_uniqued (:615), so a hash resolves for the life of the
process whatever happens to the branch it is on. Reorged-out blocks in fact
answer better than before: the record is popped on disconnect, but the tree
node stays, so getblockheader still serves a header.

On Bitcoin Core

For the record, since this change is about removing something Core does not
duplicate either. CBlockIndex stores the header fields as members and
reconstructs the header from them:

CBlockHeader GetBlockHeader() const {
    CBlockHeader block;
    block.nVersion = nVersion;
    if (pprev) block.hashPrevBlock = pprev->GetBlockHash();
    ...
}

getblockheader calls LookupBlockIndex(hash) and reads the fields off the
index — never from disk, which is why it works on pruned blocks. That is the
shape this change moves toward: the in-memory block index as the single
authority for headers.

Two honest gaps remain. Core keeps header and payload facts in one structure
where this codebase now has two — merging them is the Core-shaped end state and
would make the invariant above unrepresentable rather than asserted. And Core
does not store hashPrevBlock at all, deriving it from pprev; BlockTreeNode
stores both its own hash and header.prev_blockhash, which is 32 redundant
bytes per node. Both are separate candidates.

The node holds one `BlockRecord` per applied block for the life of the process
and nothing removes one, so the record's own footprint is the cost. The
`BlockTree` already holds a full header on every node, so the record stored it
twice.

Every constructor now leaves the field empty. `Context::header_record` is the
only thing that produces a header, from the tree node it resolved - the tree
becomes the single source of truth for what a block's header is.

  size_of::<BlockRecord>(): 168 -> 88
  80 bytes per block, about 73.5 MiB at a mainnet tip

on top of the 88 MiB the parent commit saved by storing raw bytes instead of hex.
This is footprint, not time: no benchmark, and it should not be quoted as an
observed RSS drop.

`record_for_hash` step 1 resolves the tree node and then looks for a cached
record with the same hash and height. It used to return that cached record
verbatim, which would now answer with no header at all; it splices the tree
header in instead. Costs no extra lock - `header_record` has already taken and
released the tree guard, and the header it produced outlives it.

The boxing is the saving, not a detail. `Option<[u8; 80]>` costs its full 80
bytes in every record even when `None`, so emptying the log's records while
leaving the array inline would have saved nothing. The parent commit rejected
`Option<Box<..>>` because it "lands at the same 168 bytes while keeping the
per-block allocation" - true while every record carries a header, and no longer
true once none of them do.

Removing the field was a design question rather than a refactor because two
fallbacks exist for "the tree does not know this hash". Neither is reachable in a
running node: `apply_block` inserts the header into the tree before pushing the
record through the same handles; the tree never drops a node (the only `Slab`
operation is `insert`, and `invalidate_subtree` only flips status); the log is
not durable, rebuilt empty on every open; and a checkpoint restore rebuilds the
tree from genesis contiguously. Both fallbacks are left in place - removing an
unreachable fallback is a separate claim from removing a stored field - but the
REST one now yields an empty result, which a new test pins rather than leaving
silent.

Three `getblock` tests built a context from a record alone. They seed the tree
too now, through a `seed_block` helper that does what `apply_block` does. A
record on its own was never a node's state.

Five mutations, all killed; baseline and restored green across 14 targets. See
docs/benchmarks/block-record-footprint.md.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 21, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 36dbbb2a-cb28-417c-b981-3d0932c7d560

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

The parent commit stopped storing the block header in `BlockRecord` on the
grounds that the block tree already has it by the time the record is in the log.
That is an ordering: `applied_header_tip` inserts, then the record is pushed,
through the same handles. Nothing checked it.

The push site now asserts it. Reverse the two statements and every `getblock` /
`getblockheader` answer for a freshly applied block loses its header, with
nothing failing at the point the mistake is made.

The tree lock is free there - `applied_header_tip` releases its write guard
before returning - and the check is one hash-table lookup, compiled out of
release builds.

It is not a lone test. Moving the record push above the tree insert, which is
exactly the mistake it defends against, fails 52 node tests, each on this
assertion naming the block that would have lost its header. Every node test that
applies a block now exercises the invariant.

Also records, in docs/benchmarks/block-record-footprint.md, the one part of the
safety argument that needed checking rather than assuming: a reorg calls
`invalidate_subtree`, and had `lookup` filtered on node status then an
invalidated block's header would have become unreachable while its record was
still in the log. `lookup` matches on hash alone and `by_hash` is insert-only, so
a hash resolves for the life of the process whatever happens to its branch.

This remains weaker than Bitcoin Core, where `CBlockIndex` holds the header and
the payload facts in one structure so "a record with no index entry" is not
representable. Here they are two structures held in step by an ordering. Merging
them is the Core-shaped end state and a separate change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.

1 participant