Holocene: contracts part for separating dynamic&static l1 attributes #11600

blockchaindevsh · 2024-08-26T08:02:04Z

This PR serves as a first step for "Separate Dynamic and Static L1 Attributes Values", it migrates existing SystemConfig updates to follow the pattern designed in ethereum-optimism/specs#122 .

blockchaindevsh · 2024-08-26T11:35:22Z

The remaining test failure is here.

It seems the testcase itself is not sound:

SystemConfig/OptimismPortal2 contracts are counted twice by _getNumInitializable() because of the existence of SystemConfigInterop/OptimismPortalInterop .
OptimismSuperchainERC20 is not excluded by _getNumInitializable(), but the comment Omitting OptimismSuperchainERC20 due to using OZ v5 Initializable. indicates it should.
The contracts variable includes DisputeGameFactory, but it's under dispute directory, not under L1/L2 directory(_getNumInitializable() only scans L1/L2).

roberto-bayardo · 2024-08-26T21:47:59Z

packages/contracts-bedrock/src/L2/L1BlockHolocene.sol

+        if (msg.sender != DEPOSITOR_ACCOUNT()) revert NotDepositor();
+
+        if (_type == ConfigType.SET_GAS_PAYING_TOKEN) {
+            _setGasPayingToken(_value);
+        } else if (_type == ConfigType.ADD_DEPENDENCY) {
+            _addDependency(_value);
+        } else if (_type == ConfigType.REMOVE_DEPENDENCY) {
+            _removeDependency(_value);


not a solidity expert can/should you call "super.setConfig(_type, _value)" instead of duplicating this code here?

Note: L1BlockHolocene will be shipped before L1BlockInterop, and L1BlockInterop.setConfig is external instead of public, so not callable from inherited functions.

roberto-bayardo · 2024-08-26T22:08:48Z

packages/contracts-bedrock/src/L2/L1BlockInterop.sol

-    REMOVE_DEPENDENCY
+    REMOVE_DEPENDENCY,
+    SET_BATCHER_HASH,
+    SET_GAS_CONFIG_ECOTONE,


Can we replace this with two new config type events for each of the two values we hacked into the gas config with Ecotone? E.g.

SET_BASE_FEE_SCALAR
SET_BLOB_BASE_FEE_SCALAR

...allowing each to be updated independently.

I am on board with this if you think that it results in less operational overhead, this just means a multicall is necessary to set both

Alternatively just have a SET_FEE_SCALARS for both ... Important thing is to get away from the Ecotone hack that combined both fee scalars into the single "scalar" L1 attribute.

Personally I don't think it's worth having two ways to do the same thing. Probably it's best to follow Mark's advice of starting with a specification where we can argue over these API details before turning it into code?

roberto-bayardo · 2024-08-26T22:12:19Z

packages/contracts-bedrock/src/L1/SystemConfig.sol

@@ -387,7 +387,7 @@ contract SystemConfig is OwnableUpgradeable, ISemver, IGasToken {
    /// @notice Internal function for updating the fee scalars as of the Ecotone upgrade.
    /// @param _basefeeScalar     New basefeeScalar value.
    /// @param _blobbasefeeScalar New blobbasefeeScalar value.
-    function _setGasConfigEcotone(uint32 _basefeeScalar, uint32 _blobbasefeeScalar) internal {
+    function _setGasConfigEcotone(uint32 _basefeeScalar, uint32 _blobbasefeeScalar) internal virtual {


If we separate out the gasconfig into two update types like I suggested, we can deprecate setGasConfigEcotone entirely. (Of course we have make sure this one works if someone calls it, so we'll still have to override this implementation in SystemConfigHolocene.)

Oh and you'll also want to change "_setGasConfig" to virtual so we can override it to make it do the right thing. Though since setGasConfig() has been deprecated since Ecotone, you might also just remove _setGasConfig() entirely, then make setGasConfig() revert, with the error indicating to use set(Blob)BasefeeScalar() instead.

Correction to my statement above, setGasConfig() was actually not deprecated until after Ecotone (Ecotone didn't involve any changes to L1 contracts, which is why we had to hack in the new behavior using the old API). The setGasConfigEcotone() was added in a later update of the L1 contracts. IMO it should have been named setFeeScalars() -- I don't think hardfork names should be in public APIs if not entirely necessary. Regardless, since setGasConfig() wasn't deprecated until later, probably we want to keep it working for a bit longer rather than removing it entirely or just making it revert. Its implementation of course can just be made to call the new contract function(s) after appropriately parsing out the values.

roberto-bayardo · 2024-08-26T22:13:54Z

packages/contracts-bedrock/src/L1/SystemConfigHolocene.sol

+    /// @notice Internal function for updating the fee scalars as of the Ecotone upgrade.
+    /// @param _basefeeScalar     New basefeeScalar value.
+    /// @param _blobbasefeeScalar New blobbasefeeScalar value.
+    function _setGasConfigEcotone(uint32 _basefeeScalar, uint32 _blobbasefeeScalar) internal override {


Implementation here could just be:

_setBasefeeScalar(_basefeeScalar);
_setBlobBasefeeScalar(_blobBasefeeScalar);

roberto-bayardo · 2024-08-26T22:16:53Z

packages/contracts-bedrock/src/L2/L1BlockHolocene.sol

+
+    /// @notice Internal method to set new gas config.
+    /// @param _value The encoded value with which to set the new batcher hash.
+    function _setGasConfigEcotone(bytes calldata _value) internal {


drop, replace with _setBasefeeScalar() and _setBlobBasefeeScalar()

roberto-bayardo · 2024-08-26T22:17:31Z

packages/contracts-bedrock/src/L2/L1BlockHolocene.sol

+    event BatcherHashSet(bytes32 indexed batcherHash);
+
+    /// @notice Event emitted when a new ecotone gas config is set.
+    event GasConfigEcotoneSet(uint32 indexed blobBaseFeeScalar, uint32 indexed baseFeeScalar);


replace w/ BasefeeScalarSet and BlobBasefeeScalarSet.

(nit: Note that "Basefee" and "basefee" are the idiomatic casings, whereas "BaseFee" and "baseFee" are not.)

I changed the naming convention to the idiomatic casings for the new code, but didn't change the existing naming convention in the old code, e.g.,

optimism/packages/contracts-bedrock/src/L2/L1Block.sol

Line 41 in a36291d

uint32 public blobBaseFeeScalar;

and

optimism/packages/contracts-bedrock/src/L2/L1Block.sol

Line 44 in a36291d

uint32 public baseFeeScalar;

, since it may break things. We can do it in the future.

roberto-bayardo · 2024-08-26T22:25:49Z

Summarizing most of my comments above:

I think we should use this change as an opportunity to undo the ugly Ecotone gas config hack where we smash basefeeScalar and blobBasefeeScalar into a single value.

For SystemConfigHolocene.sol, suggest adding:

setBasefeeSalar()
setBlobBasefeeScalar()

For SystemConfig.sol, deprecate:

setGasConfig (make it revert, or make it virtual so we can override it to make it do the right thing if it still gets called.
setGasConfigEcotone (make it virtual, then Holocene implementation will just call the two new methods above).

tynes · 2024-08-26T23:39:50Z

packages/contracts-bedrock/src/L2/L1BlockHolocene.sol

+/// @custom:predeploy 0x4200000000000000000000000000000000000015
+/// @title L1BlockHolocene
+/// @notice Interop extenstions of L1Block.
+contract L1BlockHolocene is L1BlockInterop {


We don't want to target L1BlockInterop here since this will go to production before interop. I think naming this L1BlockHolocene for now is safer, even though I think this could technically go right into the L1Block contract itself. By not putting this logic into L1Block now, it reduces risk that somebody deploys with it before its ready end to end, we will need to pull it into the regular L1Block contract in the future.

Mark, should we also be modifying SystemConfig.sol directly rather than introducing a SystemConfigHolocene.sol?

Good catch, fixed in 4618303 .

tynes · 2024-08-26T23:41:45Z

packages/contracts-bedrock/src/L2/L1BlockHolocene.sol

+    ///   4. _basefee            L1 base fee.
+    ///   5. _blobBaseFee        L1 blob base fee.
+    ///   6. _hash               L1 blockhash.
+    function setL1BlockValuesHolocene() external {


We will want to implement the following comment as setL1BlockValuesHolocene: ethereum-optimism/specs#122 (comment)

Done in 4618303

tynes · 2024-08-26T23:49:19Z

This is great work so far, ultimately we will want a more formal spec before we can merge this to ensure that multi client implementations go easily and also its good for auditors to catch bugs. You can find an example of a similar sort of change here where the L1 attributes was modified for the ecotone hardfork

You can see an example of the SystemConfig setter spec here or here

roberto-bayardo · 2024-08-27T02:43:58Z

@blockchaindevsh I shared the above API discussion & background in the #hf-holocene discord to see if others have strong preferences.

blockchaindevsh · 2024-08-27T07:34:04Z

@tynes @roberto-bayardo The latest commit still keeps changes in the XXXHolocene file(SystemConfigHolocene.sol/L1BlockHolocene.sol), and retains only setFeeScalars for setting fee scalars, the remaining issue to address is: should we pack basefeeScalar/blobBasefeeScalar into a single scalar as per the previous hack?

tynes · 2024-08-27T23:23:47Z

Usually we have this level of conversation at the specs level rather than the implementation so that code review is more checking specs against code.

We would want to get weigh in from other people like @roberto-bayardo @protolambda @sebastianst @geoknee

tynes · 2024-08-28T05:01:11Z

packages/contracts-bedrock/src/L2/L1BlockHolocene.sol

+
+    /// @notice Internal method to decode blobBaseFeeScalar and baseFeeScalar.
+    /// @return Decoded blobBaseFeeScalar and baseFeeScalar.
+    function _decodeScalar(uint256 _scalar) internal pure returns (uint32, uint32) {


I think we will want encode/decode logic in a library so it can be a single source of truth and then imported everywhere that it is required so we only implement the logic once and we can unit test roundtrip

Good point, the packing/unpacking logic is now moved into the library as packScalar/unpackScalar in this commit: e876fe4 . StaticConfig.encodeXXX returns bytes, while we need to maintain the scalar variable as uint256. This is the primary reason it was not encapsulated within the StaticConfig library.

github-actions · 2024-09-12T01:57:14Z

This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days.

contracts part for separating dynamic&static l1 attributes

0e71cf5

blockchaindevsh requested a review from a team as a code owner August 26, 2024 08:02

blockchaindevsh requested a review from maurelian August 26, 2024 08:02

fix Specification_Test

6a54324

roberto-bayardo reviewed Aug 26, 2024

View reviewed changes

blockchaindevsh mentioned this pull request Aug 26, 2024

Potential Research Area: Separate Dynamic and Static L1 Attributes Values ethereum-optimism/specs#122

Open

tynes reviewed Aug 26, 2024

View reviewed changes

tynes mentioned this pull request Aug 27, 2024

experimental: allow the batch inbox to be a contract ethereum-optimism/specs#284

Open

blockchaindevsh mentioned this pull request Aug 27, 2024

EthStorage Devs Meeting #95 Agenda ethstorage/pm#116

Closed

fix for review

4618303

fix Specification_Test

d7e5418

tynes reviewed Aug 28, 2024

View reviewed changes

move packing logic to library

e876fe4

BlocksOnAChain changed the title ~~contracts part for separating dynamic&static l1 attributes~~ Holocene: contracts part for separating dynamic&static l1 attributes Aug 28, 2024

BlocksOnAChain assigned blockchaindevsh Aug 28, 2024

blockchaindevsh mentioned this pull request Aug 31, 2024

QKC/OP Devs Meeting #16 Agenda ethstorage/pm#117

Closed

github-actions bot added the Stale label Sep 12, 2024

tynes closed this Sep 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Holocene: contracts part for separating dynamic&static l1 attributes #11600

Holocene: contracts part for separating dynamic&static l1 attributes #11600

blockchaindevsh commented Aug 26, 2024

blockchaindevsh commented Aug 26, 2024 •

edited

Loading

roberto-bayardo Aug 26, 2024

blockchaindevsh Aug 27, 2024

roberto-bayardo Aug 26, 2024

tynes Aug 26, 2024

roberto-bayardo Aug 27, 2024

roberto-bayardo Aug 27, 2024

roberto-bayardo Aug 26, 2024

roberto-bayardo Aug 26, 2024 •

edited

Loading

roberto-bayardo Aug 27, 2024 •

edited

Loading

roberto-bayardo Aug 26, 2024 •

edited

Loading

roberto-bayardo Aug 26, 2024

roberto-bayardo Aug 26, 2024 •

edited

Loading

blockchaindevsh Aug 27, 2024

roberto-bayardo commented Aug 26, 2024

tynes Aug 26, 2024

roberto-bayardo Aug 27, 2024

blockchaindevsh Aug 27, 2024 •

edited

Loading

tynes Aug 26, 2024

blockchaindevsh Aug 27, 2024

tynes commented Aug 26, 2024

roberto-bayardo commented Aug 27, 2024

blockchaindevsh commented Aug 27, 2024 •

edited

Loading

tynes commented Aug 27, 2024

tynes Aug 28, 2024

blockchaindevsh Aug 28, 2024 •

edited

Loading

github-actions bot commented Sep 12, 2024

Holocene: contracts part for separating dynamic&static l1 attributes #11600

Holocene: contracts part for separating dynamic&static l1 attributes #11600

Conversation

blockchaindevsh commented Aug 26, 2024

blockchaindevsh commented Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roberto-bayardo Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

roberto-bayardo Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

roberto-bayardo Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roberto-bayardo Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roberto-bayardo commented Aug 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blockchaindevsh Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tynes commented Aug 26, 2024

roberto-bayardo commented Aug 27, 2024

blockchaindevsh commented Aug 27, 2024 • edited Loading

tynes commented Aug 27, 2024

Choose a reason for hiding this comment

blockchaindevsh Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Sep 12, 2024

blockchaindevsh commented Aug 26, 2024 •

edited

Loading

roberto-bayardo Aug 26, 2024 •

edited

Loading

roberto-bayardo Aug 27, 2024 •

edited

Loading

roberto-bayardo Aug 26, 2024 •

edited

Loading

roberto-bayardo Aug 26, 2024 •

edited

Loading

blockchaindevsh Aug 27, 2024 •

edited

Loading

blockchaindevsh commented Aug 27, 2024 •

edited

Loading

blockchaindevsh Aug 28, 2024 •

edited

Loading