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 26 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
18 changes: 16 additions & 2 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,9 +28,13 @@ var _ precompileconfig.Config = &Config{}
// adds specific configuration for {{.Contract.Type}}.
type Config struct {
{{- if .Contract.AllowList}}
allowlist.AllowListConfig
// Serialize allowList to let nodes exchange information about their
// precompile configs
nytzuga marked this conversation as resolved.
Show resolved Hide resolved
allowlist.AllowListConfig ` + "`" + `serialize:"true"` + "`" + `
{{- end}}
precompileconfig.Upgrade
// 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 ` + "`" + `serialize:"true"` + "`" + `
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) ToBytes() ([]byte, error) {
return nil, errors.New("implement ToBytes() method")
}

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

// NewDisableConfig returns config for a network upgrade at [blockTimestamp]
// that disables {{.Contract.Type}}.
func NewDisableConfig(blockTimestamp *uint64) *Config {
Expand Down
52 changes: 52 additions & 0 deletions commontype/fee_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"fmt"
"math/big"

"github.com/ava-labs/avalanchego/utils/wrappers"
"github.com/ava-labs/subnet-evm/utils"
"github.com/ethereum/go-ethereum/common"
)
Expand Down Expand Up @@ -144,6 +145,57 @@ 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) ToBytesWithPacker(p *wrappers.Packer) error {
for _, bigint := range c.getBigIntToSerialize() {
p.PackBool(*bigint == nil)
if p.Err != nil {
return p.Err
}
if bigint != nil {
p.PackBytes((*bigint).Bytes())
if p.Err != nil {
return p.Err
}
}
}
p.PackLong(c.TargetBlockRate)
if p.Err != nil {
return p.Err
}

return nil
}

func (c *FeeConfig) FromBytesWithPacker(p *wrappers.Packer) error {
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
72 changes: 72 additions & 0 deletions params/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -328,3 +328,75 @@ func TestChainConfigMarshalWithUpgrades(t *testing.T) {
require.NoError(t, err)
require.Equal(t, config, unmarshalled)
}

func TestChainConfigMarshalWithUpgradesAndOptionalUpgrade(t *testing.T) {
config := ChainConfigWithUpgradesJSON{
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems to test the json marshal/unmarshal, but does not test the new marshal / unmarshal binary code.

We should add test coverage for the binary marshal / unmarshal as well.

ChainConfig: ChainConfig{
ChainID: big.NewInt(1),
FeeConfig: DefaultFeeConfig,
AllowFeeRecipients: false,
HomesteadBlock: big.NewInt(0),
EIP150Block: big.NewInt(0),
EIP155Block: big.NewInt(0),
EIP158Block: big.NewInt(0),
ByzantiumBlock: big.NewInt(0),
ConstantinopleBlock: big.NewInt(0),
PetersburgBlock: big.NewInt(0),
IstanbulBlock: big.NewInt(0),
MuirGlacierBlock: big.NewInt(0),
MandatoryNetworkUpgrades: MandatoryNetworkUpgrades{
SubnetEVMTimestamp: utils.NewUint64(0),
DUpgradeTimestamp: utils.NewUint64(0),
},
GenesisPrecompiles: Precompiles{},
},
UpgradeConfig: UpgradeConfig{
PrecompileUpgrades: []PrecompileUpgrade{
{
Config: txallowlist.NewConfig(utils.NewUint64(100), nil, nil, nil),
},
},
},
}
result, err := json.Marshal(&config)
require.NoError(t, err)
expectedJSON := `{
"chainId": 1,
"feeConfig": {
"gasLimit": 8000000,
"targetBlockRate": 2,
"minBaseFee": 25000000000,
"targetGas": 15000000,
"baseFeeChangeDenominator": 36,
"minBlockGasCost": 0,
"maxBlockGasCost": 1000000,
"blockGasCostStep": 200000
},
"homesteadBlock": 0,
"eip150Block": 0,
"eip155Block": 0,
"eip158Block": 0,
"byzantiumBlock": 0,
"constantinopleBlock": 0,
"petersburgBlock": 0,
"istanbulBlock": 0,
"muirGlacierBlock": 0,
"subnetEVMTimestamp": 0,
"dUpgradeTimestamp": 0,
"upgrades": {
"precompileUpgrades": [
{
"txAllowListConfig": {
"blockTimestamp": 100
}
}
]
}
}`
require.JSONEq(t, expectedJSON, string(result))

var unmarshalled ChainConfigWithUpgradesJSON
err = json.Unmarshal(result, &unmarshalled)
require.NoError(t, err)
require.Equal(t, config, unmarshalled)
}
13 changes: 7 additions & 6 deletions params/network_upgrades.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,16 +64,17 @@ func (m *MandatoryNetworkUpgrades) mandatoryForkOrder() []fork {
}
}

