update L1Block contract with new fields for 4844 #5

roberto-bayardo · 2023-12-12T00:20:25Z

No description provided.

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

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

mds1 · 2023-12-14T17:31:18Z

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

-    ///         roots to L1. Adds 68 bytes of padding to account for the fact that the input does
-    ///         not have a signature.
+    /// @notice Computes the amount of L1 gas used for a transaction. Adds 68 bytes
+    ///         of padding to account for the fact that the input does not have a signature.
    /// @param _data Unsigned fully RLP-encoded transaction to get the L1 gas for.
    /// @return Amount of L1 gas used to publish the transaction.
    function getL1GasUsed(bytes memory _data) public view returns (uint256) {


Looks like this can be pure

mds1 · 2023-12-14T17:32:06Z

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

+        uint256 compressedTxSize = total / 16;
+        return compressedTxSize + (68 * 16);


We can remove some precision error here by moving division to the end, the current version will slightly underestimate due to flooring when computing total / 16

Suggested change

uint256 compressedTxSize = total / 16;

return compressedTxSize + (68 * 16);

return (total + (68 * 16 * 16)) / 16;

@mds1 do you mean return (total + (68 * 16)) / 16; ?

I think the original suggestion was correct, here's my work:

gasUsed = compressedTxSize + (68 * 16) gasUsed = (total / 16) + (68 * 16) gasUsed * 16 = total + (68 * 16 * 16) gasUsed = (total + (68 * 16 * 16)) / 16

Oh I see, I was missing a set of parentheses, just edited it 😅

ok yes makes sense!

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

anikaraghu · 2023-12-15T23:39:23Z

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

    uint256 public l1FeeOverhead;

    /// @notice The scalar value applied to the L1 portion of the transaction fee.
+    /// @custom:legacy
    uint256 public l1FeeScalar;


would be nice to make l1FeeOverhead and l1FeeScalar internal so that people don't call them, but then we can't use them in the GPO contract (which we need for backwards compatibility)

If we really wanted that we could revert on msg.sender but then people can spoof with eth_call anyways

tynes · 2023-12-16T00:04:07Z

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

+            msg.sender == L1Block(Predeploys.L1_BLOCK_ATTRIBUTES).DEPOSITOR_ACCOUNT(),
+            "GasPriceOracle: only the depositor account can set isEclipse flag"
+        );
+        isEclipse = _isEclipse;


Perhaps only allow for setting it to true because there isn't really a concept of turning a hardfork off

tynes · 2023-12-16T00:04:38Z

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

    /// @notice Retrieves the current fee overhead.
    /// @return Current fee overhead.
