Integrate Autobahn with in-memory EVM-only executor - #4028
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## codex/integrate-evmonly-giga-store #4028 +/- ##
======================================================================
- Coverage 58.03% 57.99% -0.05%
======================================================================
Files 2208 2207 -1
Lines 185680 185358 -322
======================================================================
- Hits 107761 107490 -271
+ Misses 68086 68053 -33
+ Partials 9833 9815 -18
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
PR SummaryMedium Risk Overview Docker / CI: RPC: Autobahn block height validation for Reviewed by Cursor Bugbot for commit a81e71a. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a81e71a. Configure here.
| return nil, nil, err | ||
| } | ||
| logger.Warn("Autobahn EVM-only in-memory execution enabled; state is ephemeral and unsafe for persistent networks") | ||
| app = p2p.NewEVMOnlyInMemoryProxy(config.AutobahnEVMOnlyInMemoryChainID, maxGas, genDoc.InitialHeight) |
There was a problem hiding this comment.
Status height still uses Cosmos app
Medium Severity
EVM-only mode replaces the app only inside buildGigaRouter, so env.App stays the Cosmos application that never executes Autobahn blocks. /block now reads giga.LastCommittedBlockNumber(), but /status and /abci_info still take LatestBlockHeight and LastBlockAppHash from env.App.Info(), which remains at genesis. Standard height polling therefore reports a halted chain while validators are producing blocks.
Reviewed by Cursor Bugbot for commit a81e71a. Configure here.
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
A well-scoped, test-only Autobahn EVM-only in-memory executor: the app-hash derivation is deterministic (the executor's changeset is sorted before encoding), the RPC head switch to the GigaRouter is behaviour-preserving for the existing Cosmos-app path, and the docker port/env plumbing checks out. No blockers; the notes below concern failure modes of the new runtime (a tx-level execution error halts the whole cluster), an unenforced config constraint, and a few structural/robustness cleanups.
Findings: 0 blocking | 6 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] Layering:
evmonly_inmemory_app.golands insei-tendermint/internal/p2p, andsei-tendermint/configimports that package — so the config package now transitively depends ongiga/evmonlyand the whole go-ethereum EVM. It compiles today only because nothing undergiga/evmonlyreaches back tosei-tendermint/config; a future import there becomes a cycle. The app has no p2p concern (it is only constructed fromnode/setup.go); its own package would keep the dependency out ofconfig's import graph. - [suggestion] The block's changeset is encoded twice per block: once inside
ExecutePreparedBlock(via theWithStore(store, store.EncodeChangeSet)encoder) and again inhashEVMOnlyInMemoryResult(evmonly.EncodeMemoryStoreChangeSet(result.ChangeSet)). On the 4,000-tx blocks this harness exists to measure, that doubles changeset allocation on the hot path.result.ChangeSetis already deterministically sorted, so hashing it directly avoids the second encode. - 4 suggestion(s)/nit(s) flagged inline on specific lines.
| panic("unreachable") | ||
| } | ||
|
|
||
| func (a *evmOnlyInMemoryApplication) CheckTx(_ context.Context, req *abci.RequestCheckTxV2) *abci.ResponseCheckTxV2 { |
There was a problem hiding this comment.
[suggestion] CheckTx validates signature, chain ID, tx type and gas price, but not the constraints that make a tx executable, while FinalizeBlock turns any tx-level execution failure into a whole-block error. executeBlockSequential returns fmt.Errorf("execute tx %d ...") on intrinsic-gas-too-low, nonce mismatch or insufficient funds, which propagates out of FinalizeBlock → gigaRouterCommon.executeBlock → the execute loop, halting the node — and because the failure is deterministic, every validator halts on the same block and the ephemeral state makes it unrecoverable.
The producer mempool already closes part of this (it rejects EVMNonce != app.EvmNonce and gas beyond the per-block budgets), so the residual gap is a well-signed, correctly-nonced tx with Gas below intrinsic cost (e.g. 21,000 for a transfer, more with calldata), and nonce races when the same sender is broadcast to two validators' lanes. A cheap fix is an intrinsic-gas check here; the more robust one is to report a failed tx as a non-zero Code in evmOnlyABCIResults (which today hardcodes CodeTypeOK even for reverted txs) rather than failing the block, matching how the Cosmos app behaves. Scoped to the load-test mode, hence a suggestion rather than a blocker.
| // EVMOnlyInMemory replaces the Cosmos application used by Autobahn with an | ||
| // ephemeral EVM-only executor. It exists only for Docker load testing and | ||
| // must not be enabled on a persistent network. | ||
| EVMOnlyInMemory bool `json:"evm_only_in_memory,omitzero"` |
There was a problem hiding this comment.
[suggestion] The doc comment says this "must not be enabled on a persistent network", but nothing enforces it: AutobahnFileConfig.Validate accepts evm_only_in_memory: true together with persistent_state_dir, and only the docker script happens to del(.persistent_state_dir). A hand-written config with both set gets persisted consensus blocks over an EVM state that resets to the funded base on every restart. Validate() is the choke point every config load passes through — rejecting the combination there makes it an invariant instead of a convention the next config author has to remember.
| return env.getHeight(env.App.Info().LastBlockHeight, heightPtr) | ||
| giga, ok := env.gigaRouter().Get() | ||
| if !ok { | ||
| panic("autobahnCheckAndGetHeight called without GigaRouter") |
There was a problem hiding this comment.
[suggestion] Both callers (Block at line 100, BlockResults at line 261) already hold the router from their own env.gigaRouter().Get() check, so this second lookup can never fail and the panic is unreachable by construction — but it is a panic on the RPC surface if that ever stops holding. Taking giga p2p.GigaRouter as a parameter removes both the redundant lookup and the panic.
| seid tendermint gen-autobahn-config $NODE_DIRS --output "$AUTOBAHN_CONFIG" | ||
| if [ "$AUTOBAHN_EVMONLY_IN_MEMORY" = "true" ]; then | ||
| AUTOBAHN_CONFIG_TMP="$AUTOBAHN_CONFIG.tmp" | ||
| jq '.evm_only_in_memory = true | del(.persistent_state_dir)' "$AUTOBAHN_CONFIG" > "$AUTOBAHN_CONFIG_TMP" |
There was a problem hiding this comment.
[suggestion] This drops the && guard the repo's other jq rewrites use (override_genesis in step2_genesis.sh:13 is jq ... > tmp && mv tmp target). The script has no set -e, so if jq fails the mv on the next line still runs and replaces autobahn.json with a truncated file, and the node fails later with an unrelated-looking config error. Chaining > "$AUTOBAHN_CONFIG_TMP" && mv ... restores the existing idiom.
| } | ||
| } | ||
|
|
||
| func evmOnlyInMemoryEnabled() bool { |
There was a problem hiding this comment.
I acknowledge that this is for integration testing, but are there facilities to auto-map env variables to the json config? I'm only seeing JSON for AutobahnFileConfig.
| seid tendermint gen-autobahn-config $NODE_DIRS --output "$AUTOBAHN_CONFIG" | ||
| if [ "$AUTOBAHN_EVMONLY_IN_MEMORY" = "true" ]; then | ||
| AUTOBAHN_CONFIG_TMP="$AUTOBAHN_CONFIG.tmp" | ||
| jq '.evm_only_in_memory = true | del(.persistent_state_dir)' "$AUTOBAHN_CONFIG" > "$AUTOBAHN_CONFIG_TMP" |
There was a problem hiding this comment.
You can pass empty string to persistent_state_dir, which I think will disable persistence.
| return nil, nil, err | ||
| } | ||
| logger.Warn("Autobahn EVM-only in-memory execution enabled; state is ephemeral and unsafe for persistent networks") | ||
| app = p2p.NewEVMOnlyInMemoryProxy(config.AutobahnEVMOnlyInMemoryChainID, maxGas, genDoc.InitialHeight) |
There was a problem hiding this comment.
I think this only updates buildGigaRouter's local app. Maybe try to do it like MockApp, do it earlier in node.New? #3905
3 participants
Describe your changes and provide context
This PR is stacked on #3864.
Testing performed to validate your change
make fmtcheck was attempted but golangci-lint v2.8.0 exits before scanning files because its binary reports Go 1.24.5 while this repository targets Go 1.25.6.