// OptionalNetworkUpgrades includes overridable and optional Subnet-EVM network upgrades.
// These can be specified in genesis and upgrade configs.
// Timestamps can be different for each subnet network.
// TODO: once we add the first optional upgrade here, we should uncomment TestVMUpgradeBytesOptionalNetworkUpgrades
type OptionalNetworkUpgrades struct{}
type OptionalNetworkUpgrades struct {
// This is an example of a configuration.
//FeatureConfig *uint64 `json:"test,omitempty" serialize:"true,nullable"`
}

func (n *OptionalNetworkUpgrades) CheckOptionalCompatible(newcfg *OptionalNetworkUpgrades, time uint64) *ConfigCompatError {
return nil
}

func (n *OptionalNetworkUpgrades) optionalForkOrder() []fork {
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 use the same logic as we do it in mandatoryForkOrder

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The expected output of optionalForkOrder is the same as mandatoryForkOrder. However, to make things less error-prone OptionalFork has a method ToFork that will return the expected struct.

return []fork{}
return []fork{
// {name: "foo", timestamp: n.FooBar},
}
}
10 changes: 5 additions & 5 deletions params/state_upgrade.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,18 @@ import (
// StateUpgrade describes the modifications to be made to the state during
// a state upgrade.
type StateUpgrade struct {
BlockTimestamp *uint64 `json:"blockTimestamp,omitempty"`
BlockTimestamp *uint64 `json:"blockTimestamp,omitempty" serialize:"true"`

// map from account address to the modification to be made to the account.
StateUpgradeAccounts map[common.Address]StateUpgradeAccount `json:"accounts"`
StateUpgradeAccounts map[common.Address]StateUpgradeAccount `json:"accounts" serialize:"true"`
}

// StateUpgradeAccount describes the modifications to be made to an account during
// a state upgrade.
type StateUpgradeAccount struct {
Code hexutil.Bytes `json:"code,omitempty"`
Storage map[common.Hash]common.Hash `json:"storage,omitempty"`
BalanceChange *math.HexOrDecimal256 `json:"balanceChange,omitempty"`
Code hexutil.Bytes `json:"code,omitempty" serialize:"true"`
Storage map[common.Hash]common.Hash `json:"storage,omitempty" serialize:"true"`
BalanceChange *math.HexOrDecimal256 `json:"balanceChange,omitempty" serialize:"true"`
nytzuga marked this conversation as resolved.
Show resolved Hide resolved
}

func (s *StateUpgrade) Equal(other *StateUpgrade) bool {
Expand Down
43 changes: 43 additions & 0 deletions plugin/evm/message/handshake/codec.go
nytzuga marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
// (c) 2019-2023, Ava Labs, Inc. All rights reserved.
// See the file LICENSE for licensing terms.
nytzuga marked this conversation as resolved.
Show resolved Hide resolved

// (c) 2019-2020, Ava Labs, Inc. All rights reserved.
// See the file LICENSE for licensing terms.

package handshake

import (
"errors"

"github.com/ava-labs/avalanchego/codec"
"github.com/ava-labs/avalanchego/codec/linearcodec"
"github.com/ava-labs/avalanchego/utils/units"
"github.com/ava-labs/avalanchego/utils/wrappers"
)

const (
Version = uint16(0)
maxMessageSize = 1 * units.MiB
)

var (
ErrInvalidVersion = errors.New("invalid version")
ErrUnknowPrecompile = errors.New("unknown precompile config")
Codec codec.Manager
)

func init() {
Codec = codec.NewManager(maxMessageSize)
c := linearcodec.NewDefault()

errs := wrappers.Errs{}
errs.Add(
c.RegisterType(networkUpgradeConfigMessage{}),

Codec.RegisterCodec(Version, c),
)

if errs.Errored() {
panic(errs.Err)
}
}
115 changes: 115 additions & 0 deletions plugin/evm/message/handshake/upgrade_config_message.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
// (c) 2019-2020, Ava Labs, Inc. All rights reserved.
// See the file LICENSE for licensing terms.

package handshake

import (
"github.com/ava-labs/subnet-evm/params"
"github.com/ava-labs/subnet-evm/precompile/modules"
"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/crypto"
)

type rawPrecompileUpgrade struct {
Key string `serialize:"true"`
Bytes []byte `serialize:"true"`
}

type networkUpgradeConfigMessage struct {
OptionalNetworkUpgrades *params.OptionalNetworkUpgrades `serialize:"true,nullable"`
// Config for modifying state as a network upgrade.
StateUpgrades []params.StateUpgrade `serialize:"true"`
// Config for enabling and disabling precompiles as network upgrades.
PrecompileUpgrades []rawPrecompileUpgrade `serialize:"true"`
}

type UpgradeConfigMessage struct {
bytes []byte
hash common.Hash
}

func (u *UpgradeConfigMessage) Bytes() []byte {
return u.bytes
}

func (u *UpgradeConfigMessage) ID() common.Hash {
return u.hash
}

// Attempts to parse a networkUpgradeConfigMessage from a []byte
//
// This function attempts to parse a stream of bytes as a
// networkUpgradeConfigMessage (as serialized from
// UpgradeConfigToNetworkMessage).
//
// The function returns a reference of *params.UpgradeConfig
func NewUpgradeConfigMessageFromBytes(bytes []byte) (*params.UpgradeConfig, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this returning *params.UpgradeConfig instead of UpgradeConfigMessage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

UpgradeConfigMessage is the struct to represent the bytes, it is an intermediary struct. The real struct that is used everywhere else in the code is params .UpgradeConfig

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 rename the function to reflect that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the name better now @ceyonur ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ceyonur I think now it is better. I dropped all the intermediary types and I added MarshalBinary, UnmarshalBinary and Hash to the UpgradeConfig struct. That way it is cleaner and much simpler.

I'm working now to re-structure the tests and add more coverage.

var config networkUpgradeConfigMessage
version, err := Codec.Unmarshal(bytes, &config)
if err != nil {
return nil, err
}
if version != Version {
return nil, ErrInvalidVersion
}

var PrecompileUpgrades []params.PrecompileUpgrade
for _, precompileUpgrade := range config.PrecompileUpgrades {
module, ok := modules.GetPrecompileModule(precompileUpgrade.Key)
if !ok {
return nil, ErrUnknowPrecompile
}
preCompile := module.MakeConfig()
err := preCompile.FromBytes(precompileUpgrade.Bytes)
if err != nil {
return nil, err
}
PrecompileUpgrades = append(PrecompileUpgrades, params.PrecompileUpgrade{Config: preCompile})
}

return &params.UpgradeConfig{
OptionalNetworkUpgrades: config.OptionalNetworkUpgrades,
StateUpgrades: config.StateUpgrades,
PrecompileUpgrades: PrecompileUpgrades,
}, nil
}

// Wraps an instance of *params.UpgradeConfig
//
// This function returns the serialized UpgradeConfig, ready to be send over to
// other peers. The struct also includes a hash of the content, ready to be used
// as part of the handshake protocol.
//
// Since params.UpgradeConfig should never change without a node reloading, it
// is safe to call this function once and store its output globally to re-use
// multiple times
func NewUpgradeConfigMessage(config *params.UpgradeConfig) (*UpgradeConfigMessage, error) {
PrecompileUpgrades := make([]rawPrecompileUpgrade, 0)
for _, precompileConfig := range config.PrecompileUpgrades {
bytes, err := precompileConfig.Config.ToBytes()
if err != nil {
return nil, err
}
PrecompileUpgrades = append(PrecompileUpgrades, rawPrecompileUpgrade{
Key: precompileConfig.Key(),
Bytes: bytes,
})
}

wrappedConfig := networkUpgradeConfigMessage{
OptionalNetworkUpgrades: config.OptionalNetworkUpgrades,
StateUpgrades: config.StateUpgrades,
PrecompileUpgrades: PrecompileUpgrades,
}

bytes, err := Codec.Marshal(Version, wrappedConfig)
if err != nil {
return nil, err
}

hash := crypto.Keccak256Hash(bytes)
return &UpgradeConfigMessage{
bytes: bytes,
hash: hash,
}, nil
}
Loading
Loading