Add handshake codec #762

nytzuga · 2023-07-28T15:21:05Z

The goal of the this codec is to serialize params.UpgradeConfig in a deterministic way, to be hashed later. This hash is going to be share by nodes at handshake, so they can determine if they have the same upgrade config.

It has to be deterministic, otherwise same configs may have different hashes, making them believe they are on different configs.

This PR leverages the newly introduced support to serilize Maps
ava-labs/avalanchego#1790, which are deterministic (the
values are sorted by keys before serializing)

Why this should be merged

How this works

How this was tested

How is this documented

nytzuga · 2023-07-28T17:04:25Z

I think the main thing to be done is to write our own codec, that should extend reflectcodec (we can even host it in this repo) to sort all the properties inside a Struct before serializing. This sorting is required so, no matter how the struct is defined, we have a pretty standarized way of serializing. The ultimate goal is to have the same binary and hash for the configuration.

The new codec should also serialize anything json:* as well as it does with serialize:"true". Otherwise, all precompiles must have the serialize:"true", which is fine for now, but too error prone for the future.'

Right now this PR works, but it is suboptimal as it uses raw-JSON under the hood. I don't like that.

ceyonur · 2023-07-31T09:07:47Z

As you mentioned, JSON sorting could be potentially dangerous. GoLang JSON marshalling has a sorting mechanism but that's not in the spec and cannot be relied (it's just a implementation detail and can be changed without backward compatibility). We can still try to get advantage of json tags but using json marshalling seems a bit dangerous.

Some solutions we can use:
1- get advantage of json tags and serialize without explicitly adding new methods/new tags. This would be the most ideal solution since we won't break current precompiles; but also could not be possible to implement and we might don't want to lose time with this one.

2- Break the Config interface in precompile>precompileconfig>config.go by adding encoding.BinaryMarshaler and expect implementation.

We should add the implementation to precompile generation tool
We can use go:generate to generate the imlementation: https://go.dev/blog/generate

3- Again break the Config interface but use RLP package to generate RLP encoding: https://pkg.go.dev/github.com/ethereum/go-ethereum/rlp. This could be easier than 2nd approach, as we have an implementation here; and it's easier to generate the code through go:generate go run github.com/ethereum/go-ethereum/rlp/rlpgen ....

4- Just encode Upgrade struct in upgradable: https://github.com/ava-labs/subnet-evm/blob/master/precompile/precompileconfig/upgradeable.go#L11. We can use this to understand if precompiles/upgrades were activated at correct timestamp. However this would mean we cannot differentiate the actual configs (admin addresses, initial configs etc).

5- Use the upgrade file bytes directly as it is (no sorting, no conversion). Means that we treat files as the source of truth (rather than using structs). Obviously there could be some problems like whitespacing in files, extra comas, CR/LF style endings etc.

plugin/evm/message/handshake/upgrade_config.go

plugin/evm/message/handshake/codec.go

plugin/evm/message/handshake/upgrade_config.go

ceyonur · 2023-09-11T10:29:42Z

plugin/evm/message/handshake/upgrade_config_test.go

