Repository navigation
Add [giga] app.toml section and honor it on the Autobahn node - #4323
Conversation
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
|
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 @@
## main #4323 +/- ##
==========================================
- Coverage 67.49% 66.39% -1.11%
==========================================
Files 2181 2062 -119
Lines 167844 155830 -12014
==========================================
- Hits 113294 103463 -9831
+ Misses 54540 52357 -2183
Partials 10 10
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 A new sei-tendermint applies storage through Reviewed by Cursor Bugbot for commit d9d9fa2. Bugbot is set up for automated code reviews on this repo. Configure here. |
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The new [giga] section and how it reaches the Autobahn node are clean and well tested. The blocker is that giga.execution.min_gas_price is a per-node setting, but the EVM-only executor treats it as a block-validity rule. Nodes configured with different values will disagree on whether the same block is valid and halt.
Findings: 1 blocking | 1 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion]
startInProcesscallsgigaconfig.ReadConfig(ctx.Viper)for every node, not only Autobahn/Giga nodes. A malformed[giga]value, such as an unknownmodeorblock_result_pool_size = 0, will now stop a regular Cosmos node from starting even though it never uses the section. Consider reading and validating it only whenAutobahnConfigFileis set, or say in the template that the values are validated on every node.
…rice admission-only
|
Both review findings addressed in b8d0c0d: the executor's gas price floor stays the chain-wide constant with |
|
@seidroid review |
There was a problem hiding this comment.
The earlier blocker is fixed. The executor again uses the chain-wide 1 gwei block-validity floor, and min_gas_price now only sets the local admission floor, which is clamped to at least that value. TestEVMOnlyApplicationMinGasPriceIsAdmissionOnly checks that a node with a raised floor still reaches the same app hash as its peers, and [giga] is now read only on Autobahn nodes. The new section, its registry entry and its hand-off to the Autobahn node are well tested, and every default matches the previous hardcoded behaviour.
Findings: 0 blocking | 0 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
Superseded: latest AI review found no blocking issues.
| storageConfig.PruningConfig.LookbackWindow = storage.LookbackWindow | ||
| storageConfig.PruningConfig.PruneInterval = storage.PruneInterval | ||
| storageConfig.CheckpointConfig.TimeInterval = storage.CheckpointTimeInterval | ||
| storageConfig.CheckpointConfig.BlockInterval = storage.CheckpointBlockInterval |
There was a problem hiding this comment.
nit: maybe put this above storageConfig.WithFullNodeMode(), just in case in the future we want to set things separately for full node.
@yzang2019 should we have one storageConfig.WithFullNodeMode() and another storageConfig.WithValidatorMode() maybe?
There was a problem hiding this comment.
Reordered in 15e824d: the window/interval fields are set first, mode selection is last. I kept only the WithFullNodeMode() branch rather than adding an explicit WithValidatorMode(): AutobahnStorageConfig already applies validator mode and then re-enables receipts, so calling it again here would switch receipts off on validators (which TestPrepareApplicationAutobahnUsesEVMOnly pins via manager.ReceiptDB()). If we want a symmetric pair, that probably belongs in sei-db/config (e.g. an Autobahn-aware validator mode that keeps receipts on).
There was a problem hiding this comment.
@wen-coding i specifically tried to keep this PR smaller for now and make it as an unblocker. Was planning to move or delegate moving of storage configs in a separate PR.
| @@ -31,6 +32,7 @@ type CustomAppConfig struct { | |||
| WASM WASMConfig `mapstructure:"wasm"` | |||
| EVM evmrpcconfig.Config `mapstructure:"evm"` | |||
| GigaExecutor gigaconfig.Config `mapstructure:"giga_executor"` | |||
There was a problem hiding this comment.
I forgot, is GigaExecutor config for EVM-Mixed? It's quite confusing to have GigaExecutor and Giga back to back but separate.
There was a problem hiding this comment.
Yes: [giga_executor] (enabled/occ_enabled) is read by app.go and drives the Giga executor inside the Cosmos app (the EVM-mixed path). [giga] is only read when autobahn-config-file is set and is handed to the Autobahn/EVM-only node, which never constructs the Cosmos app. I kept them separate here so the pinned [giga_executor] keys and configtest rows don't churn; if you'd rather have one section, the low-churn option is to fold enabled/occ_enabled under [giga.executor] as a follow-up, or I can do it in this PR if you prefer.
There was a problem hiding this comment.
It is the mixed, a.k.a "Hybrid Executor".
Not worth the hassle of renaming IMHO, eventhough I agree that it is confusing.
Historical naming issue.
| return fmt.Errorf("%s: %q is not one of %q, %q or empty", FlagStorageMode, c.Storage.Mode, | ||
| StorageModeValidator, StorageModeFull) | ||
| } | ||
| if c.Storage.LookbackWindow < -1 { |
There was a problem hiding this comment.
We will seriously keep all history?
There was a problem hiding this comment.
This just mirrors the existing contract of StorageGarbageCollectorConfig.LookbackWindow in sei-db/config/gc_config.go (-1 = infinite retention, already accepted by its Validate); the default stays 0, so nothing keeps full history unless an operator opts in. I only surfaced the knob rather than tightening it; happy to reject -1 here if we'd rather archive nodes use a different mechanism.
|
|
||
| # block_result_pool_size is the number of block results kept pooled between executions. | ||
| block_result_pool_size = {{ .Giga.Execution.BlockResultPoolSize }} | ||
| ` |
There was a problem hiding this comment.
I thought we would move the Autobahn configs here? will that be done in a followup PR?
There was a problem hiding this comment.
Yes, follow-up. This PR is intentionally scoped to the storage/execution knobs that were hardcoded, so operators have one place (app.toml) that is actually honored. autobahn.json (committee, block/tx limits, timeouts, block_db) and the autobahn-config-file path in config.toml are untouched here; migrating block_db and then the rest into [giga.*] is the next step once we settle what should be configurable at all before mainnet.
|
LGTM, but please make sure someone from storage team has a chance to review |
| } | ||
|
|
||
| // DefaultConfig is what a Giga node runs when the section is absent. | ||
| var DefaultConfig = Config{ |
There was a problem hiding this comment.
Do we need to set another default again here? I'd propose we reuse whatever is already defined for all the default values. The concern of having two separate places for default is that we need to then always make sure they are consistent.
We can derive the config value from DefaultCheckpointConfig() and DefaultStorageGarbageCollectorConfig(0
There was a problem hiding this comment.
Done in d9d9fa2: the [giga.storage] defaults are now built from seidbconfig.DefaultStorageGarbageCollectorConfig() and seidbconfig.DefaultCheckpointConfig(), so sei-db stays the single definition. The execution defaults still live here since the executor floor is an unexported constant in evmonlyapp.
| // DefaultConfig is what a Giga node runs when the section is absent. | ||
| var DefaultConfig = Config{ | ||
| Storage: StorageConfig{ | ||
| Mode: StorageModeAuto, |
There was a problem hiding this comment.
What does auto mode mean? How do we automatically detect which mode to run?
There was a problem hiding this comment.
"Auto" (empty string) means: don't pin a Giga storage layout, follow the node's mode from config.toml. resolveGigaStorageMode in sei-tendermint/node/setup.go maps mode = "full" to the full layout (state store + receipts on) and anything else (validator, seed) to the validator layout (state store off). So by default a node that is already configured as a full node in config.toml gets query-serving storage without a second setting; mode = "validator"/"full" here only exists to override that, e.g. a validator that also wants the state store. Open to renaming the empty value to an explicit "auto" if that reads better.
* main: (21 commits) Backport evmonly parse, app-hash and changeset perf fixes from giga-1 (#4345) Remove the oracle module behind a v6.8 upgrade (#4319) fix(seidb): report only the current migration boundary on the snapshot gauge (#4327) fix(flatkv): keep 10 old checkpoints instead of mirroring memIAVL's count (#4322) Backport Autobahn execute-loop and produced-tx metrics from giga-1 (#4330) Add giga.storage.receipts to toggle the Autobahn receipt store (#4333) optimize gather phase (#4326) Regenerate the Unreleased changelog as a plain PR list (#4336) Bump sei-protocol/go-ethereum to v1.15.7-sei-21 (#4332) Fail dynamic-gas precompile out-of-gas as an EVM out-of-gas call (#4318) Add dashboard and topology option for Autobahn e2e (#4167) Add Giga fetch/serve and BlockDB prune metrics (#4329) Add eth_getLogs to the EVM-only Giga RPC (#4308) Generate v6.8 precompiles (#4320) Add [giga] app.toml section and honor it on the Autobahn node (#4323) feat(evmonly): add eth_estimateGas via existing libraries (#4325) Use Pebble batch directly in SS (#4300) Fix pruning issue in SS causing huge disk spike (#4321) Make Autobahn always run the EVM-only executor, disable/remove some integration tests (#4316) reduce seal lock contention (#4314) ...
3 participants
A Giga (Autobahn) node ignores
app.tomlfor everything storage- and execution-related.openAutobahnStorageManagerbuilds its layout fromseidbconfig.AutobahnStorageConfig, which hardcodes validator mode (state store off), the GC and checkpoint intervals, and never looks at the node's mode;evmonlyapplikewise hardcodes the 1 gwei admission floor,GOMAXPROCSOCC/parse workers and a result pool of one. Operators editingapp.tomlsee no effect, andautobahn.jsonis the wrong place for these knobs since it is a committee/consensus file.This adds a
[giga]section to the sameapp.tomlseidalready generates (alongside the existing[giga_executor]), split into[giga.storage](mode, rollback/lookback window, prune interval, checkpoint intervals) and[giga.execution](min gas price, OCC/parse workers, block result pool size). The newgiga/configpackage registers it in the section registry and exposesReadConfig(appOpts).server/start.goreads it and hands it tonode.Newthrough a newnode.WithGigaConfigoption, mirroringWithFreezeHeight, so sei-tendermint stays viper-free.buildGigaStorageConfigoverlays the values onAutobahnStorageConfig, andNewEVMOnlyApplicationnow takes anExecutionConfigthat sizes the executor and sets the CheckTx admission floor. The executor's own gas price floor stays the chain-wide 1 gwei constant, since it is block-validity policy every node must agree on;min_gas_priceonly filters local admission and is clamped to at least that floor, so a node can neither admit what a block would refuse nor reject a block its peers accept. Storagemodedefaults to following the Tendermintmode(fullopens the state store viaWithFullNodeMode, anything else stays validator), and can be pinned explicitly.Every default is identical to the previous hardcoded behaviour, so nodes without the section are unchanged;
TestBuildGigaStorageConfigDefaultsMatchAutobahnStorageConfigpins that. Block DB retention still comes fromautobahn.json; migrating it into[giga.storage]is left for a follow-up. Validated with the newgiga/config,sei-tendermint/nodeandevmonlyapptests plus the registry and configtest suites.