perf(rpc): store the block record header as raw bytes, not hex - #86
perf(rpc): store the block record header as raw bytes, not hex#86rabbitson87 wants to merge 3 commits into
Conversation
A BlockRecord is pushed for every applied block and nothing removes one, so the record's own footprint is paid for the life of the process. header_hex held the 80-byte header rendered as a 160-byte hex String: 104 bytes inline plus a heap allocation, 264 bytes per block. Storing the raw header instead makes that 168 bytes inline and no allocation -- 96 bytes per block, about 88 MiB at a mainnet-sized chain, and roughly a million fewer heap allocations over a full sync. Two of the three readers get simpler rather than slower. Both decode_header implementations were parsing the hex straight back into bytes; the rpc one loses its hex-decode step and the whole error branch that went with it, the rest one goes from two lines to one, and FromHex is no longer imported by either. Only getblockheader still wants hex, and it now encodes on demand -- one RPC call against a cost paid per block forever. The saving is measured, not asserted: a test pins size_of::<BlockRecord>() so the claim cannot drift, and the accompanying comment records why the survey's figure of 160 bytes per block was wrong. It counted the heap String as removed without counting the 64 bytes the inline array adds to the struct. The real figure is 96, and Option<Box<[u8; 80]>> was considered and rejected: it lands at the same 168 bytes total while keeping the per-block allocation. Behaviour is unchanged. header_hex() renders byte-identically to what the field held, a record without a header renders the empty string exactly as the empty String did, and no error code or message moved. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 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. Comment |
A mutation audit of the header change found that dropping the header from `Context::header_record` entirely failed no test. That path builds a BlockRecord from the block tree rather than from applied-block bytes, and this change gave it a `serialize(&node.header).try_into().ok()` whose failure mode is a silently absent header -- where the old String field would at least have carried something. It was the one part of the change nothing exercised. `tree_derived_record_carries_the_header` seeds a tree node, resolves the record through `record_for_hash` without pushing anything into `blocks`, and asserts both the raw bytes and the rendered hex match the header the tree holds. The rest of the change is well covered, and the audit says so rather than assuming it: neutering `header_from_block_bytes` fails five tests, three of them pre-existing `getblock` tests, and emptying `header_hex()` fails the round-trip test. The new test now fails under the mutation that found the gap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Mutation audit found one gapPushed as
Full audit
The first row is the useful one for reviewers: that path was already well covered by tests this PR did not write, which is why the type change was safe to make mechanically. Two earlier mutation attempts are not in that table because they were invalid, not because they passed — one deleted lines and broke the parse, another tripped a compiler ICE. Neither proved anything and both were reformulated. Also checked
|
1 participant
Candidate B (first half) from the optimization-candidates survey (#84), and a correction to the number that survey published.
The cost
Context::add_blockpushes aBlockRecordfor every applied block and nothing removes one. The record's own footprint is therefore paid for the life of the process.header_hexheld the 80-byte header rendered as a 160-byte hexString.Measured, not derived:
96 bytes per block, about 88 MiB at a mainnet-sized chain, plus roughly a million fewer heap allocations over a full sync.
Correcting the survey
The survey put this at 160 B/block (241 MiB at tip). That figure counted the heap
Stringas removed without counting the 64 bytes the inline[u8; 80]adds to the struct. The real saving is 96 B/block and 88 MiB — worth having, but 1.7x smaller than advertised. A test pinssize_of::<BlockRecord>()so the claim cannot drift back.Option<Box<[u8; 80]>>was considered and rejected: it lands at the same 168 bytes total while keeping the per-block allocation.Two of three readers get simpler
header_hexhad three readers. Two were parsing the hex straight back into bytes:crates/rpc/src/handlers/chain.rsdecode_headerloses its hex-decode step and the entire error branch that went with itcrates/rpc/src/rest.rsdecode_headergoes from two lines to oneFromHexis no longer imported by either fileOnly
getblockheaderstill wants hex, and it now encodes on demand — one RPC call, against a cost paid per block forever.Behaviour
Unchanged.
header_hex()renders byte-identically to what the field held, a record without a header renders the empty string exactly as the emptyStringdid, and no error code or message moved.Verification
bitcoin-rs-rpclib tests: 177 passed, unfiltered —header_hexwas a public field, so the full suite was run rather than a filtered subsetbitcoin-rs-nodelib tests: 442 passedbitcoin-rs-electrumbuildscargo clippy -D warningson both changed crates: cleancargo fmt --check: cleanThree new tests pin the footprint, the hex output, and the empty-header rendering.
Scope
This is the half with no behaviour change. The other half — bounding the log, or dropping records below the prune height the way
block_hexalready is — is a behaviour change, becausecrates/rpc/src/handlers/chain.rs:45and:171scan the log. It is not in this PR.For context on where this sits: a drained memory attribution taken during a mainnet sync at height 412,169 puts the whole non-UTXO residual at 2.24 GiB, of which this log is a small part. The dominant term is index memory, which no candidate covers yet.
🤖 Generated with Claude Code