Skip to content

refactor(domain): turn header into an immutable value type#364

Open
fpelliccioni wants to merge 1 commit into
masterfrom
refactor/domain-header-value-type
Open

refactor(domain): turn header into an immutable value type#364
fpelliccioni wants to merge 1 commit into
masterfrom
refactor/domain-header-value-type

Conversation

@fpelliccioni

@fpelliccioni fpelliccioni commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Summary

Reshape chain::header into an immutable value type, then propagate the change through every layer that consumed the old surface.

Domain change

  • Drop the header_basis base + hash_memoizer mixin + mutable header.validation struct (a side-channel that callers used to stamp height / median_time_past from outside).
  • Replace them with one immutable value type, selected at compile time via KTH_USE_HEADER_MEMBERS:
    • header_raw (default): 80-byte contiguous storage with fields decoded on access. Wins on the IBD hot path — faster construction-from-bytes (~1.5 ns) and ~17% faster wire deserialization than the members variant per the bench shipped here.
    • header_members: traditional field-per-member. Wins only on the "read every field" path.
  • The cached header.hash() member is gone. There is now a free function chain::hash(header) that both impls cooperate with.
  • previous_block_hash() and merkle() return hash_digest by value on the raw impl (decoded from the byte buffer), uniform with the members impl.

Propagation through the rest of the codebase

Every layer that touched the old header had to move with it.

blockchain: block_pool and branch no longer read height / MTP from header.validation; they now read them from the block's chain_state, which the validator populates. block_organizer no longer stamps the header. block_entry::parent() had to change from returning hash_digest const& to returning by value, because the new accessor produces a temporary — every caller that held a reference (auto const& in header_organizer.cpp, in internal_database test) had to drop the reference. validate_header::check() and ::accept() precompute the hash and pass it down. Tests block_entry / block_pool / branch are rewritten: 6-arg header ctor in place of set_* mutators, chain_state set up where height is actually needed.

database: header_index no longer pulls proof() from header_basis::; it lives on header itself now. Multiple header.hash() call sites switch to chain::hash(header).

network: protocols_coro call sites for block.header().hash() switch to chain::hash(...).

node: full_node, header_list, reservation, and the test utility follow the same pattern. header_list::check() precomputes the hash for the new header.check(hash, retarget) signature.

C-API: regenerated end-to-end by the generator in ../kth-tools; no hand edits to src/c-api/. The generator's free-function sweep is extended to kth::domain::chain so chain::hash(header const&) is picked up automatically as kth_chain_header_hash. The setters and hash_pow are dropped — header is immutable and BCH/BTC have no separate PoW hash. The c-api test file is updated to construct headers via the ctor and to feed kth_chain_header_hash into is_valid_proof_of_work.

domain itself: block_basis, chain_state, message::header(s), property_tree, and the relevant tests are updated to use chain::hash(...) and the 6-arg ctor.

Notes

Old files (header.cpp, header_basis.{hpp,cpp}, hash_memoizer.hpp, the original message/header.{hpp,cpp}) are renamed to .disabled so any straggler trips the compiler instead of silently linking against the legacy code path. Removed in a follow-up commit once the dust settles.

Test plan

  • domain (3869 cases / 1011125 assertions)
  • infrastructure (474 cases)
  • blockchain (107 cases / 522 assertions)
  • blockchain_vmlimits (21 cases / 4732 assertions)
  • blockchain_tsan
  • capi (774 cases / 2779 assertions)
  • network (97 cases)
  • node (45 cases)

@coderabbitai

coderabbitai Bot commented Jun 18, 2026

Copy link
Copy Markdown

Important

Review skipped

Review was skipped due to path filters

