Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
t8n: verkle genesis #466
t8n: verkle genesis #466
Changes from all commits
1251a25
cb40b1b
8ff9410
3edbc5b
a3a612c
cf22fe9
10967b8
679b9b1
bd6b3b6
33ba03a
d186f34
a9ee2e2
3433b45
90db7db
308e802
47addd7
93ad9d7
660e2eb
6dcdaeb
8f41873
72b03c4
1f72eda
ff82721
c08f8c1
f3ff46e
b640d33
a324f59
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Each run uploads the generated fixtures as artifacts. For example, see the bottom of this run.
As we chatted in TG, after everything is stabilized we should probably:
In any case, we can chat when we get a stable set of fixtures and we can change a bit the CI jobs.
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.
With this line we generate all "targetable" tests on a Verkle fork, that is, using a VKT for the tree which includes:
These means around 458 filled test-vectors using verkle trees.
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.
Here the single state-conversion test vector is filled since we target
EIP6800Transition
. But also we try to fill all existing tests from Shanghai forward; this isn't strictly useful for "Verkle needs" but we're just making sure filling for previous forks keeps working (and in fact, I had to fix things in t8n to make this work again).This fills ~605 tests. Most of them aren't entirely useful but only the state-conversion one. The usefulness of consuming those fixtures, is making sure the EL client can keep passing tests for previous forks (e.g: full sync isn't broken).
The more test the better, but probably the other 458 fixtures are much more important to catch Verkle bugs for devnet7.
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.
Consume all fixtures. As I mentioned in other place, I found a problem in
consume direct
which doesn't return !=0 if any test fails, so the CI doesn't "fail" of some test fails.That's quite bad since that's the hole point of the CI, but Spencer will work on that to fix it. Until that happens, it might be good to "eyeball" the CI run logs and see all passed.
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.
This is something I discovered while trying to fix running the fixture for the state-conversion. This boolean indicates if the run should enable snapshot, which we need for state-conversion (since we iterate on it).
This will also be true for verkle-genesis tests (since this
true
isn't conditional). Verkle-genesis test doesn't really need snapshot, but I preferred not to add any extra logic here to make it conditional since that would require a deeper refactor. And also, enabling snapshots for verkle-genesis should still be valid.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.
Added comment on why we removed the default.
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.
Moving this from inside the
if
is required, when I was trying to make also the fixture consumption to work for forks previous to Verkle and the transition. (e.g: Shanghai).The problem was we didn't have the
else
statement I added in L556. Which meant that we didn't return a correctstatedb
for that case and nothing worked.To avoid code repetition, both for the
if
andelse
case we need to execute this logic thus I moved it out.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.
In the case of a non-verkle (nor state-conversion) fork, we simply re-open the MPT with the
mptRoot
calculated before. This fixes consuming fixtures for those.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 expect some pushback on the geth side, but we'll see. I'd prefer to be "agnostic" but they might prefer it to be obvious.
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.
This is a new CLI tool to help the testing framework deal with a situation I found when fixing test filling.
The testing-library uses their own code for generating the genesis block. They only have an MPT implementation, so the genesis block was always generated with a MPT root. This is wrong for verkle-genesis tests.
Until they have a pure Python impl of a Verkle Tree and its cryptography, I offered them a new CLI that can calculate this root for them in the case we're in verkle mode. Whenever there's a execution-spec codebase implemented in Python for Verkle, they can copy-paste that implementation and come back generating the genesis vector on their side (and we can remove this CLI helper). For now, this should work and unblocked us from the problem.