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 EIP: Forward compatible consensus data structures #8439

Merged
merged 9 commits into from
May 5, 2024

Conversation

etan-status
Copy link
Contributor

No description provided.

@eth-bot
Copy link
Collaborator

eth-bot commented Apr 15, 2024

✅ All reviewers have approved.

@eth-bot eth-bot added e-consensus Waiting on editor consensus e-review Waiting on editor to review labels Apr 15, 2024
@etan-status etan-status force-pushed the sz-consensus branch 2 times, most recently from 0876895 to f33acc9 Compare April 15, 2024 17:56
@etan-status etan-status changed the title Add EIP: Forward compatible consensus SSZ data structures Add EIP: Forward compatible consensus data structures Apr 15, 2024
Copy link

The commit f33acc9 (as a parent of 115a964) contains errors.
Please inspect the Run Summary for details.

@github-actions github-actions bot added the w-ci Waiting on CI to pass label Apr 15, 2024
EIPS/eip-####.md Outdated
@@ -0,0 +1,291 @@
---
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
---
---
eip: 7688

Assigning next sequential EIP/ERC/RIP number.

Please also update the filename.

EIPS/eip-####.md Outdated
title: Forward compatible consensus data structures
description: Transition consensus SSZ data structures to StableContainer
author: Etan Kissling (@etan-status)
discussions-to: https://ethereum-magicians.org/t/eip-forward-compatible-consensus-data-structures/19673
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
discussions-to: https://ethereum-magicians.org/t/eip-forward-compatible-consensus-data-structures/19673
discussions-to: https://ethereum-magicians.org/t/eip-7688-forward-compatible-consensus-data-structures/19673

@github-actions github-actions bot added c-new Creates a brand new proposal s-draft This EIP is a Draft t-core and removed w-ci Waiting on CI to pass labels Apr 16, 2024
Copy link

@gaudren gaudren left a comment

Choose a reason for hiding this comment

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

These aren't really that important of changes, they just fit the style better. Looks great otherwise.

EIPS/eip-7688.md Outdated Show resolved Hide resolved
EIPS/eip-7688.md Outdated Show resolved Hide resolved
etan-status and others added 2 commits April 16, 2024 21:23
Co-authored-by: Nikki Gaudreau <[email protected]>
Co-authored-by: Nikki Gaudreau <[email protected]>
EIPS/eip-7688.md Outdated
For each converted data structure, a new `StableContainer` type `S` is introduced that serves as the primary definition of each data structure.

- Each `StableContainer` is assigned a capacity to represent its potential design space that SHALL NOT change across future forks; if it is later determined that it is insufficient, a new field can be added to contain additional fields in a sub-container.
- The historical forks are examined and fields that have not been present in all forks are marked as `Optional` in the new `StableContainer` type.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this might be a little confusing (and not particularly useful) considering we don't have backwards compatibility to forks before the conversion. IMO its just easier to make all fields non-optional when considering historical forks (before conversion).

- To guarantee forward and backward compatibility, new fields from future forks MUST be appended to the end of the `StableContainer` definition. Existing fields MAY be converted to `Optional`.

Furthermore, a `Variant` type is defined that is specific to the fork at which the conversion is applied. This `Variant` is the equivalent of the legacy `Container` type, except that it inherits from `Variant[S]`. The SSZ serialization of `Variant` is compatible with `Container`, but the merkleization and `hash_tree_root` are computed differently. Furthermore, `Variant` MAY use fields of `Optional` type if necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

You might mention that a new Variant will be defined for every additional fork as needed, as fields are added or removed.

EIPS/eip-7688.md Outdated Show resolved Hide resolved
EIPS/eip-7688.md Outdated

Furthermore, a `Variant` type is defined that is specific to the fork at which the conversion is applied. This `Variant` is the equivalent of the legacy `Container` type, except that it inherits from `Variant[S]`. The SSZ serialization of `Variant` is compatible with `Container`, but the merkleization and `hash_tree_root` are computed differently. Furthermore, `Variant` MAY use fields of `Optional` type if necessary.

libp2p topics still exchange fork specific `Variant` definitions using the same serialization as the legacy `Container` type. Only merkleization and `hash_tree_root` are computed differently.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not just libp2p, but all processing will be done with Variants, right? For spec purposes, just for better type safety.

EIPS/eip-7688.md Outdated Show resolved Hide resolved
EIPS/eip-7688.md Show resolved Hide resolved
EIPS/eip-7688.md Outdated Show resolved Hide resolved
Copy link
Contributor

@g11tech g11tech left a comment

Choose a reason for hiding this comment

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

looks great, lemme know once suggestions by @wemeetagain are incorporated, will approve the draft

EIPS/eip-7688.md Outdated Show resolved Hide resolved
Copy link
Contributor

@g11tech g11tech left a comment

Choose a reason for hiding this comment

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

lgtm

@eth-bot eth-bot enabled auto-merge (squash) May 5, 2024 19:38
Copy link
Collaborator

@eth-bot eth-bot left a comment

Choose a reason for hiding this comment

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

All Reviewers Have Approved; Performing Automatic Merge...

@eth-bot eth-bot merged commit a639237 into ethereum:master May 5, 2024
10 checks passed
@etan-status etan-status deleted the sz-consensus branch May 5, 2024 19:54
Copy link

@Cloudstream-io Cloudstream-io left a comment

Choose a reason for hiding this comment

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

Copy link

@Cloudstream-io Cloudstream-io left a comment

Choose a reason for hiding this comment

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

image image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-new Creates a brand new proposal e-consensus Waiting on editor consensus e-review Waiting on editor to review s-draft This EIP is a Draft t-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants