From e501c94598076d989dbffce1f3ff684aef32b50e Mon Sep 17 00:00:00 2001 From: Stephen Buttolph Date: Tue, 11 Feb 2025 13:16:29 -0500 Subject: [PATCH] Refactor `header.Extra` verification (#788) --- consensus/dummy/consensus.go | 25 +++---- consensus/dummy/dynamic_fee_window.go | 2 +- consensus/dummy/dynamic_fee_window_test.go | 2 +- plugin/evm/block_verification.go | 28 +------ plugin/evm/header/extra.go | 49 ++++++++++++ plugin/evm/header/extra_test.go | 87 ++++++++++++++++++++++ 6 files changed, 152 insertions(+), 41 deletions(-) create mode 100644 plugin/evm/header/extra.go create mode 100644 plugin/evm/header/extra_test.go diff --git a/consensus/dummy/consensus.go b/consensus/dummy/consensus.go index 1ab9444f53..3cf65644b4 100644 --- a/consensus/dummy/consensus.go +++ b/consensus/dummy/consensus.go @@ -19,6 +19,8 @@ import ( "github.com/ava-labs/subnet-evm/trie" "github.com/ava-labs/subnet-evm/vmerrs" "github.com/ethereum/go-ethereum/common" + + customheader "github.com/ava-labs/subnet-evm/plugin/evm/header" ) var ( @@ -200,25 +202,18 @@ func (eng *DummyEngine) verifyHeaderGasFields(config *params.ChainConfig, header // modified from consensus.go func (eng *DummyEngine) verifyHeader(chain consensus.ChainHeaderReader, header *types.Header, parent *types.Header, uncle bool) error { - config := chain.Config() // Ensure that we do not verify an uncle if uncle { return errUnclesUnsupported } - switch { - case config.IsDurango(header.Time): - if len(header.Extra) < params.DynamicFeeExtraDataSize { - return fmt.Errorf("expected extra-data field length >= %d, found %d", params.DynamicFeeExtraDataSize, len(header.Extra)) - } - case config.IsSubnetEVM(header.Time): - if len(header.Extra) != params.DynamicFeeExtraDataSize { - return fmt.Errorf("expected extra-data field to be: %d, but found %d", params.DynamicFeeExtraDataSize, len(header.Extra)) - } - default: - if uint64(len(header.Extra)) > params.MaximumExtraDataSize { - return fmt.Errorf("extra-data too long: %d > %d", len(header.Extra), params.MaximumExtraDataSize) - } + + // Verify the extra data is well-formed. + config := chain.Config() + rules := config.GetAvalancheRules(header.Time) + if err := customheader.VerifyExtra(rules, header.Extra); err != nil { + return err } + // Ensure gas-related header fields are correct if err := eng.verifyHeaderGasFields(config, header, parent, chain); err != nil { return err @@ -242,7 +237,7 @@ func (eng *DummyEngine) verifyHeader(chain consensus.ChainHeaderReader, header * return consensus.ErrInvalidNumber } // Verify the existence / non-existence of excessBlobGas - cancun := chain.Config().IsCancun(header.Number, header.Time) + cancun := config.IsCancun(header.Number, header.Time) if !cancun { switch { case header.ExcessBlobGas != nil: diff --git a/consensus/dummy/dynamic_fee_window.go b/consensus/dummy/dynamic_fee_window.go index 8c56ba3a29..8e9cb1c4de 100644 --- a/consensus/dummy/dynamic_fee_window.go +++ b/consensus/dummy/dynamic_fee_window.go @@ -9,7 +9,7 @@ import ( "fmt" "github.com/ava-labs/avalanchego/utils/wrappers" - "github.com/ava-labs/coreth/params" + "github.com/ava-labs/subnet-evm/params" "github.com/ethereum/go-ethereum/common/math" ) diff --git a/consensus/dummy/dynamic_fee_window_test.go b/consensus/dummy/dynamic_fee_window_test.go index b3ec2297e6..f1ac6e1aa0 100644 --- a/consensus/dummy/dynamic_fee_window_test.go +++ b/consensus/dummy/dynamic_fee_window_test.go @@ -7,7 +7,7 @@ import ( "strconv" "testing" - "github.com/ava-labs/coreth/params" + "github.com/ava-labs/subnet-evm/params" "github.com/ethereum/go-ethereum/common/math" "github.com/stretchr/testify/require" ) diff --git a/plugin/evm/block_verification.go b/plugin/evm/block_verification.go index 753fa19b30..4ca35c1555 100644 --- a/plugin/evm/block_verification.go +++ b/plugin/evm/block_verification.go @@ -12,6 +12,7 @@ import ( "github.com/ava-labs/subnet-evm/core/types" "github.com/ava-labs/subnet-evm/params" + "github.com/ava-labs/subnet-evm/plugin/evm/header" "github.com/ava-labs/subnet-evm/trie" ) @@ -58,30 +59,9 @@ func (v blockValidator) SyntacticVerify(b *Block, rules params.Rules) error { return fmt.Errorf("invalid mix digest: %v", ethHeader.MixDigest) } - // Check that the size of the header's Extra data field is correct for [rules]. - headerExtraDataSize := len(ethHeader.Extra) - switch { - case rules.IsDurango: - if headerExtraDataSize < params.DynamicFeeExtraDataSize { - return fmt.Errorf( - "expected header ExtraData to be len >= %d but got %d", - params.DynamicFeeExtraDataSize, len(ethHeader.Extra), - ) - } - case rules.IsSubnetEVM: - if headerExtraDataSize != params.DynamicFeeExtraDataSize { - return fmt.Errorf( - "expected header ExtraData to be len %d but got %d", - params.DynamicFeeExtraDataSize, headerExtraDataSize, - ) - } - default: - if uint64(headerExtraDataSize) > params.MaximumExtraDataSize { - return fmt.Errorf( - "expected header ExtraData to be <= %d but got %d", - params.MaximumExtraDataSize, headerExtraDataSize, - ) - } + // Verify the extra data is well-formed. + if err := header.VerifyExtra(rules.AvalancheRules, ethHeader.Extra); err != nil { + return err } if rules.IsSubnetEVM { diff --git a/plugin/evm/header/extra.go b/plugin/evm/header/extra.go new file mode 100644 index 0000000000..f8c7ab3299 --- /dev/null +++ b/plugin/evm/header/extra.go @@ -0,0 +1,49 @@ +// (c) 2025, Ava Labs, Inc. All rights reserved. +// See the file LICENSE for licensing terms. + +package header + +import ( + "errors" + "fmt" + + "github.com/ava-labs/subnet-evm/params" +) + +var errInvalidExtraLength = errors.New("invalid header.Extra length") + +// VerifyExtra verifies that the header's Extra field is correctly formatted for +// [rules]. +func VerifyExtra(rules params.AvalancheRules, extra []byte) error { + extraLen := len(extra) + switch { + case rules.IsDurango: + if extraLen < params.DynamicFeeExtraDataSize { + return fmt.Errorf( + "%w: expected >= %d but got %d", + errInvalidExtraLength, + params.DynamicFeeExtraDataSize, + extraLen, + ) + } + case rules.IsSubnetEVM: + if extraLen != params.DynamicFeeExtraDataSize { + return fmt.Errorf( + "%w: expected %d but got %d", + errInvalidExtraLength, + params.DynamicFeeExtraDataSize, + extraLen, + ) + } + default: + if uint64(extraLen) > params.MaximumExtraDataSize { + return fmt.Errorf( + "%w: expected <= %d but got %d", + errInvalidExtraLength, + params.MaximumExtraDataSize, + extraLen, + ) + } + } + return nil +} diff --git a/plugin/evm/header/extra_test.go b/plugin/evm/header/extra_test.go new file mode 100644 index 0000000000..41253f9d62 --- /dev/null +++ b/plugin/evm/header/extra_test.go @@ -0,0 +1,87 @@ +// (c) 2025, Ava Labs, Inc. All rights reserved. +// See the file LICENSE for licensing terms. + +package header + +import ( + "testing" + + "github.com/ava-labs/subnet-evm/params" + "github.com/stretchr/testify/require" +) + +func TestVerifyExtra(t *testing.T) { + tests := []struct { + name string + rules params.AvalancheRules + extra []byte + expected error + }{ + { + name: "initial_valid", + rules: params.AvalancheRules{}, + extra: make([]byte, params.MaximumExtraDataSize), + expected: nil, + }, + { + name: "initial_invalid", + rules: params.AvalancheRules{}, + extra: make([]byte, params.MaximumExtraDataSize+1), + expected: errInvalidExtraLength, + }, + { + name: "subnet_evm_valid", + rules: params.AvalancheRules{ + IsSubnetEVM: true, + }, + extra: make([]byte, params.DynamicFeeExtraDataSize), + expected: nil, + }, + { + name: "subnet_invalid_less", + rules: params.AvalancheRules{ + IsSubnetEVM: true, + }, + extra: make([]byte, params.DynamicFeeExtraDataSize-1), + expected: errInvalidExtraLength, + }, + { + name: "subnet_invalid_more", + rules: params.AvalancheRules{ + IsSubnetEVM: true, + }, + extra: make([]byte, params.DynamicFeeExtraDataSize+1), + expected: errInvalidExtraLength, + }, + { + name: "durango_valid_min", + rules: params.AvalancheRules{ + IsDurango: true, + }, + extra: make([]byte, params.DynamicFeeExtraDataSize), + expected: nil, + }, + { + name: "durango_valid_extra", + rules: params.AvalancheRules{ + IsDurango: true, + }, + extra: make([]byte, params.DynamicFeeExtraDataSize+1), + expected: nil, + }, + { + name: "durango_invalid", + rules: params.AvalancheRules{ + IsDurango: true, + }, + extra: make([]byte, params.DynamicFeeExtraDataSize-1), + expected: errInvalidExtraLength, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + err := VerifyExtra(test.rules, test.extra) + require.ErrorIs(t, err, test.expected) + }) + } +}