refactor(l1): dedup the canonical-block-hash read onto load_canonical_block_hash - #7189
refactor(l1): dedup the canonical-block-hash read onto load_canonical_block_hash#7189ilitteri wants to merge 1 commit into
Conversation
…_block_hash The exact CANONICAL_BLOCK_HASHES read (begin_read -> get -> H256::decode) was inlined three times: in get_canonical_block_hash (inside its spawn_blocking closure), in get_canonical_block_hash_sync, and in the private load_canonical_block_hash. Both public variants now keep their latest_block_header fast path and delegate the DB read to load_canonical_block_hash, mirroring the existing get_block_header_by_hash / load_block_header_by_hash split. The async variant clones the Store into the spawn_blocking closure (Store is Clone over Arcs) and keeps the same JoinError -> StoreError::Custom mapping. No behavior change.
|
Lines of code reportTotal lines added: Detailed view |
🤖 Kimi Code ReviewFile: Review Summary: This is a good refactoring that eliminates duplicate database access logic. No correctness issues found, but one minor performance consideration regarding struct cloning. Detailed Feedback:
Verdict: Approved with the suggestion to verify that Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Claude Code ReviewConfirmed: line 714's occurrence in ReviewThis is a small, well-scoped refactor with no behavioral change. Verified against the current file:
No correctness, security, or performance issues found. The change reduces duplication and improves maintainability without introducing risk. Automated review by Claude (Anthropic) · sonnet · custom prompt |
🤖 Codex Code Review
No correctness, consensus, gas-accounting, or security regressions stood out in this diff otherwise. The helper extraction itself is behavior-preserving and makes the sync/async paths consistent. Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
1 participant
Motivation
The exact
CANONICAL_BLOCK_HASHESread —begin_read()? → get(CANONICAL_BLOCK_HASHES, block_number.to_le_bytes())? → H256::decode → transpose → map_err(StoreError::from)— was written out three times incrates/storage/store.rs: insideget_canonical_block_hash'sspawn_blockingclosure, inget_canonical_block_hash_sync, and in the privateload_canonical_block_hash.Triplicated read logic is a liability: a future change to the key encoding, the value decoding, or the error mapping has to be applied in three places, and the sync variant's post-fast-path body being byte-identical to
load_canonical_block_hashinvites the copies to drift apart silently.Description
Both public variants now keep their
latest_block_headerfast path and delegate the actual DB read to the existing privateload_canonical_block_hash, mirroring theget_block_header_by_hash→load_block_header_by_hashsplit already used elsewhere in this file. Net −17 lines, one concern, no behavior change.get_canonical_block_hash(async): thespawn_blockingclosure now captures aStoreclone and callsstore.load_canonical_block_hash(block_number).StoreisClone(Arcs plus plain data), and the clone happens once per call, not in any loop. The JoinError mapping.map_err(|e| StoreError::Custom(format!("Task panicked: {}", e)))?is unchanged.get_canonical_block_hash_sync: after the fast path, the body is nowself.load_canonical_block_hash(block_number).load_canonical_block_hashitself is untouched: no fast path was added to it, because theload_*family deliberately bypasses thelatest_block_headercache to serve init/genesis paths. The fast paths stay in the two public functions, so hot callers (is_canonical_sync, the BLOCKHASH-opcode path inblockchain/vm.rs) keep their cache hit.get_block_bodies, is intentionally left alone: it amortizes a single read transaction across a loop, which delegating per-iteration would undo.If this change were wrong, one of these would have to be true:
load_canonical_block_hashreads a different table, key encoding, or decoding than the inlined copies did — it is byte-identical to the removed code.Result<Option<BlockHash>, StoreError>throughspawn_blocking, and the JoinError arm is character-for-character the same.Storecould not be moved intospawn_blocking— it isCloneand all fields areSend + Sync; the crate compiles underclippy --all-targets -D warnings.How to test
cargo fmt -p ethrex-storage— no diff after the change.cargo clippy -p ethrex-storage --all-targets -- -D warnings— clean.cargo test -p ethrex-storage— 91 passed, 0 failed; doctests 1 passed, 1 ignored.Public signatures are unchanged, so no dependent crate is affected.
Checklist
STORE_SCHEMA_VERSION(crates/storage/lib.rs) if the PR includes breaking changes to theStorerequiring a re-sync. — NoStoreschema change (pure read-path dedup), soSTORE_SCHEMA_VERSIONis untouched.