Skip to content

feat: blockstm remove incarnation cache#26048

Closed
Eric-Warehime wants to merge 3 commits intomainfrom
eric/remove-incarnation-cache
Closed

feat: blockstm remove incarnation cache#26048
Eric-Warehime wants to merge 3 commits intomainfrom
eric/remove-incarnation-cache

Conversation

@Eric-Warehime
Copy link
Contributor

Description

Removes incarnation cache from blockstm.

Reasoning being that we've just removed the usage of incarnation cache for caching signature verification checks. This was removed in #25912 due to caching causing indeterminism bugs.

After this there are no current usages of the cache, and in general the cache promotes patterns which are likely to cause indeterminism and app hashes.

@Eric-Warehime Eric-Warehime changed the title feat: block remove incarnation cache feat: blockstm remove incarnation cache Mar 6, 2026
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 6, 2026

Greptile Summary

This PR removes the incarnation cache from BlockSTM, eliminating a shared mutable cache that was passed between transaction incarnations during parallel block execution. The cache was previously used to deduplicate signature verification, but its last consumer was removed in #25912 due to indeterminism bugs, leaving the mechanism with no active users.

Key changes:

  • incarnationCache map[string]any parameter removed from DeliverTxFunc, RunTx, and deliverTx across baseapp and internal/blockstm
  • incarnationCache field and all associated methods (GetIncarnationCache, SetIncarnationCache, WithIncarnationCache, IncarnationCache) removed from sdk.Context
  • Cache type alias and cache-aware Tx function signature removed from the BlockSTM mock/test infrastructure
  • TestSTMRunner_Run_IncarnationCache test deleted as it is no longer meaningful

Minor issues found:

  • The first line of the StartSpan GoDoc comment in types/context.go was accidentally deleted during the removal
  • The unused SigVerificationResultCacheKey constant in internal/blockstm/mock_block.go should be cleaned up

Confidence Score: 4/5

  • Safe to merge — the removal is consistent across all call sites and the deleted functionality had no remaining consumers.
  • The change is a clean, mechanical removal of a parameter and associated struct field across 13 files. All call sites are updated consistently, no logic was silently dropped beyond the explicitly-intended cache. Two minor style/housekeeping issues remain: an accidentally deleted StartSpan doc comment line and an orphaned constant. These don't affect functionality but should be addressed for code quality.
  • types/context.go (truncated GoDoc) and internal/blockstm/mock_block.go (orphaned constant).

Sequence Diagram

sequenceDiagram
    participant STMRunner
    participant deliverTx as DeliverTxFunc
    participant RunTx as BaseApp.RunTx
    participant Context as sdk.Context

    Note over STMRunner: Before this PR
    STMRunner->>STMRunner: init incarnationCache[]
    STMRunner->>deliverTx: deliverTx(tx, memTx, ms, idx, cache)
    deliverTx->>RunTx: RunTx(mode, tx, memTx, idx, ms, cache)
    RunTx->>Context: ctx.WithIncarnationCache(cache)
    Context-->>RunTx: ctx with cache
    RunTx-->>deliverTx: result

    Note over STMRunner: After this PR
    STMRunner->>deliverTx: deliverTx(tx, memTx, ms, idx)
    deliverTx->>RunTx: RunTx(mode, tx, memTx, idx, ms)
    RunTx-->>deliverTx: result
Loading

Comments Outside Diff (1)

  1. internal/blockstm/mock_block.go, line 14 (link)

    SigVerificationResultCacheKey is no longer referenced anywhere in the codebase following the removal of incarnation cache logic. This constant should be deleted.

Last reviewed commit: d6b9594

@codecov
Copy link

codecov bot commented Mar 6, 2026

Codecov Report

❌ Patch coverage is 85.71429% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.78%. Comparing base (ead3b81) to head (df0caab).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
baseapp/abci.go 50.00% 1 Missing ⚠️
baseapp/test_helpers.go 75.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #26048      +/-   ##
==========================================
- Coverage   59.88%   59.78%   -0.11%     
==========================================
  Files         982      966      -16     
  Lines       65193    64267     -926     
==========================================
- Hits        39038    38419     -619     
+ Misses      26155    25848     -307     
Files with missing lines Coverage Δ
baseapp/baseapp.go 85.06% <100.00%> (-0.06%) ⬇️
baseapp/genesis.go 80.00% <100.00%> (ø)
baseapp/txnrunner/default.go 100.00% <100.00%> (ø)
internal/blockstm/txnrunner.go 79.06% <100.00%> (-2.19%) ⬇️
types/abci.go 100.00% <ø> (ø)
types/context.go 91.70% <ø> (+4.33%) ⬆️
baseapp/abci.go 85.94% <50.00%> (ø)
baseapp/test_helpers.go 66.66% <75.00%> (ø)

... and 21 files with indirect coverage changes

Impacted file tree graph

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mmsqe
Copy link
Collaborator

mmsqe commented Mar 7, 2026

we met nondeterministic isssue in ibc test before, but thinking it's ok to keep the interface to support eth signature-result cache later.

@yihuang
Copy link
Collaborator

yihuang commented Mar 7, 2026

EVM tx signature verification is one use case, if we don't do it in with incarnation cache, we might want to have a special pipeline for signature verification, because in block-stm, it'll make it heavier to re-execute aborted transactions.

@Eric-Warehime
Copy link
Contributor Author

We'll keep this here for now--caching signature validation should be stateless (in the sense of not requiring store access) and so should be safe for caching.

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.

3 participants