attestantio / go-eth2-client

Apache License 2.0
112 stars 65 forks source link

Support custom chain presets #123

Closed pk910 closed 5 months ago

pk910 commented 6 months ago

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 commented 6 months ago

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

pk910 commented 6 months ago

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 commented 5 months ago

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 commented 5 months ago

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?

mcdee commented 5 months ago

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.