Gloas alpha spec 12 changes - #9596
Conversation
macladson
left a comment
There was a problem hiding this comment.
Just noting that we will need to re-enable the rest of the failing alpha 12 tests which I disabled in #9450
lighthouse/testing/ef_tests/check_all_files_accessed.py
Lines 65 to 70 in e58ec88
|
This pull request has merge conflicts. Could you please resolve them @eserilev? 🙏 |
pawanjay176
left a comment
There was a problem hiding this comment.
LGTM other than the partial column size enforcement.
|
Queued — the merge queue status continues in this comment ↓. |
chong-he
left a comment
There was a problem hiding this comment.
Looks great, just a few small comments
| #[test] | ||
| fn test_payload_withdrawals_over_limit() { |
There was a problem hiding this comment.
There are five types of execution requests and we have two tests checking the execution requests - max_withdrawals_per_payload and max_builder_exit_requests_per_payload.
I understand that the tests are repetitive and so that's probably why you didn't include a test for the other 3 types of requests. So I will leave it to you whether to add them or not.
| /* | ||
| * This test ensures that: | ||
| * | ||
| * We do not accept gloas blocks whose progressive operation lists exceed their spec limits. |
There was a problem hiding this comment.
Maybe we can be a bit more specific here? If I understand correctly this is about for Gloas, there can be no more Deposit accepted in a block. A suggestion as below, but feel free to modify as you see fit
| * We do not accept gloas blocks whose progressive operation lists exceed their spec limits. | |
| * We do not accept gloas blocks whose progressive operation lists exceed their spec limits (in this case, the `Deposit` should always be empty) |
| # 2**6 (= 64) epochs | ||
| MIN_BUILDER_WITHDRAWABILITY_DELAY: 64 |
There was a problem hiding this comment.
I see that the spec has this field:
# 5000 basis points, 50% of SLOT_DURATION_MS
PAYLOAD_DUE_BPS: 5000
Do we need to add it here too? (also the minimal.yaml)
The same goes for the file: common/eth2_network_config/built_in_network_configs/mainnet/config.yaml
Merge Queue Status
This pull request spent 1 minute 10 seconds in the queue, with no time running CI. ReasonThe pull request #9596 has been manually updated Requeued — the merge queue status continues in this comment ↓. |
Merge Queue Status
This pull request spent 20 minutes 28 seconds in the queue, including 18 minutes 32 seconds running CI. Waiting for
All conditions
ReasonThe merge conditions cannot be satisfied due to failing checks
HintYou may have to fix your CI before adding the pull request to the queue again. Requeued — the merge queue status continues in this comment ↓. |
|
Some required checks have failed. Could you please take a look @eserilev? 🙏 |
|
@mergify requeue |
Merge Queue Status
This pull request spent 28 minutes 50 seconds in the queue, including 27 minutes 23 seconds running CI. Required conditions to merge
|
Issue Addressed
All the changes required for gloas alpha spec 12 minus stable containers. Note that EF tests cannot pass without the stable container changes. For example finality tests fail w/ ParentBlockRootMismatch because progressive merkelization isn't implemented.
So I think the plan should be to merge progressive containers, without enabling the new alpha spec 12 tests, and then merge this PR