Skip to content

fix(mempool)!: Remove VM keeper end blocker notification, use PrepareCheckStater instead#1146

Merged
mattac21 merged 5 commits into
mainfrom
ma/remove-mempool-endblock-notification
Apr 29, 2026
Merged

fix(mempool)!: Remove VM keeper end blocker notification, use PrepareCheckStater instead#1146
mattac21 merged 5 commits into
mainfrom
ma/remove-mempool-endblock-notification

Conversation

@mattac21
Copy link
Copy Markdown
Contributor

@mattac21 mattac21 commented Apr 29, 2026

Description

Removes the ability to notify the mempool of new blocks within the vm keepers end blocker. This is strictly incorrect now since it will trigger a reset of the mempool on stale state. This behavior is replicated by using the PrepareCheckStater hook that is called just after a block is committed and the check state is updated.

Closes: STACK-2688


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • tackled an existing issue or discussed with a team member
  • left instructions on how to review the changes
  • targeted the main branch

@mattac21 mattac21 requested a review from a team as a code owner April 29, 2026 20:06
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 29, 2026

Greptile Summary

This PR removes the incorrect EndBlocker-based mempool notification (which ran on stale pre-commit state) and replaces it with a PrepareCheckStater hook that fires after a block is committed and check state is refreshed. The evmMempool field and SetEvmMempool/NotifiedMempool interface surface are fully cleaned up. All three issues flagged in previous review rounds — the P0 compile error, the stale doc comment, and the dead struct field — are resolved in this changeset.

Confidence Score: 5/5

Safe to merge — all previously flagged issues are resolved and no new P0/P1 issues found

The three issues from previous review rounds (P0 compile error, stale doc comment, dead struct field) are all cleanly fixed. The new PrepareCheckStater hook fires at the correct lifecycle point, the NotifyNewBlock() refactor preserves both the blockchain notification and cosmos pool recheck in both paths, and all interface/mock/import cleanup is consistent.

No files require special attention

Important Files Changed

Filename Overview
evmd/mempool.go Adds SetPrepareCheckStater hook that calls mempool.NotifyNewBlock() only when no event bus is configured; correctly replaces EndBlocker notification
mempool/mempool.go Extracts NotifyNewBlock() as a public method (blockchain notify + cosmos recheck trigger); removes vmKeeper.SetEvmMempool call; event-bus goroutine now delegates to the new method
mempool/interface.go Removes NotifiedMempool interface and SetEvmMempool from VMKeeperI; GetBlockchain() still exported on *Mempool but no longer part of any interface
x/vm/keeper/abci.go Removes the stale-state mempool notification from EndBlock; EndBlock now only calls CollectTxBloom
x/vm/keeper/keeper.go Removes evmMempool field and SetEvmMempool method from the Keeper struct; import of evmmempool package also cleaned up
mempool/mocks/VMKeeperI.go Mock updated to remove SetEvmMempool entry and the now-unused mempool import
CHANGELOG.md Adds API-BREAKING entry for the EndBlocker to PrepareCheckStater migration under a new section

Sequence Diagram

sequenceDiagram
    participant CometBFT
    participant BaseApp
    participant EVMKeeper
    participant Mempool

    note over CometBFT,Mempool: Old flow (removed)
    CometBFT->>BaseApp: FinalizeBlock
    BaseApp->>EVMKeeper: EndBlock
    EVMKeeper-->>Mempool: NotifyNewBlock (stale state)

    note over CometBFT,Mempool: New flow (this PR)
    CometBFT->>BaseApp: FinalizeBlock + Commit
    BaseApp->>BaseApp: Update check state
    BaseApp->>Mempool: PrepareCheckStater hook
    alt no event bus configured
        Mempool->>Mempool: NotifyNewBlock()
        Mempool->>Mempool: recheckCosmosPool.TriggerRecheck()
    else event bus configured
        CometBFT-->>Mempool: EventBus subscription fires
        Mempool->>Mempool: NotifyNewBlock()
        Mempool->>Mempool: recheckCosmosPool.TriggerRecheck()
    end
Loading

Reviews (2): Last reviewed commit: "Merge branch 'main' into ma/remove-mempo..." | Re-trigger Greptile

@mattac21 mattac21 force-pushed the ma/remove-mempool-endblock-notification branch from f47d9f4 to ba1429f Compare April 29, 2026 20:10
@mattac21
Copy link
Copy Markdown
Contributor Author

@greptile re review pls

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 29, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.20%. Comparing base (4b69c4e) to head (eee1af2).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1146      +/-   ##
==========================================
+ Coverage   65.27%   66.20%   +0.93%     
==========================================
  Files         336      336              
  Lines       23903    23844      -59     
==========================================
+ Hits        15602    15787     +185     
+ Misses       6937     6885      -52     
+ Partials     1364     1172     -192     
Files with missing lines Coverage Δ
mempool/mempool.go 81.29% <100.00%> (+22.53%) ⬆️
x/vm/keeper/abci.go 90.47% <ø> (-0.83%) ⬇️
x/vm/keeper/keeper.go 89.52% <ø> (-0.11%) ⬇️

... and 14 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@linear
Copy link
Copy Markdown

linear Bot commented Apr 29, 2026

@mattac21 mattac21 added this pull request to the merge queue Apr 29, 2026
Merged via the queue into main with commit 87f2772 Apr 29, 2026
18 checks passed
@mattac21 mattac21 deleted the ma/remove-mempool-endblock-notification branch April 29, 2026 20:28
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.

2 participants