feat: on_execution_payload fork choice test support#8986
feat: on_execution_payload fork choice test support#8986brech1 wants to merge 1 commit intosigp:unstablefrom
Conversation
|
|
|
|
brech1
force-pushed
the
gloas-fork-choice-tests
branch
from
672a1b5 to
286121f
Compare
|
Let me know if this is useful, as it is right now it's just handlers code. I used this to check: Otherwise, I can close and leave the committed changes on my fork for future implementers. |
|
Awesome thanks @brech1 will take a deeper look on Monday |
| .to_string(); | ||
| let spec = &testing_spec::<E>(fork_name); | ||
| let steps: Vec<Step<String, String, Vec<String>, String, String, String>> = | ||
| let steps: Vec<Step<String, String, Vec<String>, String, String, String, String>> = |
There was a problem hiding this comment.
I think we need to add a #[allow(clippy::type_complexity)] to make clippy happy. You can check locally by running make lint-full
There was a problem hiding this comment.
Is this preferred to type alias?
There was a problem hiding this comment.
a type alias is def the cleaner approach. but since its just for testing purposes im happy with either supressing the lint error or introducing a type alias.
| if valid { | ||
| self.harness | ||
| .chain | ||
| .store | ||
| .put_payload_envelope(&block_root, envelope) | ||
| .expect("should store envelope"); | ||
|
|
||
| self.harness | ||
| .chain | ||
| .store | ||
| .put_state(&envelope_state_root, &state) | ||
| .expect("should store post-envelope state"); | ||
| } |
There was a problem hiding this comment.
envelope_processing::process_execution_payload_envelope will handle updating the store with the envelope and state (once fork choice is implemented). I think we can remove this code block
We might want to write an apply_invalid_envelope in the case of a non-valid payload (sort of how we do with apply_invalid_block)
| pub fn process_execution_payload_envelope( | ||
| &self, | ||
| envelope: SignedExecutionPayloadEnvelope<E>, | ||
| valid: bool, | ||
| ) -> Result<(), Error> { |
There was a problem hiding this comment.
since columns now get imported along with the envelope (instead of with the block) I think this should be renamed to process_execution_payload_envelope_and_columns and we should add optional columns as an argument
I suspect we'll have a Step::MaybeValidEnvelopeAndColumns eventually
|
After chatting with the team we were thinking it might be best to cherry pick these changes into our fork choice implementation branch. That way we can make sure we're passing the ef tests before merging our fork choice changes. Let me know if thats okay with you. You'd get co-author credits as part of #8906 once thats merged. |
|
@eserilev that's great! I'll go ahead and close this now then, no need for credits 👌 |
2 participants
Proposed Changes
Add test harness support for the
on_execution_payloadfork choice step type andhead_payload_statuscheck.ExecutionPayloadEnvelopestep in fork choice test runneron_execution_payloadhandler to Gloas and later forksAdditional Info
Useful for:
Enable by removing:
lighthouse/testing/ef_tests/src/handler.rs
Lines 764 to 765 in 4b3a9d3
And
CONSENSUS_SPECS_TEST_VERSIONshould be updated tov1.7.0-alpha.3:lighthouse/testing/ef_tests/Makefile
Line 3 in 4b3a9d3