+	"github.com/stretchr/testify/assert"
+)
+
+func TestSerialize(t *testing.T) {


can we add more test cases with:

Enabled/Admin addresses

Initial configs (like mint config)

State Upgrades

I think we need to make sure:

all of our precompiles can be deserializable

add serialize tags to precompile config template

if possible add a test to config_test.goso that new precompiles can be tested out when generated.

This is still pending.

I'm writing more tests now

precompile/contracts/feemanager/config.go

plugin/evm/message/handshake/upgrade_config_test.go

plugin/evm/message/handshake/upgrade_config.go

plugin/evm/message/handshake/codec.go

params/config.go

ceyonur · 2023-10-09T09:19:14Z

params/network_upgrades.go

-// TODO: once we add the first optional upgrade here, we should uncomment TestVMUpgradeBytesOptionalNetworkUpgrades
-type OptionalNetworkUpgrades struct{}
+type OptionalNetworkUpgrades struct {
+	Updates []Fork `json:"serialize,omitempty" serialize:"true"`


I was thinking this to be something like:

type OptionalNetworkUpgrades struct { XYZUpgradeTimestamp *uint64 `json:"xyzUpgradeTimestamp,omitempty"` serialize:"true" ...

But I'm not completely against using a list of upgrades here. Seems like with this way we can just include them as elements in a list in the genesis/upgrades to be activated.

However we should not use fork type here as (we would not need optional + block types in the struct). We can define our custom struct to wire this and revert changes to the fork type.

Plus this is still wrong as the json tag is serialize, it should be at least something like json:"optionalNetworkUpgrades,omitempty"

cc @aaronbuchwald @darioush

precompile/contracts/feemanager/config.go

plugin/evm/message/handshake/upgrade_config.go

ceyonur · 2023-10-09T12:23:46Z

plugin/evm/message/handshake/upgrade_config_test.go

+	"github.com/stretchr/testify/assert"
+)
+
+func TestSerialize(t *testing.T) {


params/config.go

Use MarshalBinary and UnmarshalBinary

params/config.go

precompile/allowlist/config.go

aaronbuchwald · 2023-12-16T19:04:55Z

commontype/fee_config.go

+func (c *FeeConfig) getBigIntToSerialize() []**big.Int {
+	return []**big.Int{
+		&c.GasLimit, &c.MinBaseFee, &c.TargetGas, &c.BaseFeeChangeDenominator,
+		&c.MinBlockGasCost, &c.MaxBlockGasCost, &c.BlockGasCostStep,
+	}
+}


I see this is trying to make the code simpler by passing a list of pointers to pointers so we can serialize and de-serialize with a for loop instead of one by one.

It's not valid for any of these to be nil, so would it be possible to remove the double pointer and enforce that all of them are non-nil instead of allowing a boolean to indicate whether or not it's nil?

commontype/fee_config.go

aaronbuchwald · 2023-12-16T19:23:40Z

params/config.go

+	bytes, err := originalConfig.MarshalBinary()
+	require.NoError(t, err)
+
+	deserializedConfig := UpgradeConfig{}
+	require.NoError(t, deserializedConfig.UnmarshalBinary(bytes))
+
+	twiceDeserialized := UpgradeConfig{}
+	newBytes, err := deserializedConfig.MarshalBinary()
+	require.NoError(t, err)
+	require.NoError(t, twiceDeserialized.UnmarshalBinary(newBytes))


When will this fail because we do it three times instead of two?

If newBytes is the same as bytes (which should be confirmed by checking that the hashes match), then I don't think that twiceDeserialized provides additional coverage. Afaict this will fail if UnmarshalBinary is itself non-deterministic on the SAME bytes.

precompile/contracts/nativeminter/config.go

aaronbuchwald · 2023-12-16T19:30:30Z

precompile/contracts/feemanager/config_test.go

+			},
+		},
+	}
+	params.AssertConfigHashesAndSerialization(t, &config)


Could we add an expected hash to these tests, so that the tests will fail if the format changes unexpectedly? (this has the cost of needing to update the expected hash values if we change the format, so may be easiest to make this a follow up)

aaronbuchwald · 2023-12-16T19:31:47Z

precompile/contracts/nativeminter/config_test.go

