-
Notifications
You must be signed in to change notification settings - Fork 187
feat(forks,specs,fixtures): add eip-7840 blob schedule config #1040
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
Conversation
1f96c64 to
c73f685
Compare
|
PR is ready for review but all changes need to be documented, and a changelog entry needs to be added. |
danceratopz
left a comment
There was a problem hiding this 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.
src/ethereum_test_specs/tests/fixtures/chainid_cancun_blockchain_test_engine.json
Outdated
Show resolved
Hide resolved
src/ethereum_test_specs/tests/fixtures/chainid_cancun_state_test.json
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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
blobSchedulestructure andnetworkare added to the new field top-levelconfigentry in the blockchain fixture files. - The top-level
networkfield 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
|
I'm not sure I'm able to grasp every information at once, but my generic comments from the consumer POV:
|
8c91ef8 to
d4b5894
Compare
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 |
danceratopz
left a comment
There was a problem hiding this 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.
|
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",
},
}, |
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": {
"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"
}
} |
|
for normal forks for transition forks the idea here that actual configs are under forkA and forkB in transition tests. because of 2 forks involved. |
|
I take it back. I just now noticed that
I just don't understand what is |
|
|
Ok! 🙂
👍 let me check what the difference is to the format used in t8n.
JICYMI, this is still open, but seems widely agreed upon: ethereum/EIPs#9240 From the (unmerged) rationale:
|
|
or how about this one |
|
For the config, I would take it directly from EIP-7840, otherwise this config would override the EIP. (Or change the EIP) |
|
@marioevz @winsvega: As it stands, listening to @chfast:
and @jochem-brouwer (thanks for the input!):
... 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:
If there are no objections, I think we can merge this PR as-is. |
|
only one concern. if you get more then one section. so for normal fork it is ? |
I don't think I can see a
Either way I don't see why we shouldn't be including fields we need now over potential future fields that would incompatible IMO. |
|
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. |
|
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 |
|
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. |
Co-authored-by: danceratopz <[email protected]>
6d05af4 to
f42e224
Compare
🗒️ Description
Adds blob schedule as described in this PR: ethereum/EIPs#9129 and ethereum/EIPs#9240
Transition Tool Interface Changes
blobScheduleto thestateobject passed when using t8n in server mode.E.g.
State Test Fixture Format Changes
Adds a
configfield to the root of the every fixture contain in the state tests JSON fixture files. The object inside theconfigfield contains ablobSchedulefield 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
configfield to the root of the every fixture contain in the blockchain and blockchain engine tests JSON fixture files.The object inside the
configfield contains ablobSchedulefield that contains the blob schedule, and also anetworkfield, which is a copy of thenetworkfield in the root of the fixture. Thenetworkfield 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
blobScheduleelements is not exactly as represented in the EIP: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.
parentmethod inForkAll forks now contain the
parentmethod which returns the parent fork orNonefor Frontier exclusively.🔗 Related Issues
✅ Checklist
mkdocs servelocally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.