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

Create custom serialization for external structs #1020

Closed
nytzuga opened this issue Dec 4, 2023 · 3 comments
Closed

Create custom serialization for external structs #1020

nytzuga opened this issue Dec 4, 2023 · 3 comments
Assignees
Labels
enhancement New feature or request

Comments

@nytzuga
Copy link
Contributor

nytzuga commented Dec 4, 2023

Context and scope

As part of #758 and #268, it is needed to support external structures. Our codec uses reflection and serializes only fields which have the serialize:"true" tag.

That is problematic for external structs, particularly the big.Int struct, which is part of the data to be exchanged between nodes or to calculate the upgrade config change. Hashing or exchanging configurations require serialization and big.Int is ignored since they don't have any serialize:"true" for their inner members.

Discussion and alternatives

There are two alternatives proposed in this issue.

serialize:"all"

The easiest approach is introducing yet another patch to our codec, extending it to support serialize:"all".

For context, several improvements have already been made to enhance the codec to support the serialization of the config (nullable pointers, deterministic map serialization).

This new tag will be another way of serializing fields (alongside with serialize:"true", serialize:"true,nullable"). This tag will signal the codec to serialize all inner fields as if they all have the serialize:"true" tag in each field and inner fields.

This change will leverage a lot of the existing code in the codec, and it is backward compatible.

Wrap the big.Int

The alternative solution would be to have our version of big.Int, owned by the package. This type will be converted to a property big.Int for usage after deserialization.

This solution would require zero changes in the codec but will require changes in all the codec inside subnet-EVM. This solution would have to be replicated everywhere in the configurations where big.Int and any external struct is being read.

Open questions

Is there any other alternative?

@nytzuga nytzuga added the enhancement New feature or request label Dec 4, 2023
@nytzuga nytzuga self-assigned this Dec 4, 2023
@ceyonur
Copy link
Collaborator

ceyonur commented Dec 4, 2023

There are few other options:

  • Adding a custom marshaller/encoder support so that we would not need to expand the codec everytime with new types.
  • Creating our own codec in subnet-evm.
  • Create intermediate types like in gen_genesis.go

nytzuga added a commit to ava-labs/avalanchego that referenced this issue Dec 6, 2023
In an attempt to better solve
ava-labs/subnet-evm#1020, this is an approach to let
serialize types with the standard Text or Binary marshal/unmarshal.

This is an attempt to have a more robust solution to
ava-labs/subnet-evm#1022 (which relies on a feature
inside Golang that may generate determinist JSON)
nytzuga added a commit to ava-labs/avalanchego that referenced this issue Dec 6, 2023
In an attempt to better solve
ava-labs/subnet-evm#1020, this is an approach to let
serialize types with the standard Text or Binary marshal/unmarshal.

This is an attempt to have a more robust solution to
ava-labs/subnet-evm#1022 (which relies on a feature
inside Golang that may generate determinist JSON)
@ceyonur
Copy link
Collaborator

ceyonur commented Dec 9, 2023

Some more ideas:

Custom Handler In Codec

This involves adding a handler that would check if a certain interface is implemented and call custom serialize/deserialize method. This involves a change to the codec.

In this case we should decide which interface to be handled in codec. Possibilities are encoding.MarshalBinary, encoding.MarshalText, gob.Encode or a new custom interface .

IMO encoding.MarshalText is not very suitable for binary marshalling as it is used for UTF-8 texts and size is not a constraint where in our codec size is important.

gob.Encode could be a nice candidate as it's mainly used for over-the-wire encoding (like rpc), and suits our needs. Unfortunately we have seen that big.Int package implementation of gob.Encode uses a version in the serialized output. Which means if the version is bumped/changed, it requires a coordinated upgrade on the network or means we cannot update the golang version.

That leaves us with MarshalBinary but it also posses a risk to unintentionally handle our existing types if they already implements the MarshalBinary. This could change how they were encoded in the codec.

Currently none of our required types like common.Hash, common.Address or big.Int implements MarshalBinary anyway. So we should either use encoding.MarshalText to support encoding of these types out-of-package or we should wrap them to use either MarshalBinary or a new custom interface. If that's the case I'd probably prefer the safer option of introducing a new interface.

We should make sure everything works with this approach before we actually merge anything to the codec.

Create an Serializer/Deserializer interface for PrecompileConfig

With this way we can ensure every precompile config implements a serializer/deserializer of their own. This is the safest option because we'd not need to change the codec in AvalancheGo and if a bug occurs this could only impact the Subnet-EVM's config handshake. This could a bit complicated for external developers and we probably still need to use a library/helper method to make those functions a bit standardized and easier than actually encoding/decoding every field in config.

Borsh Codec (Reflect codec)

@patrick-ogrady suggested we can use this library to serialize/deserialize sturct directly without any change to types (or avago codec) https://github.com/near/borsh-go. This also would require to be tested out with our existing precompiles (and some additional test with common types like common.Hash, common.Address etc).

@nytzuga
Copy link
Contributor Author

nytzuga commented Dec 14, 2023

This is being solved in #762 implementing UnmarshalBinary/MarshalBinary

@nytzuga nytzuga closed this as completed Dec 14, 2023
@github-project-automation github-project-automation bot moved this from Backlog 🗄️ to Done ✅ in Platform Engineering Group Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

No branches or pull requests

2 participants