Skip to content

Conversation

@marioevz
Copy link
Member

@marioevz marioevz commented Dec 21, 2024

🗒️ Description

Adds blob schedule as described in this PR: ethereum/EIPs#9129 and ethereum/EIPs#9240

Transition Tool Interface Changes

  • Adds blobSchedule to the state object passed when using t8n in server mode.
  • The blob schedule is not passed to any tools that only work via command line, as they need to update support first.

E.g.

"state": {
    "fork": "Cancun",
    "chainid": 1,
    "reward": 0,
    "blobSchedule": {
            "Cancun": {
                "target": "0x3",
                "max": "0x6",
                "baseFeeUpdateFraction": "0x32f0ed"
            }
        }
  }

State Test Fixture Format Changes

Adds a config field to the root of the every fixture contain in the state tests JSON fixture files. The object inside the config field contains a blobSchedule field that contains the blob schedule.

{
    "tests/cancun/eip1153_tstore/test_basic_tload.py::test_basic_tload_after_store[fork_Cancun-state_test]": {
        "env": {
            ...
        },
        "pre": {
            ...
        },
        "transaction": {
            ...
        },
        "post": {
            ...
        },
        "config": {
            "blobSchedule": {
                "Cancun": {
                    "max": "0x06",
                    "target": "0x03",
                    "baseFeeUpdateFraction": "0x32f0ed"
                }
            },
        },
        "_info": {
            ...
        }
    }
}

Blockchain Test Fixture Format Changes

Adds a config field to the root of the every fixture contain in the blockchain and blockchain engine tests JSON fixture files.

The object inside the config field contains a blobSchedule field that contains the blob schedule, and also a network field, which is a copy of the network field in the root of the fixture. The network field at the root of the fixture is set to be deprecated.

{
    "tests/cancun/eip1153_tstore/test_basic_tload.py::test_basic_tload_after_store[fork_Cancun-blockchain_test]": {
        "network": "Cancun",
        "genesisBlockHeader": {
            ...
        },
        "pre": {
            ...
        },
        "postState": {
            ...
        },
        "lastblockhash": "...",
        "config": {
            "network": "Cancun",
            "blobSchedule": {
                "Cancun": {
                    "max": "0x06",
                    "target": "0x03",
                    "baseFeeUpdateFraction": "0x32f0ed"
                }
            },
        },
        "genesisRLP": "...",
        "blocks": [
            ...
        ],
        "sealEngine": "NoProof",
        "_info": {
            ...
        }
    }
}

Note: The format of the blobSchedule elements is not exactly as represented in the EIP:

  • The EIP uses integers to represent the values of the numbers, but zero-padded hexadecimal strings are used in the fixtures to maintain the format of the rest of the file.
  • The fork name has the first letter capitalized instead of all-lowercase.

Remove Target Blobs from Block Header

Target blobs fields are completely removed from the header fields throughout the repository, which was only used for EIP-7742.

parent method in Fork

All forks now contain the parent method which returns the parent fork or None for Frontier exclusively.

🔗 Related Issues

✅ Checklist

  • All: Set appropriate labels for the changes.
  • All: Considered squashing commits to improve commit history.
  • All: Added an entry to CHANGELOG.md.
  • All: Considered updating the online docs in the ./docs/ directory.
  • Tests: All converted JSON/YML tests from ethereum/tests have been added to converted-ethereum-tests.txt.
  • Tests: A PR with removal of converted JSON/YML tests from ethereum/tests have been opened.
  • Tests: Included the type and version of evm t8n tool used to locally execute test cases: e.g., ref with commit hash or geth 1.13.1-stable-3f40e65.
  • Tests: Ran mkdocs serve locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.

@marioevz marioevz marked this pull request as draft December 21, 2024 00:48
@marioevz marioevz added scope:evm Scope: evm_transition_tool package scope:forks Scope: Changes ethereum_test_forks package type:chore Type: Chore type:feat type: Feature scope:fw Scope: Framework (evm|tools|forks|pytest) labels Jan 15, 2025
@marioevz marioevz marked this pull request as ready for review January 15, 2025 01:17
@marioevz
Copy link
Member Author

