op-node: remove superchain cfg hardcodes #11427
Conversation
bitwiseguy
force-pushed
the
ss/remove-superchain-cfg-hardcodes
branch
from
5ada728
to
19e8572
Compare
| @@ -94,12 +84,15 @@ func LoadOPStackRollupConfig(chainID uint64) (*Config, error) { | |||
| PlasmaConfig: plasma, | |||
| } | |||
|
|
|||
| if chConfig.ChannelTimeoutBedrock != nil { | |||
There was a problem hiding this comment.
do we need to return an error if ChannelTimeoutBedrock is missing in the config? I gather that ChannelTimeoutGranite is optional at this point?
There was a problem hiding this comment.
In my eyes, we have a separation of duties. The LoadOPStackRollupConfig function reads from the superchain module and converts the superchain.ChainConfig struct into a rollup.Config struct. Then we have a Config.Check function that checks the validity of the Config. That is where we return errors, including one for ErrMissingChannelTimeout here
op-node/chaincfg/chains_test.go
Outdated
| @@ -129,7 +129,7 @@ var sepoliaDev0Cfg = rollup.Config{ | |||
| MaxSequencerDrift: 600, | |||
| SeqWindowSize: 3600, | |||
| ChannelTimeoutBedrock: 300, | |||
| ChannelTimeoutGranite: 50, | |||
| ChannelTimeoutGranite: 0, | |||
There was a problem hiding this comment.
I feel like there shouldn't be a configurable ChannelTimeoutGranite - it should just become 50 everywhere if granite is active. Wouldn't this reduce complexity? Any consensus values hardcoded and not configurable. I was trying to do this in #10628 in a backwards compatible way
There was a problem hiding this comment.
Will ChannelTimeoutGranite always be == 50, both now and in the future, for every chain (not just standard chains, but also frontier)? If so, I can put the hardcode back in. If we might allow other values in the future, perhaps we should read the value from the config imported via the superchain-registry. I can update the Config.Check() function so that we return an error if its an invalid value (maybe just != 50 for now?)
There was a problem hiding this comment.
I don't see any reason why this needs to be configurable - it just introduces another way you can break things by putting in a bad configuration value and it requires a fair bit of detail to understand the trade offs and pick a good value. And if we do need to change it in future, we'd have to do it in a new hard fork so we'd introduce a new ChannelTimeoutXXX rather than configuring ChannelTimeoutGranite.
There was a problem hiding this comment.
I don't know enough about the full effects of the ChannelTimeout parameters to have a strong opinion. I was more so following the preference of @geoknee @sebastianst (see this slack thread).
However, it seems confusing to me to include these params in a config file if they are not configurable, and rely on downstream software to always overwrite the values. Should we change these json tags so they are not included in the rollup.json files?
Also should we change the Config.Check() function so that it enforces those exact values, instead of insinuating that other values are permitted?
There was a problem hiding this comment.
I think it's worth reminding ourselves the purpose of LoadOPStackRollupConfig. It is loading configuration data from the superchain-registry, including for frontier chains which are supposed to have pretty free reign over how they want to configure their chains. We are trying to maintain the invariant that the superchain-registry + LoadOPStackRollupConfig preserve all of the data in the rollup.json file so it can be reproduced perfectly. We can't get to that state if LoadOPStackRollupConfig is using hardcoded values.
I agree with @bitwiseguy that it seems strange to put params in a config file if they shouldn't ever be configured. Actually baking them into the op-node source code would make more sense for that (as @bitwiseguy suggests).
Furthermore I think this function is probably not the place to try and enforce defaults or parameters-which-must-not-ever-change? The defaults should be handled by the tooling which generates the rollup.json, and anything which must never change could be checked when op-node starts up.
It could be that I missing something, but I just wanted to try and explain where we are coming from with this change.
There was a problem hiding this comment.
Also if I understand correctly we are currently in a state where the default value for MaxSequencerDrift has been updated to 1800 in the default rollup.json (with the Fjord upgrade), but is being overridden to the old value of 600 by LoadOPStackRollupConfig. If we move forward with the changes from this PR, we won't have to remember to update LoadOPStackRollupConfig when changes like that happen, and we will be able to support a chain in the superchain-registry if (for example) they wanted to not do the Fjord upgrade and use the old parameters.
There was a problem hiding this comment.
Thanks for calling this out Mark.
The op-node relies on the superchain-registry to pull chain-specific configuration. The channel_timeout is a protocol parameter that has superchain-wide implications for fault proofs. As such the we should not be pulling it from the superchain-registry. Instead, the op-node must use constants for these sort of configuration; ensuring that every chain does not deviate from specific behavior required for protocol upgrades.
Regarding frontier chains, they shouldn't be allowed to configure the channel_timeout. It's extra configurability that every chain operator needs to check for for little benefit. I'll be making a change to the op-node to treat channel timeout as protocol constants rather than configs.
Let's revert the changes here to load the channel_timeout values from the superchain-config. I'll follow up to use protocol constants for these instead.
There was a problem hiding this comment.
Yeah I'm happy with just hard coding ChannelTimeout in op-node. The granite version wound up here because that's where the existing ChannelTimeout was being set so I followed that pattern. Definitely on board with cleaning things up and putting these kinds of hard coded values in a better place.
There was a problem hiding this comment.
The one case I just thought of those is that we do modify the ChannelTimeout in e2e tests. For example
optimism/op-e2e/actions/l2_batcher_test.go
Line 439 in 7f403ea
There was a problem hiding this comment.
Looking at Mofi's PR it doesn't seem to matter if we can't configure it so 🤷
…tead of hardcoding
bitwiseguy
force-pushed
the
ss/remove-superchain-cfg-hardcodes
branch
from
19e8572
to
5f2ce28
Compare
|
@bitwiseguy It is true that |
|
Closing in favor of #11459, which does not make any changes to how |
Read the following fields from the superchain config file instead of harding as part of
LoadOPStackRollupConfig:MaxSequencerDriftChannelTimeoutBedrockChannelTimeoutGraniteComplementary pr in superchain-registry: ethereum-optimism/superchain-registry#459