-
Notifications
You must be signed in to change notification settings - Fork 331
tests: Support EIP-7892 - BPO forks #1330
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
|
||
namespace evmone::state | ||
{ | ||
BlobParams get_blob_params(evmc_revision rev) |
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.
NOTE: this used to be constexpr
but it cannot be anymore, since I've decided to overload get_blob_params
. I don't think the constexprness has been leveraged anywhere, and now it also throws so probably doesn't even make sense to be constexpr.. But yea, lmk if this a problem
b57b3f9
to
9cb611b
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1330 +/- ##
==========================================
- Coverage 87.25% 87.24% -0.01%
==========================================
Files 169 169
Lines 24864 24970 +106
Branches 4081 4106 +25
==========================================
+ Hits 21695 21786 +91
- Misses 521 529 +8
- Partials 2648 2655 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
13a0b8d
to
408b15a
Compare
This is the last thing to tackle here I guess, but otherwise ready for review EDIT: of course, the ignore is for the EEST job only, that's why |
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 think the general direction is good: don't hardcode the blob schedule in EVM.
return {6, 9, 5007716}; | ||
return {3, 6, 3338477}; | ||
} | ||
using BlobSchedule = std::unordered_map<std::string, state::BlobParams>; |
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.
Focusing on the interface for EVM here, I think the mapping should be rather timestamp => BlobParams
so this is independent of the revision
and the network name.
So we should use something like flat_map
to represent BlobSchedule
.
Getting the BlobSchedule
by name or from JSON should be the job for the EVM user. I imagine this will be hardcoded for Mainnet and constructed from JSON for tests.
I'm not sure how much refactoring this is, so we can leave it for some other task. Maybe when we will be putting better boundary between test and consensus code.
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.
Now I realized that passing BlobParams
instead of BlobSchedule
is more in-line with passing revision
.
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.
Focusing on the interface for EVM here, I think the mapping should be rather
timestamp => BlobParams
so this is independent of therevision
and the network name.
So I thought about this today. What troubles me, is whether or not we'd still want to fetch BlobParams
by fork explicitly, bypassing timestamp. More generally, I'm not 100% sure about the boundary between EEST and evmone here - in the contexts of state/blockchaintests, t8n, and potentially the future Engine API test consumption.
I think it's ok to merge as is to tick off coverage work, but I can try the refactor afterwards and we can decide the API considerations with the testing team.
Now I realized that passing
BlobParams
instead ofBlobSchedule
is more in-line with passingrevision
.
Not sure what do you mean? passing where?
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 meant the same as your concern.
805042d
to
a5bb986
Compare
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.
You can rework commits and we can merge this.
e2e6150
to
f88e644
Compare
@chfast I've tacked on a commit implementing the change we've discussed (using 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.
Make sure the final commit message will be up to date.
https://eips.ethereum.org/EIPS/eip-7892
Closes #1325 and follows the proposal there.
Blob parameter only forks. This ended up a considerable revamp, as we've moved on from hardcoded blob schedules to ones coming from JSON config (post Osaka). Because of this, a bunch of internal APIs needed to be shuffled, I hope this all still makes sense.
To ease review, the "main" commit follows the old naming convention, which seems to have bin a misnomer - it took a single entry of a blob schedule for the blob schedule. The two follow-up commits, to be squashed away before merging, clean this up.
But, this PR should also be reviewable as a whole, just need to follow the new naming.
Filling with evmone-t8n requires an update to EEST: ethereum/execution-spec-tests#2264