PR is ready for review but all changes need to be documented, and a changelog entry needs to be added.

Copy link
Member

@danceratopz danceratopz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One quick question below.

@danceratopz danceratopz changed the title feat(forks,specs,fixtures): Add Blob Schedule feat(forks,specs,fixtures): add eip-7840 blob schedule config Jan 21, 2025
Copy link
Member

@danceratopz danceratopz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this approach, great to have this config in the fork classes.

I added documentation as a PR to this branch: #1099.

I'm not sure we whether we should start adding new, very specific config objects to the top level of our fixture structures. We could better future-proof this addition by adding a top-level config entry to the JSON fixtures. This would allow us to expand it later if other configs are required.

Suggestion

State Test Fixture Format Changes

Adds blobSchedule to a new top-level config entry in state test JSON fixture files:

{
    "tests/cancun/eip1153_tstore/test_basic_tload.py::test_basic_tload_after_store[fork_Cancun-state_test]": {
        "env": {
            ...
        },
        "pre": {
            ...
        },
        "transaction": {
            ...
        },
        "post": {
            ...
        },
        "config": {
            "blobSchedule": {
                "cancun": {
                    "max": "0x06",
                    "target": "0x03",
                    "baseFeeUpdateFraction": "0x32f0ed"
                },
            },
        },
        "_info": {
            ...
        }
    }
}

Blockchain Test Fixture Format Changes

  • The blobSchedule structure and network are added to the new field top-level config entry in the blockchain fixture files.
  • The top-level network field is left as-is, to be potentially deprecated in the future.
{
    "tests/cancun/eip1153_tstore/test_basic_tload.py::test_basic_tload_after_store[fork_Cancun-blockchain_test]": {
        "network": "Cancun",
        "genesisBlockHeader": {
            ...
        },
        "pre": {
            ...
        },
        "postState": {
            ...
        },
        "lastblockhash": "...",
        "config": {
            "blobSchedule": {
                "cancun": {
                    "max": "0x06",
                    "target": "0x03",
                    "baseFeeUpdateFraction": "0x32f0ed"
                },
                "network": "Cancun",
            },
        },
        "genesisRLP": "...",
        "blocks": [
            ...
        ],
        "sealEngine": "NoProof",
        "_info": {
            ...
        }
    }
}

Just an idea, let me know what you think. cc @winsvega

@chfast
Copy link
Member

chfast commented Jan 21, 2025

I'm not sure I'm able to grasp every information at once, but my generic comments from the consumer POV:

  1. Reuse sub-objects between different formats (statetest, blockchaintest, t8n) if possible.
  2. What works best for "structured JSON parsers" is that we have agreed default values and ability to overwrite them in tests. E.g. omitting a key should not mean something different than having the key with default value.
  3. I'm quite confused why "cancun" is even here and repeated twice. For me the ability to overwrite by-the-spec values is wrong because I usually don't give such ability in API. E.g. if the test says blobSchedule.cancun.max = 7 should I reject/skip/fail the test?

@marioevz
Copy link
Member Author

3. I'm quite confused why "cancun" is even here and repeated twice. For me the ability to overwrite by-the-spec values is wrong because I usually don't give such ability in API. E.g. if the test says blobSchedule.cancun.max = 7 should I reject/skip/fail the test?

I think it would be nice to have input from other clients with regards what we want to see in this object.

Particularly, are we ok with this field containing different values than mainnet, and then producing a test that verifies these value deviations somehow during the test? Like setting a max blobs to 100 and then verifying that a transaction with these amount of blobs is correctly accepted.

As to why is Cancun duplicated, we have the value contained in network, which can be Cancun, Prague or CancunToPragueAtTime15k, and for example for CancunToPragueAtTime15k we require cancun and prague inside of the blob schedule because Cancun's target and max will be applied before the block with 15k timestamp, and Prague's values will be applied afterwards. I hope this made sense but I agree it's confusing, so I can also add this description to the docs.

Copy link
Member