⛔ Files ignored due to path filters (49)
  • src/blockchain/include/kth/blockchain/pools/block_entry.hpp is excluded by !src/blockchain/** and included by none
  • src/blockchain/src/interface/block_chain.cpp is excluded by !src/blockchain/** and included by none
  • src/blockchain/src/pools/block_entry.cpp is excluded by !src/blockchain/** and included by none
  • src/blockchain/src/pools/block_organizer.cpp is excluded by !src/blockchain/** and included by none
  • src/blockchain/src/pools/block_pool.cpp is excluded by !src/blockchain/** and included by none
  • src/blockchain/src/pools/branch.cpp is excluded by !src/blockchain/** and included by none
  • src/blockchain/src/pools/header_organizer.cpp is excluded by !src/blockchain/** and included by none
  • src/blockchain/src/validate/validate_header.cpp is excluded by !src/blockchain/** and included by none
  • src/blockchain/test/block_entry.cpp is excluded by !src/blockchain/**, !**/test/** and included by none
  • src/blockchain/test/block_index_bench/benchmarks.cpp is excluded by !src/blockchain/**, !**/test/** and included by none
  • src/blockchain/test/block_pool.cpp is excluded by !src/blockchain/**, !**/test/** and included by none
  • src/blockchain/test/branch.cpp is excluded by !src/blockchain/**, !**/test/** and included by none
  • src/blockchain/test/header_index.cpp is excluded by !src/blockchain/**, !**/test/** and included by none
  • src/blockchain/test/header_organizer.cpp is excluded by !src/blockchain/**, !**/test/** and included by none
  • src/c-api/include/kth/capi/chain/header.h is excluded by !src/c-api/** and included by none
  • src/c-api/src/chain/header.cpp is excluded by !src/c-api/** and included by none
  • src/c-api/test/chain/header.cpp is excluded by !src/c-api/**, !**/test/** and included by none
  • src/database/include/kth/database/databases/header_database.ipp is excluded by !src/database/**, !**/*.ipp and included by none
  • src/database/src/data_base.cpp is excluded by !src/database/** and included by none
  • src/database/src/header_index.cpp is excluded by !src/database/** and included by none
  • src/database/test/internal_database.cpp is excluded by !src/database/**, !**/test/** and included by none
  • src/domain/CMakeLists.txt is excluded by !src/domain/**, !**/CMakeLists.txt and included by none
  • src/domain/include/kth/domain/chain/hash_memoizer.hpp is excluded by !src/domain/** and included by none
  • src/domain/include/kth/domain/chain/header.hpp is excluded by !src/domain/** and included by none
  • src/domain/include/kth/domain/chain/header_basis.hpp is excluded by !src/domain/** and included by none
  • src/domain/include/kth/domain/chain/header_members.hpp is excluded by !src/domain/** and included by none
  • src/domain/include/kth/domain/chain/header_raw.hpp is excluded by !src/domain/** and included by none
  • src/domain/include/kth/domain/message/header.hpp is excluded by !src/domain/** and included by none
  • src/domain/src/chain/block_basis.cpp is excluded by !src/domain/** and included by none
  • src/domain/src/chain/chain_state.cpp is excluded by !src/domain/** and included by none
  • src/domain/src/chain/header.cpp is excluded by !src/domain/** and included by none
  • src/domain/src/chain/header_basis.cpp is excluded by !src/domain/** and included by none
  • src/domain/src/chain/header_members.cpp is excluded by !src/domain/** and included by none
  • src/domain/src/chain/header_raw.cpp is excluded by !src/domain/** and included by none
  • src/domain/src/message/header.cpp is excluded by !src/domain/** and included by none
  • src/domain/src/message/headers.cpp is excluded by !src/domain/** and included by none
  • src/domain/src/utility/property_tree.cpp is excluded by !src/domain/** and included by none
  • src/domain/test/chain/block.cpp is excluded by !src/domain/**, !**/test/** and included by none
  • src/domain/test/chain/compare_header_benchmarks.py is excluded by !src/domain/**, !**/test/** and included by none
  • src/domain/test/chain/header.cpp is excluded by !src/domain/**, !**/test/** and included by none
  • src/domain/test/chain/header_benchmark_comparison.md is excluded by !src/domain/**, !**/test/** and included by none
  • src/domain/test/chain/header_benchmarks.cpp is excluded by !src/domain/**, !**/test/** and included by none
  • src/domain/test/chain/light_block.cpp is excluded by !src/domain/**, !**/test/** and included by none
  • src/domain/test/message/headers.cpp is excluded by !src/domain/**, !**/test/** and included by none
  • src/network/src/protocols_coro.cpp is excluded by !src/network/** and included by none
  • src/node/src/full_node.cpp is excluded by !src/node/** and included by none
  • src/node/src/utility/header_list.cpp is excluded by !src/node/** and included by none
  • src/node/src/utility/reservation.cpp is excluded by !src/node/** and included by none
  • src/node/test/utility.cpp is excluded by !src/node/**, !**/test/** and included by none

CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including **/dist/** will override the default block on the dist directory, by removing the pattern from both the lists.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 3b76fb3e-977e-4dc2-be53-8865ccb2b1a2

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
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/domain-header-value-type

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 and usage tips.

The old header inherited from header_basis, mixed in hash_memoizer to
cache the block hash, and carried a mutable `validation` struct holding
height and median_time_past stamped from outside. The cache was a thread
hazard, the inheritance leaked basis internals, and the validation
struct was a side-channel for height/MTP that the value type should
never own.

Replace it with two interchangeable value types selected at compile
time via `KTH_USE_HEADER_MEMBERS`:

- header_raw (default): 80-byte contiguous storage, fields decoded on
  access. Wins on the IBD hot path — faster deserialization, zero-copy
  raw access for wire-handling code.
- header_members: traditional struct, field-per-member. Wins on
  repeated full-record access (wallets, explorers).

Both expose the same surface plus a free function `chain::hash(header)`
in lieu of the cached `header.hash()` member.

The validation side-channel is gone: block_pool and branch now read
height and MTP off the block's chain_state, which the validator
already populates. block_organizer no longer stamps the header.

C-API regenerated via kth-tools (no hand edits). The generator picks
up `chain::hash(header const&)` automatically thanks to a new
`extra_namespaces=['kth::domain::chain']` on the header config, so
`kth_chain_header_hash` is exposed as a normal accessor.

Caught while wiring up the tests, fixed in passing:
- block_entry::parent() returned `hash_digest const&` but the new
  header decodes previous_block_hash from raw bytes into a temporary
  — every caller was holding a dangling reference.
- Same dangle in header_organizer.cpp and internal_database test.
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