[NIT-4242] Add initial L1 base fee to node configuration #4171
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4171 +/- ##
===========================================
+ Coverage 33.01% 56.93% +23.92%
===========================================
Files 459 461 +2
Lines 55830 55942 +112
===========================================
+ Hits 18430 31850 +13420
+ Misses 34185 19260 -14925
- Partials 3215 4832 +1617 |
❌ 4 Tests Failed:
View the top 3 failed tests by shortest run time
📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
|
LGTM |
There was a problem hiding this comment.
As you mentioned in a meeting, since there are multiple valid ways to serialize chain config, in order to Execution build parsedInitMessage only through its local config, users will also need to have a way to provide the serialized chain config.
This can be done in this PR, a new PR, or even a new linear task can be created for that, up to you to decide the best path to move forward 🙂
| return fmt.Errorf("invalid value of rebuild-local-wasm, want: auto or force or false, got: %s", c.RebuildLocalWasm) | ||
| } | ||
| if _, success := big.NewInt(0).SetString(c.InitialL1BaseFee, 10); !success { | ||
| return fmt.Errorf("failed to parse L1 BaseFee for L2 config: `%s`", c.InitialL1BaseFee) |
There was a problem hiding this comment.
nitpick: change error message to not use "L2 config", and use something related to init instead.
|
|
||
| parsed, success := big.NewInt(0).SetString(c.InitialL1BaseFee, 10) | ||
| if !success { | ||
| return nil, fmt.Errorf("failed to parse L1 BaseFee for L2 config: `%s` (passed with `initial-l1base-fee` flag)", c.InitialL1BaseFee) |
There was a problem hiding this comment.
nitpick: change error message to not use "L2 config", and use something related to init instead.
| return executionDB, nil, fmt.Errorf("incompatible chain config read from init message in L1 inbox: %w", err) | ||
| } | ||
| } | ||
| if configuredInitialL1BaseFee.Cmp(parsedInitMessage.InitialL1BaseFee) != 0 { |
There was a problem hiding this comment.
In the same way we can also compare the provided serialized chain config with the parsedInitMessage's serialized chain config.
| return executionDB, nil, fmt.Errorf("incompatible chain config read from init message in L1 inbox: %w", err) | ||
| } | ||
| } | ||
| if configuredInitialL1BaseFee.Cmp(parsedInitMessage.InitialL1BaseFee) != 0 { |
There was a problem hiding this comment.
How about also creating a function, such as getExecutionParsedInitMessage, similar to the GetConsensusParsedInitMessage that Igor is proposing here?
| log.Info("Read serialized chain config from init message", "json", string(parsedInitMessage.SerializedChainConfig)) | ||
| } else { | ||
| serializedChainConfig, err := json.Marshal(chainConfig) | ||
| serializedChainConfig, err := resolveSerializedChainConfig(genesisArbOSInit, &config.Init, chainConfig) |
There was a problem hiding this comment.
This is not where this resolveSerializedChainConfig call should be.
This else represents a use case in which a L2 node doesn't have connection with the parent chain.
Think of it as a child chain that doesn't settle data in the parent chain.
I am not sure if anyone is relying on this use case today.
The returns of resolveSerializedChainConfig and resolveInitialL1BaseFee should be used to build a ParsedInitMessage object (parsedInitMessage built only through local configs, and only to be used by Execution).
In this PR, if the user provided enough info to build this ParsedInitMessage object, it should be compared with the parsedInitMessage returned from initMessage.ParseInitMessage (parsedInitMessage read from the parent chain, and only to be retrieved by Consensus).
This affects where to place resolveSerializedChainConfig and resolveInitialL1BaseFee calls, parsedInitMessage comparison, and this PR overall design.
Maybe checking Igor's PR will be simpler to understand what openInitializaExecutionDB does.
Makes sense?
| return rebuildLocalWasm(ctx, &config.Execution, l2BlockChain, executionDB, wasmDB, config.Init.RebuildLocalWasm) | ||
| } | ||
|
|
||
| func resolveInitialL1BaseFee(genesisArbOSInit *params.ArbOSInit, initConfig *conf.InitConfig) (*big.Int, error) { |
There was a problem hiding this comment.
This func will be easier to read if you follow a similar approach that you followed in resolveSerializedChainConfig.
You don't assign a default value to feeCLIFlag and feeGenesisJSON, and only compare them if both are provided.
You only look for a default value if both are not provided.
WDYT?
| if !tc.shouldErr && err != nil { | ||
| Fail(t, "unexpected error:", err) | ||
| } | ||
| if !reflect.DeepEqual(resolvedConfig, tc.expected) { |
There was a problem hiding this comment.
nitpick: Here bytes.Equal can be used instead of DeepEqual.
| chainConfigSerialized1 := []byte(`{"chainId":1}`) | ||
| chainConfigSerialized2 := []byte(`{"chainId":2}`) |
There was a problem hiding this comment.
nitpick: renaming these to chainConfigGenesisJSON and chainConfigInitFlag can help with reading the code. WDYT?
InitConfigwithInitialL1BaseFeefield. Since the fee might exceed1<<64, the field type isstringand we have a getterInitialL1BaseFeeParsedthat returns:DefaultInitialL1BaseFeewhen the string is empty or it cannot be parsed in decimal systemInitConfigwithSerializedChainConfigfield.cmd/nitro/init.goresolve† the new parameters from either CLI flag or genesis configuration. Ensure they match what was in the parsed init message on the parent chain.† config resolution:
genesis.jsonis absent, use the--chain.initial-l1base-feeflag valuegenesis.jsonis present and--chain.initial-l1base-feeis not empty, ensure the values matchgenesis.jsonSimilar logic is applied to the serialized chain configuration resolution. Both resolutions are covered with tests.
closes NIT-4242, NIT-4264
pulls in OffchainLabs/go-ethereum#603