-    function overhead() public view returns (uint256) {
-        return L1Block(Predeploys.L1_BLOCK_ATTRIBUTES).l1FeeOverhead();
+    function overhead() public pure returns (uint256) {


We would still want these functions to operate in the non eclipse mode

ah yes fair

tynes · 2023-12-16T00:05:22Z

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

-        uint256 l1GasUsed = getL1GasUsed(_data);
-        uint256 l1Fee = l1GasUsed * l1BaseFee();
+        uint256 unscaled;
+        if (isEclipse) {


How would you feel about breaking this up into 2 internal functions so each branch could be easily observed individually?

sounds reasonable to me

tynes · 2023-12-16T00:07:39Z

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

@@ -83,7 +122,7 @@ contract GasPriceOracle is ISemver {
    ///         not have a signature.
    /// @param _data Unsigned fully RLP-encoded transaction to get the L1 gas for.
    /// @return Amount of L1 gas used to publish the transaction.
-    function getL1GasUsed(bytes memory _data) public view returns (uint256) {
+    function _getL1GasUsedPreEclipse(bytes memory _data) internal view returns (uint256) {


Is this used? I don't see branching logic in getL1GasUsed

Also the name is very verbose but we don't want to break the public api

oops yes I will just do the branching logic in the original function, got confused for a sec

tynes · 2023-12-16T00:09:16Z

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

+
+        (uint64 _number, uint64 _timestamp, uint256 _basefee, uint256 _blobBaseFee, bytes32 _hash,
+        uint64 _sequenceNumber, bytes32 _batcherHash, uint32 _baseFeeScalar, uint32 _blobBaseFeeScalar) =
+            abi.decode(msg.data[4:], (uint64, uint64, uint256, uint256, bytes32, uint64, bytes32, uint32, uint32));


Are you sure this will tightly pack things?

no not 100% sure, definitely need to test this 😅 and also add some validations probably. @tynes did you have a different method for parsing the msg.data in mind though?

ah I see, I can manually check the bits Im interested in

[edit] yes I believe abi.encode/decode assumes 32 bytes for each fixed-width parameter regardless of type. there's abi.encodePacked but there's no corresponding decoder. I think we will want to just assume each is fixed-width encoded big endian? Looks like this would be straightforward to decode in Solidity (see "Integer types" section on page 66 of https://buildmedia.readthedocs.org/media/pdf/solidity/v0.5.11/solidity.pdf)

I've documented the layout in a spec update PR here: https://github.com/roberto-bayardo/optimism/pull/6/files

tynes · 2023-12-19T03:25:53Z

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

+        if (isEclipse) {
+            return _getL1GasUsed(_data);
+        }
+        return _getL1GasUsedPreEclipse(_data);


nit: _getL1GasUsedBedrock and _getL1GasUsedEclipse. Will make future extensions easier

tynes · 2023-12-19T03:27:55Z

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

@@ -72,5 +84,66 @@ contract L1Block is ISemver {
        batcherHash = _batcherHash;
        l1FeeOverhead = _l1FeeOverhead;
        l1FeeScalar = _l1FeeScalar;
+
+        (uint256 _blobBaseFee) = abi.decode(msg.data[260:], (uint256));


This function should not change

oops sorry was just testing somethig

tynes · 2023-12-19T03:29:14Z

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

+        uint32 _blobBaseFeeScalar;
+
+        assembly {
+            _number := mload(add(_data, 0x8))


chatgpt should be able to autogen this code with a good prompt

tynes · 2023-12-19T03:31:42Z

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

+        require(msg.sender == DEPOSITOR_ACCOUNT, "L1Block: only the depositor account can set L1 block values");
+
+        bytes memory _data = msg.data[4:];
+        // if (_msgData.length != XXX) { // TODO: configure


We should omit this revert check to save gas. The more gas we can save here, the more scalable the system is because it saves more gas for user transactions. We should be able to do everything using calldataload instead of msg.data. We get good test coverage through differential testing against the go code

tynes · 2023-12-19T03:43:21Z

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

+
+    /// @notice Updates the L1 block values for a post-blob activated chain.
+    /// Params are passed in as part of msg.data in order to compress the calldata.
+    /// Params should be passed in in the following order:


These should be in order so they are 32 byte aligned so that it is easy to use calldataload to pop all of the values to the stack

tynes · 2023-12-19T03:44:00Z

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

        uint256 divisor = 10 ** DECIMALS;
-        uint256 unscaled = l1Fee * scalar();
        uint256 scaled = unscaled / divisor;
        return scaled;
    }



this needs natspec

tynes · 2023-12-19T03:44:48Z

Thoughts on breaking out the smart contract changes solely in this PR?

Original rebased prototype by proto, plus changes from Roberto: - encapsulate data <-> blob conversion code, add unit tests - update 4844 code to be compatible with latest beacon node api - remove stray file, include one more invalid blob test - appropriate fee bumping & blob fee estimation for blob transactions - misc other improvements

…dBatchTx code

tynes · 2023-12-20T21:31:26Z

packages/contracts-bedrock/test/L2/GasPriceOracle.t.sol

+contract GasPriceOracleBedrock_Test is GasPriceOracle_Test {
+    /// @dev Sets up the test suite.
+    function setUp() public virtual override {
+        super.setUp();
        depositor = l1Block.DEPOSITOR_ACCOUNT();


we can delete this assignment of depositor since now it happens in super.setUp

tynes · 2023-12-20T21:32:25Z

packages/contracts-bedrock/test/L2/GasPriceOracle.t.sol

+        bytes4 functionSignature = bytes4(keccak256("setL1BlockValuesV2()"));
+
+        // Encode the function signature and extra data
+        bytes memory callDataPacked = abi.encodePacked(


You technically can just add the function signature in as the first arg here and not need to calls to abi.encodePacked

tynes · 2023-12-20T21:32:40Z

packages/contracts-bedrock/test/L2/GasPriceOracle.t.sol

+        super.setUp();
+
+        // Define the function signature
+        bytes4 functionSignature = bytes4(keccak256("setL1BlockValuesV2()"));


We agreed on updating this to setL1BlockValuesEcotone

tynes · 2023-12-20T21:34:35Z

packages/contracts-bedrock/test/L2/GasPriceOracle.t.sol

@@ -93,3 +102,90 @@ contract GasPriceOracle_Test is CommonTest {
        assertEq(returndata, hex"");
    }
 }
+


I would recommend making a GasPriceOracleEclipseTransition_Test contract and explicitly test the transition. Then in GasPriceOracleEclipse_Test I would recommend using vm.store to set isEclipse to true directly via storage and then test the functionality that way. Also it should be isEcotone now

This will make the hardfork activation testing explicit and not part of setup and decouple the tests cleanly from implementation and transition

tynes · 2023-12-20T21:37:37Z

packages/contracts-bedrock/test/L2/L1Block.t.sol

+    function setUp() public virtual override {
+        super.setUp();
+
+        bytes4 functionSignature = bytes4(keccak256("setL1BlockValuesV2()"));


We should consider writing a simple library that can be imported that can abi encode the data properly and used in many locations. It would accept all of the args and return bytes with the function selector prefixed to the fixed size encodings of each arg

We would basically be able to delete the setup function and instead only fuzz

tynes · 2023-12-20T21:39:38Z

packages/contracts-bedrock/test/L2/L1Block.t.sol

+        (bool success, ) = address(l1Block).call(functionCallDataPacked);
+        require(success, "Function call failed");
+    }
+


A fuzz test here would be useful. To be honest I think only having a fuzz test is much more useful than having tests that assert against "magic" values that are set in setup. I know you are following precedent, curious what you think

tynes · 2023-12-20T21:40:37Z

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

+    string public constant version = "1.2.0";
+
+    /// @notice Flag that indicates whether the network is in eclipse mode.
+    bool public isEclipse;


This should be isEcotone

mds1 · 2023-12-20T23:01:00Z

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

+        assembly {
+            let offset := 0x4
+            _number := shr(192, calldataload(offset)) // uint64


Is dropping into assembly and using calldataload cheaper than saying in solidity and indexing into msg.data? The latter would be more readable, but assembly is ok if it provides gas savings that we need

roberto-bayardo force-pushed the 4844/contracts branch from 94f825c to 1a917ae Compare December 12, 2023 00:24

tynes reviewed Dec 14, 2023

View reviewed changes

packages/contracts-bedrock/src/L2/GasPriceOracle.sol Show resolved Hide resolved

tynes reviewed Dec 14, 2023

View reviewed changes

packages/contracts-bedrock/src/L2/L1Block.sol Show resolved Hide resolved

mds1 reviewed Dec 14, 2023

View reviewed changes

roberto-bayardo force-pushed the 4844/rebase branch from f5a3456 to a7024d6 Compare December 14, 2023 21:23

anikaraghu changed the base branch from 4844/rebase to develop December 15, 2023 01:22

anikaraghu changed the base branch from develop to 4844/rebase December 15, 2023 01:24

anikaraghu force-pushed the 4844/contracts branch from 96728f8 to 6e4e12c Compare December 15, 2023 01:29

mdehoog reviewed Dec 15, 2023

View reviewed changes

packages/contracts-bedrock/src/L2/GasPriceOracle.sol Show resolved Hide resolved

anikaraghu reviewed Dec 15, 2023

View reviewed changes

tynes reviewed Dec 16, 2023

View reviewed changes

roberto-bayardo force-pushed the 4844/rebase branch 2 times, most recently from a370485 to 6bb91fa Compare December 18, 2023 16:12

tynes reviewed Dec 19, 2023

View reviewed changes

roberto-bayardo force-pushed the 4844/rebase branch from 6bb91fa to b96fc11 Compare December 19, 2023 16:27

anikaraghu force-pushed the 4844/contracts branch from 181663b to 9cf46ce Compare December 19, 2023 19:00

roberto-bayardo force-pushed the 4844/rebase branch from b96fc11 to 3779153 Compare December 19, 2023 20:16

protolambda and others added 2 commits December 19, 2023 15:33

disallow blob calldata from being used for batches, factor out isVali…

899346c

…dBatchTx code

roberto-bayardo force-pushed the 4844/rebase branch from 3779153 to 899346c Compare December 19, 2023 23:33

roberto-bayardo and others added 4 commits December 19, 2023 15:48

Eclipse -> Ecotone

2a262b1

update L1Block contract with new fields for 4844

5ab7c46

Update pricing function

f51594f

Comments, untested

d7ea078

anikaraghu added 3 commits December 19, 2023 17:16

WIP packing logic

91edb9b

Add tests, not yet passing

62aac26

Tests passing

07278b7

anikaraghu force-pushed the 4844/contracts branch from 9cf46ce to 07278b7 Compare December 20, 2023 01:16

tynes reviewed Dec 20, 2023

View reviewed changes

mds1 reviewed Dec 20, 2023

View reviewed changes

roberto-bayardo force-pushed the 4844/rebase branch 4 times, most recently from 536034a to afc8480 Compare December 28, 2023 00:45

		uint256 compressedTxSize = total / 16;
		return compressedTxSize + (68 * 16);

	uint256 compressedTxSize = total / 16;
	return compressedTxSize + (68 * 16);
	return (total + (68 * 16 * 16)) / 16;

update L1Block contract with new fields for 4844 #5

Are you sure you want to change the base?

update L1Block contract with new fields for 4844 #5

Conversation

roberto-bayardo commented Dec 12, 2023

Choose a reason for hiding this comment

mds1 Dec 14, 2023 • 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

anikaraghu Dec 15, 2023 • 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

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 Dec 17, 2023 • edited Loading

Choose a reason for hiding this comment

roberto-bayardo Dec 19, 2023 • 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

Choose a reason for hiding this comment

tynes commented Dec 19, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mds1 Dec 14, 2023 •

edited

Loading

anikaraghu Dec 15, 2023 •

edited

Loading

roberto-bayardo Dec 17, 2023 •

edited

Loading

roberto-bayardo Dec 19, 2023 •

edited

Loading