feat: support tx replacement #1840
Conversation
thomas-nguy
requested review from
JayT106 and
songgaoye
and removed request for
a team
WalkthroughIntroduces AnteCache into the ante handler and wires mempoolMaxTxs through app initialization. Adds UnsafeUnorderedTx option in a nix patch. Updates Ethermint replace target/version. Adds integration tests and utilities for transaction replacement. Updates changelog. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Web3
participant Node
participant Mempool
participant Consensus
User->>Web3: sendTransaction(oldTx)
Web3->>Node: eth_sendRawTransaction(oldTx)
Node->>Mempool: enqueue oldTx (nonce N)
User->>Web3: replace_transaction(oldTxHash, newTx)
Web3->>Node: eth_replaceTransaction(...)
Node->>Mempool: replace oldTx with newTx (same nonce, higher gasPrice)
Mempool->>Consensus: propose newTx
Consensus-->>User: receipt for newTx
sequenceDiagram
participant Client
participant Mempool
participant AnteHandler
participant AnteCache
Client->>Mempool: CheckTx(tx)
Mempool->>AnteHandler: run ante checks
AnteHandler->>AnteCache: get(tx, mempoolMaxTxs)
alt cache hit
AnteCache-->>AnteHandler: result
else cache miss
AnteHandler->>AnteCache: put(tx, result)
end
AnteHandler-->>Mempool: validation result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
🧰 Additional context used🧬 Code Graph Analysis (1)integration_tests/test_basic.py (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
🔇 Additional comments (7)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
This comment has been minimized.
This comment has been minimized.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1840 +/- ##
===========================================
+ Coverage 16.87% 35.79% +18.91%
===========================================
Files 72 127 +55
Lines 6163 11812 +5649
===========================================
+ Hits 1040 4228 +3188
- Misses 5000 7161 +2161
- Partials 123 423 +300 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
app/app.go (1)
629-644: Add validation for query gas limit parameter.The query gas limit is directly cast from app options without validation. Consider adding bounds checking to ensure it's within reasonable limits.
+ queryGasLimit := cast.ToUint64(appOpts.Get(server.FlagQueryGasLimit)) + if queryGasLimit == 0 { + queryGasLimit = 50_000_000 // Set a default if not specified + } app.EvmKeeper = evmkeeper.NewKeeper( appCodec, keys[evmtypes.StoreKey], okeys[evmtypes.ObjectStoreKey], authtypes.NewModuleAddress(govtypes.ModuleName), app.AccountKeeper, app.BankKeeper, app.StakingKeeper, app.FeeMarketKeeper, tracer, evmS, []evmkeeper.CustomContractFn{ func(_ sdk.Context, rules ethparams.Rules) vm.PrecompiledContract { return cronosprecompiles.NewRelayerContract(app.IBCKeeper, appCodec, rules, app.Logger()) }, func(ctx sdk.Context, rules ethparams.Rules) vm.PrecompiledContract { return cronosprecompiles.NewIcaContract(ctx, app.ICAControllerKeeper, &app.CronosKeeper, appCodec, gasConfig) }, }, - cast.ToUint64(appOpts.Get(server.FlagQueryGasLimit)), + queryGasLimit, )
🧹 Nitpick comments (1)
integration_tests/test_basic.py (1)
1035-1060: Consider adding more comprehensive test coverage for transaction replacement.The test correctly verifies basic transaction replacement functionality. Consider enhancing it with:
- Explicit verification that the original transaction was replaced (compare transaction hashes)
- Negative test cases (e.g., replacement with insufficient gas price increase)
- Verification of the gas price in the final transaction
Example enhancement:
def test_tx_replacement(cronos): w3 = cronos.w3 gas_price = w3.eth.gas_price nonce = get_account_nonce(w3) initial_balance = w3.eth.get_balance(ADDRS["community"]) - txhash = replace_transaction( + result = replace_transaction( w3, { "to": ADDRS["community"], "value": 1, "gasPrice": gas_price, "nonce": nonce, "from": ADDRS["validator"], }, { "to": ADDRS["community"], "value": 5, "gasPrice": gas_price * 2, "nonce": nonce, "from": ADDRS["validator"], }, KEYS["validator"], - )["transactionHash"] + ) + txhash = result["transactionHash"] tx1 = w3.eth.get_transaction(txhash) assert tx1["transactionIndex"] == 0 + assert tx1["gasPrice"] == gas_price * 2, "Gas price should be doubled for replacement" assert w3.eth.get_balance(ADDRS["community"]) == initial_balance + 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
app/app.go(5 hunks)app/sim_test.go(1 hunks)app/test_helpers.go(2 hunks)go.mod(8 hunks)gomod2nix.toml(14 hunks)integration_tests/test_basic.py(2 hunks)integration_tests/utils.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-08T19:00:42.142Z
Learnt from: yihuang
PR: crypto-org-chain/cronos#1618
File: memiavl/db_test.go:193-194
Timestamp: 2024-10-08T19:00:42.142Z
Learning: In Go version 1.21 and later, the `max` function is defined and available.
Applied to files:
gomod2nix.tomlgo.mod
🧬 Code Graph Analysis (1)
integration_tests/utils.py (2)
integration_tests/network.py (1)
w3(39-42)integration_tests/cosmoscli.py (1)
address(302-312)
🔇 Additional comments (14)
gomod2nix.toml (3)
5-77: LGTM - Cosmos SDK dependency updates look good.The cosmossdk.io module version bumps appear to be appropriate patch/minor version updates with corresponding hash changes. The addition of
cosmossdk.io/schema v1.1.0aligns with the broader codebase changes.
151-663: Verify the necessity of new performance-oriented dependencies.Several new modules have been added that appear to be performance optimizations:
github.com/bytedance/sonic- High-performance JSON librarymygithub.libinneed.workers.dev/cloudwego/base64x- Optimized base64 encodingmygithub.libinneed.workers.dev/klauspost/cpuid/v2- CPU feature detectionmygithub.libinneed.workers.dev/twitchyliquid64/golang-asm- Assembly utilitiesPlease verify these dependencies are intentionally added and align with the project's performance optimization goals.
302-304: Verify personal ethermint fork in gomod2nix.tomlThe replacement for the ethermint module has been updated in gomod2nix.toml (from lines 302–304) from the organization’s repo to a personal fork:
• File: gomod2nix.toml, lines 302–304
• New entries:- replaced = "github.com/crypto-org-chain/ethermint" + replaced = "github.com/thomas-nguy/ethermint" version = "v0.4.2-0.20250807144210-5566b126ea2e" hash = "sha256-hzYos7zLb4zBhnnl03zxGEL/XYd92T4FMFM/c9HqCt4="Please confirm:
- Is switching to the personal fork intentional (e.g., for testing or specific features)?
- What fixes or enhancements does this fork include?
- Do you plan to revert this to the official crypto-org-chain/ethermint repo before release?
go.mod (2)
8-47: LGTM - Standard dependency updates.The direct dependency updates appear to be routine maintenance, including appropriate version bumps for cosmossdk.io modules and golang.org/x packages.
297-297: Critical: Justify Ethermint fork replacementThe
ethermintmodule has been redirected from the organization’s upstream to a personal fork in both dependency manifests. Before approving, please confirm and document why this change is necessary:• go.mod (line 297)
github.com/evmos/ethermint => github.com/thomas-nguy/ethermint v0.4.2-0.20250807144210-5566b126ea2e• gomod2nix.toml (line 304)
replaced = "github.com/thomas-nguy/ethermint"– What fixes or features does the personal fork provide over the official
crypto-org-chain/ethermintrelease?
– Is the forked code under active maintenance, and will it be kept up-to-date with security patches?
– Can these changes be upstreamed or merged back into the main project?Please add justification and consider whether this replacement should remain or be reverted to the canonical repository.
app/sim_test.go (1)
18-18: LGTM - Import path update aligns with architectural changes.The import path change from
app/antetoevmd/anteis consistent with the broader refactoring to consolidate EVM functionality into the evmd package structure.app/test_helpers.go (1)
19-19: LGTM - Consistent import and usage update for evmd package.The import path change and corresponding usage update (
evmd.DefaultNodeHome) are consistent with the architectural refactoring to use the evmd package structure.Also applies to: 73-73
integration_tests/utils.py (2)
454-456: LGTM - Clean and correct nonce retrieval utility.The
get_account_noncefunction is well-implemented, using the standardw3.eth.get_transaction_countmethod with appropriate account derivation from the private key.
465-469: LGTM - Well-implemented transaction replacement utility.The
replace_transactionfunction correctly implements the transaction replacement flow:
- Signs and sends the original transaction to obtain its hash
- Uses
w3.eth.replace_transactionwith the old hash and new transaction- Waits for the replacement transaction receipt
This implementation is appropriate for testing transaction replacement scenarios.
integration_tests/test_basic.py (1)
28-33: LGTM!The new utility function imports are correctly placed and follow the existing import pattern.
app/app.go (4)
65-69: LGTM!Import paths correctly updated to use the new
evmdsubmodule structure, consistent with the refactoring objectives.
383-391: LGTM!Good addition of logging statements for mempool configuration visibility. The SignerExtractor correctly migrated to use the evmd package.
1025-1036: LGTM!Transaction executor setup correctly migrated to use evmd package executors while preserving the conditional logic for block-stm mode.
1070-1091: Unable to Locate AnteCache Implementation—Please Verify Memory BoundsI wasn’t able to find the
cachepackage or theNewAnteCachedefinition in the repo. Before landing this change, please manually confirm that the AnteCache you’re instantiating:
- Has a configurable/bounded size (or sensible defaults)
- Implements eviction or TTL to prevent unbounded memory growth
- Includes any other resource‐management safeguards appropriate for long‐lived caches
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
app/app.go(7 hunks)go.mod(8 hunks)gomod2nix.toml(14 hunks)nix/testground-cronosd.patch(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- go.mod
- gomod2nix.toml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: Run golangci-lint
- GitHub Check: gomod2nix
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: unittest
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: Analyze (go)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (8)
nix/testground-cronosd.patch (1)
10-11: Review Testground ante handler divergence and cache sizing
- UnsafeUnorderedTx is enabled here but remains unset in the default
app/app.go. Please confirm this test-only divergence is intentional; otherwise align with the main handler or gate it behind a flag.cache.NewAnteCache(mempoolMaxTxs)may receive a negative value (e.g.-1in a NoOp mempool). Consider clamping or disabling the cache when inputs are negative to avoid unexpected behavior.Suggested clamp pattern before building options:
effectiveMempoolMaxTxs := mempoolMaxTxs if effectiveMempoolMaxTxs < 0 { // disable ante-cache when mempool size is negative effectiveMempoolMaxTxs = 0 } // … AnteCache: cache.NewAnteCache(effectiveMempoolMaxTxs),app/app.go (7)
65-70: Confirm ante/cache import path consistency with evmd reorgYou're importing:
- evmante from github.com/evmos/ethermint/evmd/ante
- cache from github.com/evmos/ethermint/ante/cache
If ante/cache also moved under evmd, adjust to evmd/ante/cache to avoid mismatched versions. If it remained at the root ante/, keep as-is. Please verify against the current Ethermint tree.
If needed:
- "github.com/evmos/ethermint/ante/cache" + "github.com/evmos/ethermint/evmd/ante/cache"
115-115: LGTM: sdk mempool importNo issues spotted with switching to types/mempool.
383-388: Priority mempool + evmd signer extractor — confirm compatibilitySwitching SignerExtractor to evmd.NewEthSignerExtractionAdapter should make the PriorityNonceMempool correctly extract account/nonce from Ethereum txs. Confirm it handles all supported tx types (legacy, EIP-1559, blob/4844 if enabled) and that priority ordering remains deterministic across rechecks.
390-392: LGTM: NoOp mempool branch loggingGood to have explicit logs; helps in diagnosing environment-specific behavior.
1034-1037: LGTM: switch to evmd Tx executorsSTMTxExecutor/DefaultTxExecutor migration looks correct. Using GetStoreKeys() (sorted) is appropriate for deterministic key ordering.
1043-1043: Method signature change: ensure all call sites updatedsetAnteHandler now requires mempoolMaxTxs. All internal callers appear updated here; verify any test helpers or app variants (e.g., sim/test helpers) also pass the new arg.
643-644: Confirm safe handling of zero query gas limit in EVM keeperI wasn’t able to locate the EVM keeper implementation in this repo’s code to verify how a zero value is treated. Please ensure that passing
cast.ToUint64(appOpts.Get(server.FlagQueryGasLimit))into
evmkeeper.NewKeeper(...)with a value of0is interpreted as “no cap” or otherwise safe. If a zero value would disable queries or underflow gas accounting, add a guard and default, for example:qgl := cast.ToUint64(appOpts.Get(server.FlagQueryGasLimit)) if qgl == 0 { qgl = evmtypes.DefaultQueryGasLimit } app.EvmKeeper = evmkeeper.NewKeeper(…, qgl)• Location: app/app.go:643–644
# Conflicts: # app/app.go # go.mod # go.sum
thomas-nguy
force-pushed
the
thomas/support-tx-replacement
branch
from
1303c0a to
8c6551d
Compare
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
PR Checklist:
make)make test)go fmt)golangci-lint run)go list -json -m all | nancy sleuth)Thank you for your code, it's appreciated! :)
Summary by CodeRabbit