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

Support custom chain presets #123

Merged
merged 20 commits into from
May 7, 2024
Merged

Conversation

pk910
Copy link
Contributor

@pk910 pk910 commented Mar 30, 2024

This PR introduces support for custom chain presets through the use of a dynamic SSZ marshaller.

The dynamic SSZ marshaller is available at https://github.com/pk910/dynamic-ssz
It uses reflection to provide flexibility in handling SSZ encoding/decoding, enabling support for custom presets that are not covered by static code generation methods like fastssz.

While reflection-based marshalling is inherently slower than its statically generated counterparts (approximately 10x slower for unmarshalling tasks), I've mitigated this performance impact significantly.
The dynamic marshaller smartly defaults to using static code for encoding/decoding wherever no dynamic specification values are applied, ensuring that the performance penalty is minimized.
In practice, with presets such as minimal, only a small subset of structs (around 5-7) are processed dynamically, with the majority being handled by the faster static code path.

Detailed performance benchmarks can be found in the dynamic-ssz repository.

Given the complexity and the potential performance implications of the dynamic SSZ marshaller, it is not enabled by default. To opt-in for dynamic SSZ marshalling, it can be activated using the http.WithDynamicSSZ(true) parameter

Additionally, to ensure compatibility with non-mainnet presets, I've removed two tests related to committee sizes from the JSON decoding code path.
These tests relied on hardcoded spec values, which would fail under the minimal preset.
Although removing tests is generally not preferred, this trade-off was necessary to support the minimal preset without encountering conflicts with static checks in the JSON path. I hope this adjustment is acceptable :)

pk910 added a commit to pk910/dynamic-ssz that referenced this pull request Mar 30, 2024
@pk910 pk910 marked this pull request as ready for review March 31, 2024 05:58
@pk910 pk910 changed the title WIP: Support custom chain presets Support custom chain presets Mar 31, 2024
@pk910
Copy link
Contributor Author

pk910 commented Mar 31, 2024

PR check fails because 2 tests regarding the committee size check fail.
These tests have been removed from this branch, but the tests running for this PR are loaded from master

Copy link
Contributor

@mcdee mcdee left a comment

Choose a reason for hiding this comment

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

A few changes requested to reduce complexity.

One thing I note is that the module is licensed under a GPL derivative. All of the current direct dependencies are using a more liberal license (BSD,Apache,MPL) so would you consider relicensing under one of these or similar?

I'm also unsure of the parameter name. It may not be immediately obvious to someone that dynamic SSZ will create a significant overhead over the static SSZ system, and dynamic often sounds better. Perhaps something like WithCustomSpecification() or similar, although I'm not totally happy with that either. Thoughts on this?

http/beaconstate.go Outdated Show resolved Hide resolved
http/beaconstate.go Outdated Show resolved Hide resolved
spec/altair/syncaggregate.go Show resolved Hide resolved
@pk910
Copy link
Contributor Author

pk910 commented Apr 6, 2024

One thing I note is that the module is licensed under a GPL derivative. All of the current direct dependencies are using a more liberal license (BSD,Apache,MPL) so would you consider relicensing under one of these or similar?

I've changed the license of the library to apache-2.0 👍

I'm also unsure of the parameter name. It may not be immediately obvious to someone that dynamic SSZ will create a significant overhead over the static SSZ system, and dynamic often sounds better. Perhaps something like WithCustomSpecification() or similar, although I'm not totally happy with that either. Thoughts on this?

What about WithExperimentalDynamicSSZ() / WithExperimentalSSZ()?
I'm happy with WithCustomSpecification() too, maybe WithCustomSpecSupport()?

@mcdee
Copy link
Contributor

mcdee commented Apr 28, 2024

WithCustomSpecSupport() sounds good. Looks like there is a rebase required, and a tweak to some of the tests, but after that I can look to merge this.

@pk910
Copy link
Contributor Author

pk910 commented May 3, 2024

I've rebased to latest master and renamed the parameter to WithCustomSpecSupport :)

There's one important thing to note with the current implementation:
While go-eth2-client uses the dynamic SSZ library internally when WithCustomSpecSupport is true, there is no way to change the exported MarshalSSZ / UnmarshalSSZ / SizeSSZ / HashTreeRoot methods for each available spec type.
These methods still use the static fastssz code, which is expected to fail when using non-mainnet presets.
Downstream applications need to avoid using these methods when working with non-mainnet presets. Instead, there are corresponding functions available via the dynamic SSZ library (except HashTreeRoot, which is still WIP).

I think this should be documented somewhere, but I'm not sure where to do?

@pk910 pk910 requested a review from mcdee May 3, 2024 11:15
@mcdee
Copy link
Contributor

mcdee commented May 6, 2024

I think that it makes most sense to document this in WithCustomerSpecSupport, as that way it will be in the code and anyone looking up the function will see the information there.

@mcdee mcdee merged commit 2730975 into attestantio:master May 7, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants