Codecov Report

❌ Patch coverage is 91.30435% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.24%. Comparing base (7dbcfd9) to head (7bd87fa).
⚠️ Report is 1 commits behind head on main.

@@            Coverage Diff             @@
##             main    #3789      +/-   ##
==========================================
- Coverage   75.34%   75.24%   -0.10%     
==========================================
  Files         438      438              
  Lines       39528    39569      +41     
==========================================
- Hits        29781    29773       -8     
- Misses       7676     7707      +31     
- Partials     2071     2089      +18     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

Claude finished @ongyimeng's task in 2m 32s —— View job

PR Review: perf(state): avoid full header decode in deprecated state backend

• Gather context (diff, changed files)
• Review core/accessors.go partial accessor
• Review pruner/retention.go retention guard
• Review blockchain/statebackend/deprecated.go changes
• Verify retention/pruning semantics
• Review tests
• Post findings

I traced the retention semantics end-to-end to confirm the behavior change is safe. The optimization is correct — no blockers.

Correctness verification

The key question is whether dropping the full header decode weakens the state-retention guard. It doesn't:

• The pruner signals state pruned by deleting the hash→number index (DeleteBlockHeaderNumberByHash in pruner/accessors.go:183,203), while keeping the header-by-number (needed for block-level data). So the index lookup is the retention signal.
• StateAtBlockNumber → RequireStateRetainedByBlockNumber: does GetBlockHeaderHashByNumber (header exists) + GetBlockHeaderNumberByHash(hash) (the retention signal). Same two lookups as the old HeaderByNumberIfStateRetained, just a partial decode. ✅
• StateAtBlockHash → BlockNumberByHashIfStateRetained: the old path resolved the index then decoded the header only to read header.Number; on a consistent DB index[hash] == header.Number, so returning the index value directly is equivalent. The dropped header decode was an incidental corruption guard, correctly called out in the PR description. ✅
• HeadState: dropping the discarded GetBlockHeaderByNumber(height) is pure win — GetChainHeight remains the head-exists guard. ✅
• Partial CBOR decode into struct{ Hash *felt.Felt } is sound: Header uses canonical named-field (map) CBOR, so unknown fields are ignored — same established pattern as GetGlobalStateRootByBlockNumber. The added nil-check makes a corrupt/missing Hash stricter than before, not weaker. ✅
• Old HeaderByNumberIfStateRetained / HeaderByHashIfStateRetained are still used by the non-deprecated stateBackend (which genuinely needs GlobalStateRoot/Number), so they're not dead code and the scoping is right.

Findings

Blocker: none.

Important: none.

Nit (posted inline):

• core/accessors_test.go:299 — the table-driven refactor dropped the old "missing global state root" subtest, so the header.GlobalStateRoot == nil / header.Hash == nil guards are now uncovered (matches Codecov's missing-lines report). Worth re-adding a malformed-header case.
• pruner/retention.go:75 — doc comments on RequireStateRetainedByBlockNumber and BlockNumberByHashIfStateRetained don't start with the function name (unlike the neighbouring helpers); minor godoc/revive consistency.

Optional follow-up (out of scope): the non-deprecated stateBackend.StateAtBlockNumber (statebackend.go:37) could adopt the same GetGlobalStateRootByBlockNumber partial decode for the same win.
· branch perf/deprecated-state-partial-header

Claude finished @ongyimeng's task in 56s —— View job

PR Re-review: perf(state): avoid full header decode in deprecated state backend

The two new commits address both nits from my previous review — re-checked the current tree.

• Verify doc-comment fix (pruner/retention.go)
• Verify missing-field test coverage (core/accessors_test.go)
• Re-confirm correctness of the optimization

Follow-ups from prior review — both resolved ✅

• Doc convention (7bd87fa): RequireStateRetainedByBlockNumber and BlockNumberByHashIfStateRetained now start with the function name, matching the neighbouring helpers and revile/godoc convention. ✅
• Test coverage (a5da185): TestPartialBlockHeaderAccessorsByNumber is now table-driven over both partial accessors and re-adds a "missing field returns error" case that writes a header omitting the target field, restoring coverage of the header.Hash == nil / header.GlobalStateRoot == nil guards that the earlier refactor had dropped. ✅

Correctness (unchanged from prior review)

The optimization remains sound. Retention semantics are preserved:

• RequireStateRetainedByBlockNumber does the same two lookups as the old HeaderByNumberIfStateRetained (header-hash exists + hash→number index = the retention signal), just with a partial CBOR decode.
• BlockNumberByHashIfStateRetained returns the index value directly; on a consistent DB index[hash] == header.Number, so it's equivalent to the old header-decode path.
• HeadState dropping the discarded GetBlockHeaderByNumber is pure win; GetChainHeight remains the head-exists guard.
• Partial named-field CBOR decode is the established pattern (GetGlobalStateRootByBlockNumber); the added nil-check makes a corrupt Hash stricter, not weaker.

Blocker: none · Important: none · Nit: none outstanding.

LGTM — ready to merge once CI is green. (I couldn't run go test in this environment due to sandbox permissions, but CI covers it.)

Optional out-of-scope follow-up still stands: the non-deprecated stateBackend.StateAtBlockNumber could adopt the same partial-decode win.
· branch perf/deprecated-state-partial-header