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

Add handshake codec #762

Draft
wants to merge 57 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 34 commits
Commits
Show all changes
57 commits
Select commit Hold shift + click to select a range
0f72644
Add handshake codec
nytzuga Jul 28, 2023
89bf82c
Use 8 bytes to hash UpgradeConfig
nytzuga Aug 14, 2023
19e0d18
Fixed linting issues
nytzuga Sep 8, 2023
30b8aaa
Addressed PR comments
nytzuga Sep 14, 2023
4a59b8a
Improve API usage
nytzuga Sep 14, 2023
acc8195
Rewrote configs
nytzuga Oct 13, 2023
a0ceae8
WIP: Optional upgrades tests
nytzuga Oct 20, 2023
c5d86a4
Upgrade to the latest version
nytzuga Nov 2, 2023
b6f8a33
Merge remote-tracking branch 'origin/master' into handshake-codec
nytzuga Nov 20, 2023
453c84d
Remove unknown field
nytzuga Nov 20, 2023
7c507ac
Address comments from PR
nytzuga Nov 20, 2023
9c954d5
Update precompile template
nytzuga Nov 20, 2023
a27ecc8
Merge remote-tracking branch 'origin/master' into handshake-codec
nytzuga Nov 27, 2023
771d511
Improve template
nytzuga Nov 27, 2023
c261ce2
Add more test cases
nytzuga Nov 27, 2023
6fff967
Linting
nytzuga Nov 27, 2023
e3601b0
Remove Test config
nytzuga Nov 27, 2023
cade28f
Use whole hash instead of the first 8 bytes
nytzuga Nov 28, 2023
6b96e44
Merge remote-tracking branch 'origin/master' into handshake-codec
nytzuga Nov 29, 2023
a2848d7
Merge remote-tracking branch 'origin/master' into handshake-codec
nytzuga Nov 30, 2023
d996f99
Update how optional upgrades are going to be represented internally
nytzuga Dec 4, 2023
758441f
Merge remote-tracking branch 'origin/master' into handshake-codec
nytzuga Dec 4, 2023
634e9b5
Add ToBytes() and FromBytes() to the Config interface
nytzuga Dec 12, 2023
d1b37c4
Merge remote-tracking branch 'origin/master' into handshake-codec
nytzuga Dec 12, 2023
eef61dd
Update template
nytzuga Dec 12, 2023
541acd7
Update mocks
nytzuga Dec 13, 2023
2309af5
Remove serialize tag
nytzuga Dec 13, 2023
74e25d5
Implement custom marshal/unmarshal for all types
nytzuga Dec 13, 2023
f1d3d07
Use standard Marshal/Unmarshaller
nytzuga Dec 14, 2023
55f4b95
Implement UpgradeConfig Marshal/Unmarshal
nytzuga Dec 14, 2023
d9851ca
Added Marshal / Unmarshal to OptionalNetworkUpgrades
nytzuga Dec 14, 2023
1946de4
Remove sorting from AllowListConfig
nytzuga Dec 15, 2023
2efae3d
Move tests to their own file
nytzuga Dec 15, 2023
6785dd9
Remove all custom serializers
nytzuga Dec 15, 2023
32486fe
WIP
nytzuga Dec 28, 2023
8bc4f87
Merge remote-tracking branch 'origin/master' into handshake-codec
nytzuga Jan 4, 2024
2de9095
Do not use double pointers
nytzuga Jan 4, 2024
3a1bd28
Apply suggestions from code review
nytzuga Jan 4, 2024
b2020cb
Update precompile/allowlist/config.go
nytzuga Jan 4, 2024
beb7ab8
Update precompile/allowlist/config.go
nytzuga Jan 4, 2024
8192cb2
Bring back getBigIntToSerialize() until a better solution can be found
nytzuga Jan 4, 2024
b86226d
Merge remote-tracking branch 'origin/handshake-codec' into handshake-…
nytzuga Jan 4, 2024
8935259
Remove all magic bytes
nytzuga Jan 4, 2024
5c88c08
Use fixed bytes for addresses
nytzuga Jan 4, 2024
1819327
Improve serialization and testing
nytzuga Jan 4, 2024
e079719
Use fixed bytes for addresses
nytzuga Jan 4, 2024
17bf11e
Minor changes
nytzuga Jan 4, 2024
91bdff5
Move UnpackAddresses/PackAddresses to utils
nytzuga Jan 4, 2024
a593a68
Fix typo
nytzuga Jan 4, 2024
987017b
Update template
nytzuga Jan 4, 2024
19b3073
Move serialization to utils
nytzuga Jan 4, 2024
e8ffb13
Fixed typo
nytzuga Jan 4, 2024
550f7c0
Fixed typo
nytzuga Jan 4, 2024
a08d746
Fix typo
nytzuga Jan 4, 2024
e0b4b61
Merge remote-tracking branch 'origin/master' into handshake-codec
nytzuga Jan 5, 2024
1bf4664
Rebase with master
nytzuga Jan 5, 2024
ac0e945
Add comments back
nytzuga Jan 5, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions accounts/abi/bind/precompilebind/precompile_config_template.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ const tmplSourcePrecompileConfigGo = `
package {{.Package}}

import (
"errors"

"github.com/ava-labs/subnet-evm/precompile/precompileconfig"
{{- if .Contract.AllowList}}
"github.com/ava-labs/subnet-evm/precompile/allowlist"
Expand All @@ -26,8 +28,12 @@ var _ precompileconfig.Config = &Config{}
// adds specific configuration for {{.Contract.Type}}.
type Config struct {
{{- if .Contract.AllowList}}
// Serialize allowList to let nodes exchange information about their
// precompile configs
nytzuga marked this conversation as resolved.
Show resolved Hide resolved
allowlist.AllowListConfig
{{- end}}
// Serialize the Upgrade config to let nodes exchange information about
// their precompile configs
nytzuga marked this conversation as resolved.
Show resolved Hide resolved
precompileconfig.Upgrade
nytzuga marked this conversation as resolved.
Show resolved Hide resolved
// CUSTOM CODE STARTS HERE
// Add your own custom fields for Config here
Expand All @@ -48,6 +54,14 @@ func NewConfig(blockTimestamp *uint64{{if .Contract.AllowList}}, admins []common
}
}

func (c * Config) MarshalBinary() ([]byte, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

return nil, errors.New("implement MarshalBinary() method")
nytzuga marked this conversation as resolved.
Show resolved Hide resolved
}

func (c * Config) UnmarshalBinary(bytes []byte) error {
return errors.New("implement UnmarshalBinary() method")
}

// NewDisableConfig returns config for a network upgrade at [blockTimestamp]
// that disables {{.Contract.Type}}.
func NewDisableConfig(blockTimestamp *uint64) *Config {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ package {{.Package}}
import (
"testing"

"github.com/ava-labs/subnet-evm/params"
"github.com/ava-labs/subnet-evm/precompile/precompileconfig"
"github.com/ava-labs/subnet-evm/precompile/testutils"
"github.com/ava-labs/subnet-evm/utils"
Expand All @@ -22,6 +23,7 @@ import (
"github.com/ethereum/go-ethereum/common"
{{- end}}
"go.uber.org/mock/gomock"
"github.com/stretchr/testify/require"
)

// TestVerify tests the verification of Config.
Expand Down Expand Up @@ -61,6 +63,35 @@ func TestVerify(t *testing.T) {
{{- end}}
}

func TestSerialize(t *testing.T) {
var t0 uint64 = 2
var t1 uint64 = 1001

config := params.UpgradeConfig{
PrecompileUpgrades: []params.PrecompileUpgrade{
{
Config: NewConfig(&t0,
{{- if .Contract.AllowList}}
[]common.Address{
common.BytesToAddress(common.Hex2Bytes("0000000000000000000000000000000000000000000000000000000000000020")),
common.BytesToAddress(common.Hex2Bytes("0000000000000000000000000000000000000000000000000000000000000030")),
}, []common.Address{
common.BytesToAddress(common.Hex2Bytes("0000000000000000000000000000000000000000000000000000000000000040")),
}, []common.Address{
common.BytesToAddress(common.Hex2Bytes("0000000000000000000000000000000000000000000000000000000000000050")),
},
{{- end}}
), // enable at genesis
nytzuga marked this conversation as resolved.
Show resolved Hide resolved
},
{
Config: NewDisableConfig(&t1), // disable at timestamp 1
},
},
}
require.NotNil(t, config)
//params.AssertConfigHashesAndSerialization(t, &config)
nytzuga marked this conversation as resolved.
Show resolved Hide resolved
}

// TestEqual tests the equality of Config with other precompile configs.
func TestEqual(t *testing.T) {
{{- if .Contract.AllowList}}
Expand Down
61 changes: 61 additions & 0 deletions commontype/fee_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@ import (
"fmt"
"math/big"

"github.com/ava-labs/avalanchego/utils/wrappers"
"github.com/ava-labs/subnet-evm/utils"
"github.com/docker/docker/pkg/units"
"github.com/ethereum/go-ethereum/common"
)

Expand Down Expand Up @@ -144,6 +146,65 @@ func (f *FeeConfig) checkByteLens() error {
return nil
}

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

Choose a reason for hiding this comment

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

what is this used for? this look a bit weird.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will return a list of fields to be serialized, in that order. This list is being used to deserialize and serialize. It is just less error prone this way I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aaronbuchwald Is this kinda of code not desired? It is a canonical list of numbers and how they are marshal and unmarshal. It is way error prone and it takes advantage of the pointer to a pointer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 2de9095


func (c *FeeConfig) MarshalBinary() ([]byte, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add the following test cases for the serialization of FeeConfig:

  1. serialize and de-serialize valid config and get the same value
  2. hardcoded test vector and require it results in the same value
  3. unmarshal should fail if there are insufficient bytes to unpack big int values

p := wrappers.Packer{
Bytes: []byte{},
MaxSize: 1 * units.MiB,
}

for _, bigint := range c.getBigIntToSerialize() {
p.PackBool(*bigint == nil)
if p.Err != nil {
return nil, p.Err
}
if bigint != nil {
p.PackBytes((*bigint).Bytes())
if p.Err != nil {
return nil, p.Err
}
}
}
p.PackLong(c.TargetBlockRate)
if p.Err != nil {
return nil, p.Err
}

return p.Bytes, nil
}

func (c *FeeConfig) UnmarshalBinary(data []byte) error {
p := wrappers.Packer{
Bytes: data,
}
for _, bigint := range c.getBigIntToSerialize() {
isNil := p.UnpackBool()
if p.Err != nil {
return p.Err
}
if isNil {
continue
}
*bigint = big.NewInt(0).SetBytes(p.UnpackBytes())
nytzuga marked this conversation as resolved.
Show resolved Hide resolved
if p.Err != nil {
return p.Err
}
}

c.TargetBlockRate = p.UnpackLong()
if p.Err != nil {
return p.Err
}

return nil
}

func isBiggerThanHashLen(bigint *big.Int) bool {
buf := bigint.Bytes()
isBigger := len(buf) > common.HashLength
Expand Down
186 changes: 186 additions & 0 deletions params/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,17 +27,23 @@
package params

import (
"bytes"
"encoding/json"
"errors"
"fmt"
"math/big"
"testing"

"github.com/ava-labs/avalanchego/snow"
"github.com/ava-labs/avalanchego/utils/wrappers"
"github.com/ava-labs/subnet-evm/commontype"
"github.com/ava-labs/subnet-evm/precompile/modules"
"github.com/ava-labs/subnet-evm/precompile/precompileconfig"
"github.com/ava-labs/subnet-evm/utils"
"github.com/docker/docker/pkg/units"
"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/crypto"
"github.com/stretchr/testify/require"
)

const maxJSONLen = 64 * 1024 * 1024 // 64MB
Expand Down Expand Up @@ -67,6 +73,17 @@ var (
MaxBlockGasCost: big.NewInt(1_000_000),
BlockGasCostStep: big.NewInt(200_000),
}

// For UpgradeConfig Marshal/Unmarshal
magicHeader = []byte{0xff}
sectionHeaderSizes = 1
precompileHeader = []byte{0xaa}
stateUpdateHeader = []byte{0xc8}
endHeader = []byte{0}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need these headers? I believe these are typically used to signal what type of data they include, but we should know that when serializing/de-serializing, so I don't think these should be necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 8935259, instead I'm having a deterministic serialization

ErrIncorrectHeader = errors.New("invalid magic header")
ErrMalformedConfigHeader = errors.New("malformed config header")
ErrUnknowPrecompile = errors.New("unknown precompile config")
MaxMessageSize = 1 * units.MiB
)

var (
Expand Down Expand Up @@ -171,6 +188,142 @@ type UpgradeConfig struct {
PrecompileUpgrades []PrecompileUpgrade `json:"precompileUpgrades,omitempty"`
}

func (c *UpgradeConfig) Hash() (common.Hash, error) {
bytes, err := c.MarshalBinary()
if err != nil {
return common.Hash{}, err
}
return crypto.Keccak256Hash(bytes), nil
}
Comment on lines +183 to +189
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably move all of these to a separate upgrade_config file in the same package.


func (c *UpgradeConfig) MarshalBinary() ([]byte, error) {
p := wrappers.Packer{
Bytes: []byte{},
MaxSize: MaxMessageSize,
}
p.PackFixedBytes(magicHeader)
if p.Err != nil {
return nil, p.Err
}
nytzuga marked this conversation as resolved.
Show resolved Hide resolved

p.PackBool(c.OptionalNetworkUpgrades == nil)
if p.Err != nil {
return nil, p.Err
}
if c.OptionalNetworkUpgrades != nil {
bytes, err := c.OptionalNetworkUpgrades.MarshalBinary()
if err != nil {
return nil, err
}
p.PackBytes(bytes)
if p.Err != nil {
return nil, p.Err
}
}

for _, precompileConfig := range c.PrecompileUpgrades {
bytes, err := precompileConfig.Config.MarshalBinary()
if err != nil {
return nil, err
}
p.PackFixedBytes(precompileHeader)
if p.Err != nil {
return nil, p.Err
}
p.PackStr(precompileConfig.Key())
if p.Err != nil {
return nil, p.Err
}
p.PackBytes(bytes)
if p.Err != nil {
return nil, p.Err
}
}

for _, config := range c.StateUpgrades {
bytes, err := config.MarshalBinary()
if err != nil {
return nil, err
}
p.PackFixedBytes(stateUpdateHeader)
if p.Err != nil {
return nil, p.Err
}
p.PackBytes(bytes)
if p.Err != nil {
return nil, p.Err
}
}

p.PackFixedBytes(endHeader)

return p.Bytes, nil
}

func (c *UpgradeConfig) UnmarshalBinary(data []byte) error {
p := wrappers.Packer{
Bytes: data,
}
if !bytes.Equal(p.UnpackFixedBytes(sectionHeaderSizes), magicHeader) {
return ErrIncorrectHeader
}
if p.Err != nil {
return p.Err
}

isNil := p.UnpackBool()
if !isNil {
c.OptionalNetworkUpgrades = &OptionalNetworkUpgrades{}
bytes := p.UnpackBytes()
if p.Err != nil {
return p.Err
}
if err := c.OptionalNetworkUpgrades.UnmarshalBinary(bytes); err != nil {
return nil
nytzuga marked this conversation as resolved.
Show resolved Hide resolved
}
}

for {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's switch to packing the number of elements of each type, so that we can proceed through field by field and unpack the number of elements, all of the elements of that type, and then proceed to the next field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 8935259

header := p.UnpackFixedBytes(sectionHeaderSizes)
if p.Err != nil {
return p.Err
}
if bytes.Equal(header, precompileHeader) {
key := p.UnpackStr()
if p.Err != nil {
return p.Err
}
config := p.UnpackBytes()
if p.Err != nil {
return p.Err
}
module, ok := modules.GetPrecompileModule(key)
if !ok {
return ErrUnknowPrecompile
}
preCompile := module.MakeConfig()
err := preCompile.UnmarshalBinary(config)
if err != nil {
return err
}
c.PrecompileUpgrades = append(c.PrecompileUpgrades, PrecompileUpgrade{Config: preCompile})
} else if bytes.Equal(header, stateUpdateHeader) {
config := p.UnpackBytes()
stateUpgrade := StateUpgrade{}
if err := stateUpgrade.UnmarshalBinary(config); err != nil {
return err
}
c.StateUpgrades = append(c.StateUpgrades, stateUpgrade)
} else if bytes.Equal(header, endHeader) {
break
} else {
return ErrMalformedConfigHeader
}
}

return nil
}

// AvalancheContext provides Avalanche specific context directly into the EVM.
type AvalancheContext struct {
SnowCtx *snow.Context
Expand Down Expand Up @@ -865,3 +1018,36 @@ func (c *ChainConfig) ToWithUpgradesJSON() *ChainConfigWithUpgradesJSON {
UpgradeConfig: c.UpgradeConfig,
}
}

// Checks if messages have the same hash
//
// `message` is the simulation of a configuration being parsed from the local
// config. `message2` is parsing a message being exchanged through the network
// (a foreign config), and `message3` is the the deserialization and
// serialization of the foreign config. All 3 instances should have the same
// 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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you re-phrase this comment, it seems like some variable names have changed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I made it way better in 1819327

func AssertConfigHashesAndSerialization(t *testing.T, originalConfig *UpgradeConfig) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

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))
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.


hash1, err := originalConfig.Hash()
require.NoError(t, err)
hash2, err := deserializedConfig.Hash()
require.NoError(t, err)
hash3, err := twiceDeserialized.Hash()
require.NoError(t, err)

require.Equal(t, deserializedConfig, twiceDeserialized)
require.Equal(t, hash1, hash2)
require.Equal(t, hash2, hash3)
}
Loading
Loading