Codecov Report

❌ Patch coverage is 86.36364% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 50.61%. Comparing base (d7b2be8) to head (eca8045).
⚠️ Report is 5 commits behind head on develop.

❌ Your patch status has failed because the patch coverage (86.36%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage.

@@             Coverage Diff             @@
##           develop    #2080      +/-   ##
===========================================
- Coverage    50.62%   50.61%   -0.01%     
===========================================
  Files          875      875              
  Lines       151794   151799       +5     
===========================================
- Hits         76842    76835       -7     
- Misses       69879    69895      +16     
+ Partials      5073     5069       -4     

@claude Can you review this replacement check from the root hash to the end hash on the function and analyze the impacts? Do a deep analysis based on your deep knowledge on reorgs and how they should be handled. Get the context of the affected function and analyze the impact of this change. By the end evaluate if the tests made for these changes are compatible with the changes or if we should cover any other scenario

Claude finished @lucca30's task in 1m 24s —— View job

Deep Analysis: Root Hash Calculation During Reorgs

• Read and understand the changed files
• Analyze the core fix and its implications for reorg handling
• Examine how this relates to blockchain reorg fundamentals
• Evaluate test coverage and identify any gaps

Summary

This PR fixes a subtle but important bug in the GetRootHash function related to cache invalidation during blockchain reorganizations. The fix changes from tracking the chain tip hash to tracking the end block hash of the checkpoint range, which is a more precise and correct approach.

Core Change Analysis

Before (Bug)

type rootHashCacheEntry struct {
    headHash common.Hash  // Tracked chain tip
    root     string
}

// Cache invalidation checked:
if entry.headHash == headHash { /* cache hit */ }

// Reorg detection checked:
newHead := api.chain.CurrentHeader()
if newHead.Hash() != headHash { /* reorg detected */ }

After (Fix)

type rootHashCacheEntry struct {
    endHash common.Hash  // Tracks end block of range
    root    string
}

// Cache invalidation checks:
if entry.endHash == endHash { /* cache hit */ }

// Reorg detection checks:
newEndHeader := api.chain.GetHeaderByNumber(end)
if newEndHeader.Hash() != endHash { /* reorg detected */ }

Why This Fix is Correct

The Problem with Tracking Head Hash

Consider a checkpoint request for blocks [100, 200] when the chain is at block 500:

Scenario: Normal block production (no reorg)

1. Call GetRootHash(100, 200) → computes root, caches with headHash=hash(500)
2. Chain advances to block 501
3. Call GetRootHash(100, 200) → cache miss because hash(500) != hash(501)
4. Unnecessarily recomputes the same root hash

This caused spurious cache invalidations whenever the chain tip advanced, even though the blocks [100, 200] hadn't changed.

The Solution: Track End Block Hash

By tracking endHash (hash of block 200 in the range [100, 200]):

1. Call GetRootHash(100, 200) → computes root, caches with endHash=hash(200)
2. Chain advances to block 501
3. Call GetRootHash(100, 200) → cache hit because hash(200) is unchanged
4. No recomputation needed

Reorg Detection Remains Sound

The fix also correctly updates the reorg detection mechanism:

Defense in Depth: Multiple Reorg Safety Mechanisms

The function employs three layers of protection against mixed-fork data:

1. End-block hash binding (lines 302-318): Cache entries are invalidated if the canonical block at end changes
2. Post-computation verification (lines 351-353): After fetching all headers concurrently, re-checks that the end block hasn't changed mid-flight
3. Continuity validation (lines 367-372): Verifies H[n].ParentHash == H[n-1].Hash() for all headers, catching any interleaved forks

This multi-layered approach is sound from a reorg-safety perspective.

Test Coverage Evaluation

Tests Added/Modified

The New Test is Key

The new test TestAPI_GetRootHash_StableWhenTipAdvancesPastEnd specifically validates the fix:

• Computes root for range [0, 3]
• Advances tip to block 5
• Verifies the same root is returned (cache hit)
• Confirms only 1 additional read (the end header check), not a full recomputation

This directly tests the scenario that was broken before.

Potential Gap: Non-contiguous Header Error Test

The errNonContiguousHeaderRange path (line 371) is not explicitly tested in the reorg tests. While the continuity check is a safety mechanism that would catch mixed-fork headers, adding a test that triggers this specific error path would improve coverage.

Suggested additional test scenario:

• Configure a mid-flight reorg that switches chains after fetching some headers but before the end-hash check
• Verify that either errReorgDuringRootComputation or errNonContiguousHeaderRange is returned

However, this is a minor gap since:

1. The existing TestGetRootHash_ReorgDuringCall_ReturnsError covers the end-hash detection
2. The continuity check is a secondary safety net

Conclusion

This fix is correct and improves both performance and precision:

The change from headHash to endHash is semantically correct: the root hash for range [start, end] depends only on the blocks in that range, not on blocks that come after. Binding the cache to the end block hash precisely captures this invariant.

govuln check is pointing some errors, also kurtosis e2e is not properly running. Would be good having both working before merging.

govuln check is pointing some errors, also kurtosis e2e is not properly running. Would be good having both working before merging.

@lucca30
govuln is fixed here #2081
And kurtosis is failing due to pumba which was fixed in my PRs and in @kamuikatsurgi's too

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud