Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug in SystemConfig Holocene Upgrade #394

Open
tynes opened this issue Dec 10, 2024 · 9 comments · May be fixed by #418
Open

Bug in SystemConfig Holocene Upgrade #394

tynes opened this issue Dec 10, 2024 · 9 comments · May be fixed by #418
Assignees

Comments

@tynes
Copy link
Contributor

tynes commented Dec 10, 2024

The Holocene SystemConfig upgrade simply modified the implementations of the SystemConfig contracts rather than calling initialize on them. This means that there are two functions that currently do not work as expected.

This is because these values are their own storage slots rather than functions that read from a single source of truth (the scalar). Without calling initialize or calling setGasConfigEcotone, they will return 0.

We have a few options:

  • redo the upgrade and call initialize as part of the upgrade
  • do a new task that calls setGasConfigEcotone to set the values equal to what they are currently
  • leave them as 0 and update the implementation of the SystemConfig to parse from the scalar value when calling these functions

These functions would implement the inverse of this:

scalar = (uint256(0x01) << 248) | (uint256(_blobbasefeeScalar) << 32) | _basefeeScalar;

So the basefeeScalar function would sload the scalar, and mask out the least significant 32 bits. The blobBasefeeScalar function would mask out the next least most significant 32 bits.

@tynes
Copy link
Contributor Author

tynes commented Dec 10, 2024

Decision: we are going to redo the upgrade and call initialize as part of the upgrade

The post checks should assert specifically on basefeeScalar() and blobBasefeeScalar()

@geoknee geoknee linked a pull request Dec 17, 2024 that will close this issue
3 tasks
@BlocksOnAChain BlocksOnAChain changed the title Bug in Holocene Upgrade Bug in systemconfig Holocene Upgrade Dec 17, 2024
@BlocksOnAChain BlocksOnAChain changed the title Bug in systemconfig Holocene Upgrade Bug in SystemConfig Holocene Upgrade Dec 18, 2024
@BlocksOnAChain BlocksOnAChain moved this from Todo to In Progress in OP Stack Upgrades Dec 18, 2024
@geoknee
Copy link
Contributor

geoknee commented Dec 18, 2024

@tynes what should we do for chains that have a scalar set to something we can't decode? See here https://sepolia.etherscan.io/address/0x5D63A8Dc2737cE771aa4a6510D063b6Ba2c4f6F2#readProxyContract#F33

@tynes
Copy link
Contributor Author

tynes commented Dec 18, 2024

Can you give more details? I don't understand why you cant decode 684000. Encoded as bytes32, it is version 0 so it should be fine

@sebastianst
Copy link
Member

Oh yeah right, that's a pre-Ecotone encoding, so this is a base fee scalar of 684000 (and implicitly blob base fee scalar of 0) because it's a version 0 pre-Ecotone encoding. See specs for more detail @geoknee on how to parse version-0 scalars.

@geoknee
Copy link
Contributor

geoknee commented Dec 18, 2024

That makes sense. I had thought there was a problem because I couldn't get the ecotone-scalar to decode it. I think it is a bit buggy, will send a fix.

@geoknee
Copy link
Contributor

geoknee commented Dec 18, 2024

@geoknee
Copy link
Contributor

geoknee commented Dec 20, 2024

Just to point out that initialize will reencode with version 1 of the scalar encoder, meaning that chains with a version 0 scalar will get a non trivial change to the raw data of scalar (even if semantically there is no change).

@sebastianst
Copy link
Member

Just to point out that initialize will reencode with version 1 of the scalar encoder, meaning that chains with a version 0 scalar will get a non trivial change to the raw data of scalar (even if semantically there is no change).

I discussed this with George. I believe it should be fine because both encodings lead to the same outcome (if blob base fee is 0). We're not emitting a config change event during initialize(), so this just migrates the contract state into a consistent state.

@tynes
Copy link
Contributor Author

tynes commented Dec 20, 2024

Just to point out that initialize will reencode with version 1 of the scalar encoder, meaning that chains with a version 0 scalar will get a non trivial change to the raw data of scalar (even if semantically there is no change).

What does non trivial mean? From the pov of bytes32, its a single bit. From the pov of uint256, its a massively different number. Is this what you mean? This is expected behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging a pull request may close this issue.

3 participants