+	config := params.UpgradeConfig{
+		PrecompileUpgrades: []params.PrecompileUpgrade{
+			{
+				Config: NewConfig(&t0, []common.Address{


can we add a test with multiple addresses?

aaronbuchwald · 2023-12-16T19:32:21Z

precompile/contracts/rewardmanager/config.go

+	if p.Err != nil {
+		return p.Err
+	}
+	u.RewardAddress = common.BytesToAddress(p.UnpackBytes())


can we use fixed bytes of length 20?

Done in e079719

accounts/abi/bind/precompilebind/precompile_config_template.go

accounts/abi/bind/precompilebind/precompile_config_test_template.go

ceyonur · 2023-12-18T08:10:24Z

params/state_upgrade_test.go

+			{
+				BlockTimestamp: &t0,
+				StateUpgradeAccounts: map[common.Address]StateUpgradeAccount{
+					common.BytesToAddress(common.Hex2Bytes("0000000000000000000000000000000000000000000000000000000000000050")): StateUpgradeAccount{


nit: can we create a variable for these addresses instead of hardcoding them into tests?

precompile/allowlist/config.go

accounts/abi/bind/precompilebind/precompile_config_test_template.go

ceyonur · 2023-12-18T08:21:55Z

params/config.go

+// hashing, depite maybe not being identical (some configurations may be in a
+// different order, but our hashing algorithm is resilient to those changes,
+// thanks for our serialization library, which produces always the same output.
+func AssertConfigHashesAndSerialization(t *testing.T, originalConfig *UpgradeConfig) {


Requiring this function from params package in precompiles will create a cyclic dependency most likely. I think this assertion can be reduced as suggested. So we can just readd these assertions in relevant test files without requiring to import from params package.

params/config.go

accounts/abi/bind/precompilebind/precompile_config_template.go

ceyonur · 2023-12-18T08:28:47Z

accounts/abi/bind/precompilebind/precompile_config_template.go

@@ -48,6 +54,14 @@ func NewConfig(blockTimestamp *uint64{{if .Contract.AllowList}}, admins []common
 	}
 }

+func (c * Config) MarshalBinary() ([]byte, error) {


can we add some comments here why these are important, how these can be implemented etc?

Co-authored-by: Ceyhun Onur <[email protected]> Signed-off-by: Cesar <[email protected]>

Co-authored-by: aaronbuchwald <[email protected]> Signed-off-by: Cesar <[email protected]>

…codec

Instead loop them together

It generates valid marshal/unmarshal t# Please enter the commit message for your changes. Lines starting

nytzuga requested review from ceyonur, darioush and aaronbuchwald as code owners July 28, 2023 15:21

nytzuga force-pushed the handshake-codec branch 2 times, most recently from 7c0580f to a5266ff Compare July 28, 2023 16:58

nytzuga force-pushed the handshake-codec branch from a5266ff to 50741b9 Compare August 14, 2023 02:37

nytzuga self-assigned this Aug 14, 2023

nytzuga marked this pull request as draft August 14, 2023 02:40

nytzuga force-pushed the handshake-codec branch 2 times, most recently from de94050 to 6740f23 Compare August 14, 2023 03:04

nytzuga force-pushed the handshake-codec branch from 83cb2b6 to a3a6471 Compare September 8, 2023 06:43

nytzuga changed the title ~~Draft: Add handshake codec~~ Add handshake codec Sep 8, 2023

nytzuga marked this pull request as ready for review September 8, 2023 14:48

nytzuga requested a review from anusha-ctrl as a code owner September 8, 2023 14:48

ceyonur reviewed Sep 11, 2023

View reviewed changes

aaronbuchwald reviewed Sep 13, 2023

View reviewed changes

plugin/evm/message/handshake/codec.go Outdated Show resolved Hide resolved

nytzuga force-pushed the handshake-codec branch 3 times, most recently from 7117339 to 41d7776 Compare September 14, 2023 19:22

nytzuga force-pushed the handshake-codec branch from 41d7776 to 952f21c Compare September 26, 2023 06:15

aaronbuchwald requested a review from ceyonur October 4, 2023 14:44

ceyonur reviewed Oct 9, 2023

View reviewed changes

nytzuga force-pushed the handshake-codec branch 5 times, most recently from 5a1af29 to b6c0e8a Compare October 13, 2023 03:33

nytzuga commented Oct 13, 2023

View reviewed changes

params/config.go Outdated Show resolved Hide resolved

Remove all custom serializers

6785dd9

Use MarshalBinary and UnmarshalBinary

nytzuga force-pushed the handshake-codec branch from 4591ccd to 6785dd9 Compare December 15, 2023 21:54

aaronbuchwald reviewed Dec 16, 2023

View reviewed changes

ceyonur reviewed Dec 18, 2023

View reviewed changes

nytzuga and others added 16 commits December 28, 2023 17:32

WIP

32486fe

Merge remote-tracking branch 'origin/master' into handshake-codec

8bc4f87

Do not use double pointers

2de9095

Apply suggestions from code review

3a1bd28

Co-authored-by: Ceyhun Onur <[email protected]> Signed-off-by: Cesar <[email protected]>

Update precompile/allowlist/config.go

b2020cb

Co-authored-by: aaronbuchwald <[email protected]> Signed-off-by: Cesar <[email protected]>

Update precompile/allowlist/config.go

beb7ab8

Co-authored-by: aaronbuchwald <[email protected]> Signed-off-by: Cesar <[email protected]>

Bring back getBigIntToSerialize() until a better solution can be found

8192cb2

Merge remote-tracking branch 'origin/handshake-codec' into handshake-…

b86226d

…codec

Remove all magic bytes

8935259

Instead loop them together

Use fixed bytes for addresses

5c88c08

Improve serialization and testing

1819327

Use fixed bytes for addresses

e079719

Minor changes

17bf11e

Move UnpackAddresses/PackAddresses to utils

91bdff5

Fix typo

a593a68

Update template

987017b

It generates valid marshal/unmarshal t# Please enter the commit message for your changes. Lines starting

nytzuga marked this pull request as draft January 4, 2024 14:09

nytzuga added 7 commits January 4, 2024 15:09

Move serialization to utils

19b3073

Fixed typo

e8ffb13

Fixed typo

550f7c0

Fix typo

a08d746

Merge remote-tracking branch 'origin/master' into handshake-codec

e0b4b61

Rebase with master

1bf4664

Add comments back

ac0e945

ceyonur modified the milestones: subnet-evm@Future Release, v0.6.x (Future) Jan 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add handshake codec #762

Add handshake codec #762

nytzuga commented Jul 28, 2023 •

edited

Loading

nytzuga commented Jul 28, 2023

ceyonur commented Jul 31, 2023

ceyonur Sep 11, 2023

ceyonur Sep 11, 2023

ceyonur Oct 9, 2023

ceyonur Nov 23, 2023

nytzuga Nov 27, 2023

ceyonur Oct 9, 2023

ceyonur Oct 9, 2023

aaronbuchwald Dec 16, 2023

aaronbuchwald Dec 16, 2023

aaronbuchwald Dec 16, 2023

aaronbuchwald Dec 16, 2023

aaronbuchwald Dec 16, 2023

nytzuga Jan 4, 2024

ceyonur Dec 18, 2023

ceyonur Dec 18, 2023

ceyonur Dec 18, 2023

Add handshake codec #762

Are you sure you want to change the base?

Add handshake codec #762

Conversation

nytzuga commented Jul 28, 2023 • edited Loading

Why this should be merged

How this works

How this was tested

How is this documented

nytzuga commented Jul 28, 2023

ceyonur commented Jul 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nytzuga commented Jul 28, 2023 •

edited

Loading