Stable Containers - EIP-7495 #8444

tbenr · 2024-07-15T16:47:23Z

Introduces new interfaces:

SszStableContainer
SszStableContainerSchema
SszProfile
SszProfileSchema

which are based on new SszStableContainerBase and SszStableContainerBaseSchema interfaces representing the common base functionalities.
Corresponding Abstract and Impl classes implement the logic.

Key points

these containers have a maxFieldCount (maximum future field count): the backing tree will always have that size despite the "actual" field count.
fields can be required, optional or disallowed (disallowed fields are the ones that are not specified as required nor as optional)
StableContainers have all fields as optional, Profile can have a mix of required and optional fields.
To represent which fields are active (required+optionals) an SszBitvector is used (sized as max future field count). It will be used in serialization as well as merkelization.
due to optionals and (and potentially disabled) fields, these containers are sparse, which means that size() = last_field_index + 1 does not hold anymore.
There is a difference between maxLength and maxChunks: the first is the possible maximum number of fields (required+optionals) the latter is related to the backing tree, so it is calculated based on maxFieldCount.
getOptional has been introduced to support optionality. Similarly we have createFromOptionalFieldValues on the schema side to handle optionality.
then getting fields values, the method can now throw NoSuchElementException along with existing IndexOutOfBoundsException. The general logic is:
- NoSuchElementException is thrown whenever a field was allowed but (optional) but was not actually specified.
- IndexOutOfBoundsException is thrown whenever a field was not allowed (nor optional nor required), which also covers the case the index is truly off.
  This allows to have nice implementation of the getOptional which simply delegate to get catching NoSuchElementException and returning Optional.empty.
merkelization works the same for both StableContainers and Profiles.
serialization are different. This polymorphism is achieved via abstract methods AbstractSszStableContainerBaseSchema.

High Level Usage:

to create a StableContainerSchema you need:

definedChildrenNamedSchemas: a list of NamedSchema representing the defined schemas.
maxFieldCount: theoretical future maximum field count.

to create a ProfileSchema you need:

stableContainerSchema: a SszStableContainer to create the profile schema against
requiredFieldIndices: a set of indices to indicate which fields are mandatory
optionalFieldIndices: a set of indices to indicate which fields are optional

AbstractSszStableContainerBaseSchemaTest showcases how to use them.

What is missing

Mutability is only implemented for Profiles with no optional fields. Optionality in mutation requires some additional works.
- a new interface with optional-enabled set methods.
- handling the expansion\contraction of the number of fields (and good handling of the "sparse" index set)
benchmarking?
allow to specify optional and required fields by name when creating Profiles?

Documentation

I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

I thought about adding a changelog entry, and added one if I deemed necessary.

greptile-apps

PR Summary

Added new interfaces and classes for stable containers (SszStableContainer, SszProfile, SszStableContainerBase, etc.) in /infrastructure/ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/.
Modified existing classes to handle stable containers, including exception handling (NoSuchElementException) in /infrastructure/ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/impl/AbstractSszComposite.java.
Updated serialization methods to handle extra data in backing trees in /infrastructure/ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/schema/impl/AbstractSszCollectionSchema.java.
Introduced methods to convert to stable container schemas and profile schemas in /infrastructure/ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/schema/SszContainerSchema.java.
Enhanced schema handling for optional fields and active fields in /infrastructure/ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/schema/impl/AbstractSszStableContainerBaseSchema.java.

_{29 file(s) reviewed, no comment(s)}
_{Edit PR Review Bot Settings}

Nashatyrev

LGTM, just a couple of minor notes.
Many design question we discussed and agreed in our chat before.

Nashatyrev · 2024-07-16T09:59:44Z

...a/tech/pegasys/teku/infrastructure/ssz/schema/impl/AbstractSszStableContainerBaseSchema.java

+  abstract SszBitvector sszDeserializeActiveFieldsTree(final SszReader reader);
+
+  @Override
+  public TreeNode sszDeserializeTree(final SszReader reader) {


nit: It looks like sszSerialize/DeserializeTree() have a lot of common logic with AbstractSszContainerSchema. Maybe we could extract this logic to some util class?

I did some refactor, checkout 5b13a1e

I decided to cut it at that point even I could have continued along that direction it even further.

...ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/impl/AbstractSszMutableComposite.java

greptile-apps

PR Summary

(updates since last review)

Introduced final variables for elementType and nodesCount in sszSerializeFixedVector method in /infrastructure/ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/schema/impl/AbstractSszCollectionSchema.java
Ensures variables are not reassigned, improving code safety and readability

_{1 file(s) reviewed, no comment(s)}
_{Edit PR Review Bot Settings}

greptile-apps

PR Summary

(updates since last review)

Removed special handling for SszStableContainerBase in /infrastructure/ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/impl/AbstractSszMutableComposite.java
Added calcNewSize method for size determination in /infrastructure/ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/impl/AbstractSszMutableComposite.java
Introduced calcNewSize method for stable containers in /infrastructure/ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/impl/SszMutableStableContainerBaseImpl.java
Simplified size calculation logic for better maintainability and readability
Ensure size consistency for profiles with no optional fields in stable containers

_{2 file(s) reviewed, no comment(s)}
_{Edit PR Review Bot Settings}

greptile-apps

PR Summary

(updates since last review)

Refactored serialization/deserialization logic in /infrastructure/ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/schema/impl/AbstractSszContainerSchema.java
Updated /infrastructure/ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/schema/impl/AbstractSszStableContainerBaseSchema.java to use ContainerSchemaUtil
Added new utility class /infrastructure/ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/schema/impl/ContainerSchemaUtil.java for handling SSZ containers
Ensure new utility methods handle all edge cases and maintain functionality
Streamlined handling of fixed and variable children in SSZ containers

_{3 file(s) reviewed, 2 comment(s)}
_{Edit PR Review Bot Settings}

.../ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/schema/impl/ContainerSchemaUtil.java

greptile-apps

PR Summary

(updates since last review)

Refactored serialization/deserialization logic in /infrastructure/ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/schema/impl/AbstractSszContainerSchema.java
Updated /infrastructure/ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/schema/impl/AbstractSszStableContainerBaseSchema.java to use ContainerSchemaUtil
Added new utility class /infrastructure/ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/schema/impl/ContainerSchemaUtil.java for handling SSZ containers
Ensure new utility methods handle all edge cases and maintain functionality
Streamlined handling of fixed and variable children in SSZ containers

_{3 file(s) reviewed, 2 comment(s)}
_{Edit PR Review Bot Settings}

.../ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/schema/impl/ContainerSchemaUtil.java

rolfyone · 2024-07-18T06:47:06Z

...ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/impl/AbstractSszMutableComposite.java

+   *
+   * @param index of the field set or updated
+   */
+  protected int calcNewSize(final int index) {


should probably just call this calculateNewSize rather than abbreviate...

problem is that in the ssz module seems like "calc" is the normal:

I wanted to maintain the style here. WDYT?

ok i guess but i do hate abbreviations...

infrastructure/ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/impl/SszProfileImpl.java

.../src/main/java/tech/pegasys/teku/infrastructure/ssz/schema/SszStableContainerBaseSchema.java

rolfyone

LGTM

greptile-apps

PR Summary

(updates since last review)

Introduced new interfaces and classes to support stable containers and profiles as per EIP-7495, with significant updates across multiple files to ensure compatibility and functionality.

New Interfaces: Added SszStableContainer, SszStableContainerSchema, SszProfile, and SszProfileSchema based on SszStableContainerBase and SszStableContainerBaseSchema.
Acceptance Tests: Updated acceptance-tests/src/acceptance-test/java/tech/pegasys/teku/test/acceptance/MergedGenesisAcceptanceTest.java to use genesisExecutionPayloadHeaderSource.
Validator API: Modified beacon/validator/src/main/java/tech/pegasys/teku/validator/coordinator/ValidatorApiHandler.java to align with new getter methods for SyncCommitteeSubnetSubscription.
Sync Committee: Refactored data/beaconrestapi/src/main/java/tech/pegasys/teku/beaconrestapi/handlers/v1/validator/PostSyncCommitteeSubscriptions.java to simplify request body type definition.
IPv6 Support: Enhanced networking/p2p/src/main/java/tech/pegasys/teku/networking/p2p/discovery/discv5/DiscV5Service.java and NodeRecordConverter.java to support IPv6 addresses.

_{46 file(s) reviewed, no comment(s)}
_{Edit PR Review Bot Settings}

eip7495

78c4f5d

tbenr mentioned this pull request Jul 15, 2024

Full ssz stable container #8395

Closed

2 tasks

implement some todos

ae30b64

tbenr changed the title ~~Stable Containers - EIP - 7495~~ Stable Containers - EIP-7495 Jul 15, 2024

tbenr requested a review from Nashatyrev July 15, 2024 18:07

Merge branch 'master' into stable-containers-eip7495

bf55118

greptile-apps bot reviewed Jul 16, 2024

View reviewed changes

Nashatyrev approved these changes Jul 16, 2024

View reviewed changes

some finals

4e31e71

greptile-apps bot reviewed Jul 16, 2024

View reviewed changes

improve mutability with calcNewSize

ff3b40d

greptile-apps bot reviewed Jul 16, 2024

View reviewed changes

.../ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/schema/impl/ContainerSchemaUtil.java Show resolved Hide resolved

.../ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/schema/impl/ContainerSchemaUtil.java Show resolved Hide resolved

code consolidation in util

5b13a1e

tbenr force-pushed the stable-containers-eip7495 branch from 839dff5 to 5b13a1e Compare July 16, 2024 15:21

greptile-apps bot reviewed Jul 16, 2024

View reviewed changes

.../ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/schema/impl/ContainerSchemaUtil.java Show resolved Hide resolved

.../ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/schema/impl/ContainerSchemaUtil.java Show resolved Hide resolved

rolfyone reviewed Jul 18, 2024

View reviewed changes

infrastructure/ssz/src/main/java/tech/pegasys/teku/infrastructure/ssz/impl/SszProfileImpl.java Outdated Show resolved Hide resolved

rolfyone reviewed Jul 18, 2024

View reviewed changes

.../src/main/java/tech/pegasys/teku/infrastructure/ssz/schema/SszStableContainerBaseSchema.java Show resolved Hide resolved

some finals

dc15b31

rolfyone approved these changes Jul 22, 2024

View reviewed changes

Merge branch 'master' into stable-containers-eip7495

d5d1b22

greptile-apps bot reviewed Jul 22, 2024

View reviewed changes

Merge branch 'master' into stable-containers-eip7495

bd1fe84

tbenr enabled auto-merge (squash) July 23, 2024 07:40

tbenr disabled auto-merge July 23, 2024 07:41

tbenr added 3 commits July 23, 2024 16:50

add deserialization validation

90b0c1d

Merge branch 'master' into stable-containers-eip7495

fa042de

Merge branch 'master' into stable-containers-eip7495

c09a74c

lucassaldanha added the DO NOT MERGE Not ready to merge label Sep 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stable Containers - EIP-7495 #8444

Stable Containers - EIP-7495 #8444

tbenr commented Jul 15, 2024 •

edited

Loading

greptile-apps bot left a comment

Nashatyrev left a comment

Nashatyrev Jul 16, 2024

tbenr Jul 16, 2024

tbenr Jul 16, 2024 •

edited

Loading

greptile-apps bot left a comment

greptile-apps bot left a comment

greptile-apps bot left a comment

greptile-apps bot left a comment

rolfyone Jul 18, 2024

tbenr Jul 19, 2024

rolfyone Jul 22, 2024

rolfyone left a comment

greptile-apps bot left a comment

Stable Containers - EIP-7495 #8444

Are you sure you want to change the base?

Stable Containers - EIP-7495 #8444

Conversation

tbenr commented Jul 15, 2024 • edited Loading

Key points

High Level Usage:

What is missing

Documentation

Changelog

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

Nashatyrev left a comment

Choose a reason for hiding this comment

Nashatyrev Jul 16, 2024

Choose a reason for hiding this comment

tbenr Jul 16, 2024

Choose a reason for hiding this comment

tbenr Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

rolfyone Jul 18, 2024

Choose a reason for hiding this comment

tbenr Jul 19, 2024

Choose a reason for hiding this comment

rolfyone Jul 22, 2024

Choose a reason for hiding this comment

rolfyone left a comment

Choose a reason for hiding this comment

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

tbenr commented Jul 15, 2024 •

edited

Loading

tbenr Jul 16, 2024 •

edited

Loading