@danceratopz danceratopz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for adding the config field. One minor nit about consistency (I'd prefer to use the fork name capitalized consistently in the JSON) but I can live with it as-is, if you prefer.

@chfast
Copy link
Member

chfast commented Jan 21, 2025

If you plan to go with it, I think the nesting is backward. I'd imagine it to be organized as:

"config": {
            "cancun": {
                "blobSchedule": {
                    "max": "0x06",
                    "target": "0x03",
                    "baseFeeUpdateFraction": "0x32f0ed"
                },
                "network": "Cancun",
            },
        },

And then you can skip unnecessary grouping. Grouping may look nice, but it rather makes consumers more complex.

"config": {
            "cancun": {
                "maxBlobs": "0x06",
                "targetBlobs": "0x03",
                "blobBaseFeeUpdateFraction": "0x32f0ed",
                "network": "Cancun",
            },
        },

@danceratopz
Copy link
Member

And then you can skip unnecessary grouping. Grouping may look nice, but it rather makes consumers more complex.

"config": {
            "cancun": {
                "maxBlobs": "0x06",
                "targetBlobs": "0x03",
                "blobBaseFeeUpdateFraction": "0x32f0ed",
                "network": "Cancun",
            },
        },

Appreciate the input @chfast.

I appreciate aspiring to avoiding unnecessary nesting, but I don't think we can nest as you propose as the following example demonstrates.

Here's the current proposal for a config section for a transition fork test from Cancun to Prague (it would need #1082 to generate this transition). This is closer to the shape proposed in the EIP.

        "config": {
            "network": "CanunToPragueAtTime15k",
            "blobSchedule": {
                "Cancun": {
                    "target": "0x03",
                    "max": "0x06",
                    "baseFeeUpdateFraction": "0x32f0ed"
                },
                "Prague": {
                    "target": "0x06",
                    "max": "0x09",
                    "baseFeeUpdateFraction": "0x4c6964"
                }
            }
        }

To reduce nesting, how about this?

        "config": {
            "network": "CanunToPragueAtTime15k",
            "Cancun": {
                "targetBlobs": "0x03",
                "maxBlobs": "0x06",
                "blobBaseFeeUpdateFraction": "0x32f0ed"
            },
            "Prague": {
                "targetBlobs": "0x06",
                "maxBlobs": "0x09",
                "blobBaseFeeUpdateFraction": "0x4c6964"
            }
        }

cc @jochem-brouwer

@winsvega
Copy link
Contributor

winsvega commented Jan 22, 2025

for normal forks

        "config": {
            "network": "fork",
            "blobSchedule": {
                    "target": "0x03",
                    "max": "0x06",
                    "baseFeeUpdateFraction": "0x32f0ed"
            }
        }

for transition forks

        "config": {
            "network": "forkAToforkB",
            "forkA" : {
                "blobSchedule": {
                        "target": "0x03",
                        "max": "0x06",
                        "baseFeeUpdateFraction": "0x32f0ed"
                }
            },
           "forkB" : {
                "blobSchedule": {
                        "target": "0x03",
                        "max": "0x06",
                        "baseFeeUpdateFraction": "0x32f0ed"
                }
            }
        }

the idea here that actual configs are under forkA and forkB in transition tests. because of 2 forks involved.
for majority of the tests there is only 1 config.

@chfast
Copy link
Member

chfast commented Jan 22, 2025

I take it back. I just now noticed that "blobSchedule" is coming from EIP-7840 so if the EIP is accepted we should use it directly, by my rule number 1 :)

  1. Reuse sub-objects between different formats (statetest, blockchaintest, t8n) if possible.

I just don't understand what is "baseFeeUpdateFraction" for.

@jochem-brouwer
Copy link
Member

baseFeeUpdateFraction is in this case the 4844 update fraction of the blob gas calculation, in the 4844 EIP it is the constant BLOB_BASE_FEE_UPDATE_FRACTION @chfast

@danceratopz
Copy link
Member

I take it back. I just now noticed that "blobSchedule" is coming from EIP-7840 so if the EIP is accepted we should use it directly, by my rule number 1 :)

Ok! 🙂

  1. Reuse sub-objects between different formats (statetest, blockchaintest, t8n) if possible.

👍 let me check what the difference is to the format used in t8n.

I just don't understand what is "baseFeeUpdateFraction" for.

JICYMI, this is still open, but seems widely agreed upon: ethereum/EIPs#9240

From the (unmerged) rationale:

Additionally, the baseFeeUpdateFraction parameter was added to adjust the responsiveness of blob gas pricing per fork.

@winsvega
Copy link
Contributor

or how about this one

       "network" : "forkAtoforkb",
        "config": [
           {
               "network": "forkA",
               "blobSchedule": {
                       "target": "0x03",
                       "max": "0x06",
                       "baseFeeUpdateFraction": "0x32f0ed"
               }
          },
          {
               "network": "forkB",
               "blobSchedule": {
                       "target": "0x03",
                       "max": "0x06",
                       "baseFeeUpdateFraction": "0x32f0ed"
               }
          }
        ]

@jochem-brouwer
Copy link
Member

For the config, I would take it directly from EIP-7840, otherwise this config would override the EIP. (Or change the EIP)

@danceratopz
Copy link
Member

@marioevz @winsvega: As it stands, listening to @chfast:

I take it back. I just now noticed that "blobSchedule" is coming from EIP-7840 so if the EIP is accepted we should use it directly, by my rule number 1 :)

and @jochem-brouwer (thanks for the input!):

For the config, I would take it directly from EIP-7840, otherwise this config would override the EIP. (Or change the EIP)

... the format below is the current agreement on the fixture change. This is the current state of this PR (as of now, 6d05af4).

From the comment above:

Here's the current proposal for a config section for a transition fork test from Cancun to Prague (it would need #1082 to generate this transition). This is closer to the shape proposed in the EIP.

        "config": {
            "network": "CanunToPragueAtTime15k",
            "blobSchedule": {
                "Cancun": {
                    "target": "0x03",
                    "max": "0x06",
                    "baseFeeUpdateFraction": "0x32f0ed"
                },
                "Prague": {
                    "target": "0x06",
                    "max": "0x09",
                    "baseFeeUpdateFraction": "0x4c6964"
                }
            }
        }

If there are no objections, I think we can merge this PR as-is.

@winsvega
Copy link
Contributor

winsvega commented Jan 22, 2025

only one concern. if you get more then one section.
if there would be blobSchedule2, then you overcopy forks for each section in the config.

so for normal fork it is

        "config": {
            "network": "Cancun",
            "blobSchedule": {
                "Cancun": {
                    "target": "0x03",
                    "max": "0x06",
                    "baseFeeUpdateFraction": "0x32f0ed"
                }
            }
        }

?

@marioevz
Copy link
Member Author

only one concern. if you get more then one section. if there would be blobSchedule2, then you overcopy forks for each section in the config.

I don't think I can see a blobSchedule2 being needed, do you mean:

  1. "Another fork is added": It would be just appended to the current object, e.g. "Osaka" would simply be a new field in the dictionary.
  2. or "A new field is added to the blob schedule": Unless the field is not backwards compatible, which I don't think will happen, we can just append the new field to the current blobSchedule and should not be an issue.

Either way I don't see why we shouldn't be including fields we need now over potential future fields that would incompatible IMO.

@marioevz
Copy link
Member Author

I'll clean up the commits and then merge, because I'd like to preserve the commit history due to the amount and diversity of changes.

@winsvega
Copy link
Contributor

winsvega commented Jan 22, 2025

I meant what if there would be additional another config besides blobschedule in the future

Something like gastargetschedule

@marioevz
Copy link
Member Author

I meant what if there would be additional another config besides blobschedule in the future

Something like gastargetschedule

Got it, yes I think something like this would fit nicely within the new config object, just adding every new config property as a new field.

@lightclient
Copy link
Member

FWIW I feel like we should be moving away from hexadecimal representation where it makes sense and we're confident the number can be represented natively in js -- so much easier to read and reason about decimal representation.

@marioevz marioevz merged commit b4f9b16 into main Jan 22, 2025
21 checks passed
@marioevz marioevz deleted the eip-blob-schedule branch January 22, 2025 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scope:evm Scope: evm_transition_tool package scope:forks Scope: Changes ethereum_test_forks package scope:fw Scope: Framework (evm|tools|forks|pytest) type:chore Type: Chore type:feat type: Feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants