feat(ante): wire ethermint NewEVMSigPreVerifier for lock-free admission#2120
Conversation
Use ante.NewEVMSigPreVerifier from crypto-org-chain/ethermint#1011 instead of duplicating the logic in cronos. - Bump ethermint fork to 3247d33 (feat/evm-sig-preverifier branch) - Add preVerify field + SetPreVerify to mempool.Manager - Add lock-free pre-verify step in InsertTxHandler before mu.Lock() - Wire ethante.NewEVMSigPreVerifier(chainId, activeDecoder) in app.go
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a lock-free pre-verification hook to mempool admission, wires Ethermint’s EVM signature pre-verifier into app startup, and updates tests, dependency metadata, changelog, and one RPC receipt call. ChangesEVM Sig Pre-Verifier for Mempool Admission
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This comment has been minimized.
This comment has been minimized.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2120 +/- ##
==========================================
+ Coverage 16.87% 20.95% +4.08%
==========================================
Files 72 82 +10
Lines 6163 5655 -508
==========================================
+ Hits 1040 1185 +145
+ Misses 5000 4373 -627
+ Partials 123 97 -26
🚀 New features to boost your workflow:
|
|
@claude review |
|
Claude finished @JayT106's task in 3m 13s —— View job Code Review
Overall AssessmentThe implementation is clean and the design intent is sound: doing the expensive No test coverage for the new path
Consider adding at least one negative test before merging. Race-free only by convention
Dependency additions
No state-breaking concernsThis change only affects mempool admission (pre-
Safe to merge on a live release branch from a consensus/migration perspective. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Address review feedback on crypto-org-chain#2120: - Snapshot preVerify in InsertTxHandler for race-detector cleanliness - TestPreVerifyRejectsTamperedSig: tampered From rejected before RunTx - TestPreVerifyPassesThroughNonEVMTx: non-EVM tx falls through to antehandler
…field - Move chainId declaration before baseAppOptions closure so it can be captured; call SetPreVerify inside the closure before InsertTxHandler() so the preVerify snapshot captures the non-nil verifier - ResponseInsertTx has no Log field; keep Code-only response
The ethermint bump changes GetBlockReceipts param from BlockNumber to BlockNumberOrHash. BlockNumber accepted a plain decimal int (cast.ToUint64 fallback on missing 0x prefix); BlockNumberOrHash does not, so the raw int arg now returns an error and the response has no 'result'. Pass hex().
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/mempool/admission_test.go (1)
163-171: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThese assertions are too broad to prove the pre-verify contract.
At Line 170 and Line 186, checking only “not OK” allows false positives if
preVerifyis skipped/broken and rejection happens later inRunTx. Please add a focused path assertion (ideally a manager-level test with a spytxRunner) to verify:
- preVerify error short-circuits before
RunTx, and- non-EVM tx reaches
RunTx(preVerify passthrough).Also applies to: 173-187
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/mempool/admission_test.go` around lines 163 - 171, The assertions in TestPreVerifyRejectsTamperedSig are too broad and only verify the response code is not OK, which doesn't prove the rejection came from preVerify rather than from RunTx or later stages. Replace the current assertions in this test with more focused checks: implement a manager-level test with a spy or mock txRunner that tracks whether RunTx was actually called. This will verify that preVerify errors properly short-circuit execution before RunTx is invoked, and separately test that non-EVM transactions correctly pass through preVerify to reach RunTx, providing concrete evidence of the preVerify contract being enforced.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/mempool/admission_test.go`:
- Around line 163-171: The assertions in TestPreVerifyRejectsTamperedSig are too
broad and only verify the response code is not OK, which doesn't prove the
rejection came from preVerify rather than from RunTx or later stages. Replace
the current assertions in this test with more focused checks: implement a
manager-level test with a spy or mock txRunner that tracks whether RunTx was
actually called. This will verify that preVerify errors properly short-circuit
execution before RunTx is invoked, and separately test that non-EVM transactions
correctly pass through preVerify to reach RunTx, providing concrete evidence of
the preVerify contract being enforced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2b25f7ab-d9fb-4989-8cf8-328ac838aa49
📒 Files selected for processing (5)
app/app.goapp/mempool/admission_test.goapp/mempool/manager.gogomod2nix.tomlintegration_tests/test_basic.py
🚧 Files skipped from review as they are similar to previous changes (2)
- app/app.go
- app/mempool/manager.go
Ethermint moved the sig pre-verifier out of the ante package into the new appmempool package (stack crypto-org-chain#1016+crypto-org-chain#1019+crypto-org-chain#1011); ante/preverify.go is gone. - Repoint app.go: ethante.NewEVMSigPreVerifier -> appmempool.NewEVMSigPreVerifier. SigPreVerifier is still func([]byte) error, so SetPreVerify is unchanged. - Bump ethermint pin to 562fa2a2 (go.mod, go.sum, gomod2nix.toml).
Move PreVerifierRegistry out of ethermint into cronos per review (crypto-org-chain/ethermint#1011): the registry is an app-level composition concern. The app registers per-module verifiers — today just ethermint's NewEVMSigPreVerifier for EVM txs — and hands the composite registry.Verify to the mempool manager. The manager stays generic (SetPreVerify takes any func([]byte) error), so this is wiring only; no manager change.
…elop merge) - move PreVerifierRegistry from app/ to app/mempool/ (thomas-nguy suggestion) - tighten preVerify field comment in manager.go - drop Decode-before-locking comment; add Register EVM module preverifier comment - remove two-line comment block before registry setup in app.go - bump ethermint replace to v0.22.1-0.20260630170128-dadacd149b92 (develop merge)
Remove the local snapshot `preVerify := a.preVerify` — SetPreVerify is called once at startup before InsertTxHandler runs, so reading a.preVerify directly inside the closure is correct and clearer.
|
@JayT106 please update the latest ethermint and we can merge it 👌 |
2 participants
Depends on: crypto-org-chain/ethermint#1011
appmempool.NewEVMSigPreVerifierintoInsertTxHandler— reject bad EVM sigs beforemu.Lock().SetPreVerifyhook onmempool.Manager; nil skips.