feat: improve randomize_state field coverage for fork transition testing#4594
feat: improve randomize_state field coverage for fork transition testing#4594richardgreg wants to merge 1 commit intoethereum:masterfrom
Conversation
a20cf1d to
a2b434a
Compare
|
@jtraglia, do you think it is necessary to populate the fields for previous forks? |
Yes, it would be valuable to do this. Please complete the TODO items you've stated in the functions. Sorry I just haven't had enough time to properly review your PR. Given that this change will affect lots tests for several forks, it will require a more extensive review. In the mean time, I've started the CI checks for this. I would like for @leolara to review it too. He's in charge of testing here. |
5c00326 to
537e0d0
Compare
|
Hi @leolara, what are your thoughts? |
|
|
||
| def _randomize_phase0_fields(spec, state): | ||
| """Set Phase0-specific fields to realistic non-default values.""" | ||
| if is_post_altair(spec): |
There was a problem hiding this comment.
@richardgreg I think this ìf` either should not be here or throw an exception. Like it should not be allowed to call this function in the phase that is not supposed to be used. I search our code base and there are no examples like this, I think.
What do you think?
There was a problem hiding this comment.
You're right, Leo. It's a counterproductive measure
|
@richardgreg the reftests are failing in this branch: https://github.com/ethereum/consensus-specs/actions/runs/18327061427/job/52193981595 |
741a30e to
1f7da1b
Compare
|
@richardgreg let me know if I should try to run the tests again |
1f7da1b to
ca9f91e
Compare
@leolara sure. You can go ahead and do it. My branch is up-to-date and it generates the reftests locally. I'm hoping that will fix any conflict arising in the CI |
|
@richardgreg the reference tests are failing in this branch, please check the logs there https://github.com/ethereum/consensus-specs/actions/runs/18891974203 and see if you can fix it |
Right, Leo. I'm investigating it |
* Add Electra churn field randomization * Add withdrawal field randomization for Capella+ forks * Add finality checkpoint randomization with safety checks * Add pending operations randomization * Randomize fields for all forks transitioning from phase0 to Electra
ca9f91e to
b6f3115
Compare
|
@leolara In this latest push, Altair now inherits Phase 0's fields. The bug was likely due to BLS signature verification failing. Altair not inheriting Phase 0 fields including So I went quite all in this issue without much external input. From what you've seen so far, what are your thoughts on the fields chosen to be randomized for each fork? |
|
I am closing this, but let me know if you think we should reopen it |
|
Hi @leolara, okay. I'll let you know when to reopen it for review. My initial solution was kinda rushed, so I'm taking a new approach. I'm putting together a system design for the problem based on the learnings from my initial attempt. I'd love for you to help me review it when I'm done before I put it into code. I'd also appreciate it if you could confirm that the correct test command for spotting the bug in my initial solution is Happy New Year |
feat: improve
randomize_statefield coverage for fork transition testingDescription
randomize_statefunction to populate beacon state fields with realistic non-default values during testing.bugs because 0 == 0 assertions still pass, as demonstrated in the issue description with the Teku client. By using realistic test values, missing field copies now cause test failures
randomize_statefield population now extended to all forksRelations
Fixes #4535
Co-authored by Claude