Skip to content
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

Add ExecutionWitness to the ExecutionPayload to support the Verge #3923

Closed
wants to merge 23 commits into from

Conversation

macladson
Copy link
Member

@macladson macladson commented Jan 27, 2023

Proposed Changes

Implement the draft spec for the Verge: ethereum/consensus-specs#3230
This succeeds the more basic implementation used on the first Kaustinen testnet (see #3639)

Additional Info

Does not currently contain any fork implementation, instead it adds the new field directly into the Bellatrix structures.
Contains both #3807 (used for local testing) and #3644

Checklist

  • Add ExecutionWitness to ExecutionPayload and ExecutionPayloadHeader.
  • Add placeholder fork implementation
  • Add tests (and fix existing ones)

@macladson macladson marked this pull request as draft January 27, 2023 15:03
@macladson macladson mentioned this pull request Mar 2, 2023
7 tasks
@macladson macladson added the verge Required to support Verkle-enabled Execution label Mar 21, 2023
Copy link
Member

@paulhauner paulhauner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've had a look through the SSZ encoding of the new structs in consensus/types and they look good to me! I've left a couple of notes, nothing special.

Let me know if there are other structs you'd like me to look at ☺️

Regarding the transparent SSZ types PR, if you're using #3644 then you should be fine. The version in unstable has more features but should encode/decode the same.

#[serde(bound = "T: EthSpec")]
#[serde(rename_all = "camelCase")]
pub struct SuffixStateDiff<T: EthSpec> {
//#[serde(with = "eth2_serde_utils::quoted_u8")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unclear to me if this warrants quotes when serialized as JSON, but it shouldn't be consensus breaking and we can figure that out later.

//#[serde(with = "eth2_serde_utils::quoted_u8")]
suffix: u8,
// `None` means not currently present.
current_value: Option<StateDiffValue<T>>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Verge spec isn't clear on how this should be serialized, but I believe our implementation uses Union[None, T] which I think is the latest version to almost make it into the spec 😅

There is a contender, which is different to what we have, but it's not clear that we should switch to that IMO.

#[serde(bound = "T: EthSpec")]
#[ssz(struct_behaviour = "transparent")]
#[serde(transparent)]
pub struct BandersnatchGroupElement<T: EthSpec> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're using these transparent structs just for readability, you could also do something like:

pub type BandersnatchGroupElement<T> = FixedVector<u8, T>;

@macladson
Copy link
Member Author

Closed in favour of #4891

@macladson macladson closed this Nov 1, 2023
@macladson macladson deleted the verkle-trees-full branch November 1, 2023 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked do-not-merge verge Required to support Verkle-enabled Execution work-in-progress PR is a work-in-progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants