feat(l1): implement implement forkchoiceupdatedv2, newpayloadv2 and getpayloadv2 as enum variants #1395
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This reverts commit 4256146.
fkrause98
approved these changes
Dec 3, 2024
Oppen
reviewed
Dec 3, 2024
Comment on lines
45
to
47
| ForkChoiceUpdatedVersion::V1 => 1u8, | ||
| ForkChoiceUpdatedVersion::V2 => 2u8, | ||
| ForkChoiceUpdatedVersion::V3 => 3u8, |
There was a problem hiding this comment.
You can assign this values in the enum definition so you can return the conversion to u8 directly:
pub enum ForkChoiceUpdatedVersion {
V1 = 1,
V2 = 2,
V3 = 3,
}
fn version(&self) -> u8 {
self.version as u8
}
Oppen
reviewed
Dec 3, 2024
Comment on lines
37
to
39
| ForkChoiceUpdatedVersion::V1 => "engine_forkchoiceUpdatedV1".to_string(), | ||
| ForkChoiceUpdatedVersion::V2 => "engine_forkchoiceUpdatedV2".to_string(), | ||
| ForkChoiceUpdatedVersion::V3 => "engine_forkchoiceUpdatedV3".to_string(), |
There was a problem hiding this comment.
The same trick as below enables formatting:
Suggested change
| ForkChoiceUpdatedVersion::V1 => "engine_forkchoiceUpdatedV1".to_string(), | |
| ForkChoiceUpdatedVersion::V2 => "engine_forkchoiceUpdatedV2".to_string(), | |
| ForkChoiceUpdatedVersion::V3 => "engine_forkchoiceUpdatedV3".to_string(), | |
| format!("engine_forkchoiceUpdatedV{}", self.version as usize) |
Oppen
reviewed
Dec 3, 2024
Comment on lines
+63
to
+77
| if attributes.parent_beacon_block_root.is_some() { | ||
| return Err(RpcErr::InvalidPayloadAttributes( | ||
| "forkChoiceV2 with Beacon Root".to_string(), | ||
| )); | ||
| } | ||
| if !chain_config.is_shanghai_activated(attributes.timestamp) { | ||
| return Err(RpcErr::UnsuportedFork( | ||
| "forkChoiceV2 used to build pre-Shanghai payload".to_string(), | ||
| )); | ||
| } | ||
| if chain_config.is_cancun_activated(attributes.timestamp) { | ||
| return Err(RpcErr::UnsuportedFork( | ||
| "forkChoiceV2 used to build Cancun payload".to_string(), | ||
| )); | ||
| } |
There was a problem hiding this comment.
I think these can be avoided by replacing the optionals with the same "base struct wrapped with specific struct" technique. Then there are no extraneous attributes by construction, and the others can simply count on them.
There was a problem hiding this comment.
I think in this case I prefer to use Optionals for a couple of reasons:
- It's easier for serde to parse the request into the struct. The hive test checks if we receive a v2 request with v3 parameters and the other way around, so we need to allow v2 rpc with v3 params to signal that specific error. If we used the "base struct wrapped with specific struct" scheme, we would otherwise return a generic
RpcErr::BadParamserror (because we wouldn't be able to parse v3 params in a v2 call), but the hive test expects aRpcErr::InvalidPayloadAttributeserror instead 🤦♂️ - The resulting structure (in this case a
BlockHeader) also expectsOptionalsfor the v3 params (ie.parent_beacon_block_root). So it is easier to set it directly in the end, as optional. See here
Oppen
reviewed
Dec 3, 2024
| } | ||
| } | ||
|
|
||
| // TODO(#853): Allow fork choice to be executed even if fork choice updated was not correctly parsed. |
There was a problem hiding this comment.
I'm confused by this TODO. If the fork choice was not correctly parsed, how do you know which choice you should make?
There was a problem hiding this comment.
Not sure about this either. It was already implemented for V3. Also #853 is already fixed. I need to dig a bit on this one to check if the comment still holds
github-merge-queue bot
pushed a commit
that referenced
this pull request
Dec 5, 2024
**Motivation** We need to support post merge V1, V2 and V3 for `GetPayload`, `NewPayload` and `ForkChoiceUpdated` functions **Description** Moved @mpaulucci ideas from his [PR](#1406) into [my own PR](#1395) to get V2 code for `ForkChoiceUpdated` It includes the removal of payload_attributes being a result Closes #892, #1184, #1186
|
Included in #1411 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Motivation
We need to support post merge
V1,V2andV3forGetPayload,NewPayloadandForkChoiceUpdatedfunctionsDescription
Implemented
V2for those functionsCloses #892, #1184, #1186
Makes to pass about 19 new hive tests
This PR replaces #1297 as it is the same solution using composition instead of traits