From c982c0c43aabcbe5cbda8df9998dd44f8d93b281 Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Thu, 21 Jul 2022 18:04:01 +0200 Subject: [PATCH 1/4] core/types: make ErrInvalidChainID more useful --- core/blockchain_test.go | 2 +- core/types/transaction_signing.go | 36 +++++++++++++++++++++----- core/types/transaction_signing_test.go | 5 ++-- core/types/transaction_test.go | 15 ++++++----- 4 files changed, 42 insertions(+), 16 deletions(-) diff --git a/core/blockchain_test.go b/core/blockchain_test.go index c1e1d6371e0..4fbf411a585 100644 --- a/core/blockchain_test.go +++ b/core/blockchain_test.go @@ -1512,7 +1512,7 @@ func TestEIP155Transition(t *testing.T) { } }) _, err := blockchain.InsertChain(blocks) - if have, want := err, types.ErrInvalidChainId; !errors.Is(have, want) { + if have, want := err, types.NewInvalidChainIdError(big.NewInt(2)); !errors.Is(have, want) { t.Errorf("have %v, want %v", have, want) } } diff --git a/core/types/transaction_signing.go b/core/types/transaction_signing.go index 1d0d2a4c75e..0a6ab088359 100644 --- a/core/types/transaction_signing.go +++ b/core/types/transaction_signing.go @@ -27,7 +27,31 @@ import ( "github.com/ethereum/go-ethereum/params" ) -var ErrInvalidChainId = errors.New("invalid chain id for signer") +func NewInvalidChainIdError(id *big.Int) error { + return &ErrInvalidChainID{id} +} + +type ErrInvalidChainID struct { + id *big.Int +} + +func (e ErrInvalidChainID) Error() string { + if e.id == nil { + return "invalid chain id for signer" + } + return fmt.Sprintf("invalid chain id for signer: %v", e.id) +} + +func (e ErrInvalidChainID) Is(target error) bool { + t, ok := target.(*ErrInvalidChainID) + if !ok { + return false + } + if e.id == nil || t.id == nil { + return e.id == t.id + } + return e.id.Cmp(t.id) == 0 +} // sigCache is used to cache the derived sender and contains // the signer used to derive it. @@ -190,7 +214,7 @@ func (s londonSigner) Sender(tx *Transaction) (common.Address, error) { // id, add 27 to become equivalent to unprotected Homestead signatures. V = new(big.Int).Add(V, big.NewInt(27)) if tx.ChainId().Cmp(s.chainId) != 0 { - return common.Address{}, ErrInvalidChainId + return common.Address{}, NewInvalidChainIdError(V) } return recoverPlain(s.Hash(tx), R, S, V, true) } @@ -208,7 +232,7 @@ func (s londonSigner) SignatureValues(tx *Transaction, sig []byte) (R, S, V *big // Check that chain ID of tx matches the signer. We also accept ID zero here, // because it indicates that the chain ID was not specified in the tx. if txdata.ChainID.Sign() != 0 && txdata.ChainID.Cmp(s.chainId) != 0 { - return nil, nil, nil, ErrInvalidChainId + return nil, nil, nil, NewInvalidChainIdError(txdata.ChainID) } R, S, _ = decodeSignature(sig) V = big.NewInt(int64(sig[64])) @@ -270,7 +294,7 @@ func (s eip2930Signer) Sender(tx *Transaction) (common.Address, error) { return common.Address{}, ErrTxTypeNotSupported } if tx.ChainId().Cmp(s.chainId) != 0 { - return common.Address{}, ErrInvalidChainId + return common.Address{}, NewInvalidChainIdError(tx.ChainId()) } return recoverPlain(s.Hash(tx), R, S, V, true) } @@ -283,7 +307,7 @@ func (s eip2930Signer) SignatureValues(tx *Transaction, sig []byte) (R, S, V *bi // Check that chain ID of tx matches the signer. We also accept ID zero here, // because it indicates that the chain ID was not specified in the tx. if txdata.ChainID.Sign() != 0 && txdata.ChainID.Cmp(s.chainId) != 0 { - return nil, nil, nil, ErrInvalidChainId + return nil, nil, nil, NewInvalidChainIdError(txdata.ChainID) } R, S, _ = decodeSignature(sig) V = big.NewInt(int64(sig[64])) @@ -364,7 +388,7 @@ func (s EIP155Signer) Sender(tx *Transaction) (common.Address, error) { return HomesteadSigner{}.Sender(tx) } if tx.ChainId().Cmp(s.chainId) != 0 { - return common.Address{}, ErrInvalidChainId + return common.Address{}, NewInvalidChainIdError(tx.ChainId()) } V, R, S := tx.RawSignatureValues() V = new(big.Int).Sub(V, s.chainIdMul) diff --git a/core/types/transaction_signing_test.go b/core/types/transaction_signing_test.go index 689fc38a9b6..db0cb88aade 100644 --- a/core/types/transaction_signing_test.go +++ b/core/types/transaction_signing_test.go @@ -17,6 +17,7 @@ package types import ( + "errors" "math/big" "testing" @@ -127,8 +128,8 @@ func TestChainId(t *testing.T) { } _, err = Sender(NewEIP155Signer(big.NewInt(2)), tx) - if err != ErrInvalidChainId { - t.Error("expected error:", ErrInvalidChainId) + if !errors.Is(err, NewInvalidChainIdError(big.NewInt(1))) { + t.Error("expected error:", NewInvalidChainIdError(big.NewInt(1)), err) } _, err = Sender(NewEIP155Signer(big.NewInt(1)), tx) diff --git a/core/types/transaction_test.go b/core/types/transaction_test.go index 2e418b23098..2106c8c2823 100644 --- a/core/types/transaction_test.go +++ b/core/types/transaction_test.go @@ -20,6 +20,7 @@ import ( "bytes" "crypto/ecdsa" "encoding/json" + "errors" "fmt" "math/big" "math/rand" @@ -137,7 +138,7 @@ func TestEIP2930Signer(t *testing.T) { tx: tx0, signer: signer1, wantSignerHash: common.HexToHash("846ad7672f2a3a40c1f959cd4a8ad21786d620077084d84c8d7c077714caa139"), - wantSenderErr: ErrInvalidChainId, + wantSenderErr: NewInvalidChainIdError(big.NewInt(0)), wantHash: common.HexToHash("1ccd12d8bbdb96ea391af49a35ab641e219b2dd638dea375f2bc94dd290f2549"), }, { @@ -151,17 +152,17 @@ func TestEIP2930Signer(t *testing.T) { // This checks what happens when trying to sign an unsigned tx for the wrong chain. tx: tx1, signer: signer2, - wantSenderErr: ErrInvalidChainId, + wantSenderErr: NewInvalidChainIdError(big.NewInt(1)), wantSignerHash: common.HexToHash("367967247499343401261d718ed5aa4c9486583e4d89251afce47f4a33c33362"), - wantSignErr: ErrInvalidChainId, + wantSignErr: NewInvalidChainIdError(big.NewInt(1)), }, { // This checks what happens when trying to re-sign a signed tx for the wrong chain. tx: tx2, signer: signer1, - wantSenderErr: ErrInvalidChainId, + wantSenderErr: NewInvalidChainIdError(big.NewInt(2)), wantSignerHash: common.HexToHash("846ad7672f2a3a40c1f959cd4a8ad21786d620077084d84c8d7c077714caa139"), - wantSignErr: ErrInvalidChainId, + wantSignErr: NewInvalidChainIdError(big.NewInt(2)), }, } @@ -171,14 +172,14 @@ func TestEIP2930Signer(t *testing.T) { t.Errorf("test %d: wrong sig hash: got %x, want %x", i, sigHash, test.wantSignerHash) } sender, err := Sender(test.signer, test.tx) - if err != test.wantSenderErr { + if !errors.Is(err, test.wantSenderErr) { t.Errorf("test %d: wrong Sender error %q", i, err) } if err == nil && sender != keyAddr { t.Errorf("test %d: wrong sender address %x", i, sender) } signedTx, err := SignTx(test.tx, test.signer, key) - if err != test.wantSignErr { + if !errors.Is(err, test.wantSignErr) { t.Fatalf("test %d: wrong SignTx error %q", i, err) } if signedTx != nil { From 858b9b2b0ba3c6ab2cdd0e97966e1d4491aa4e58 Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Tue, 6 Sep 2022 13:02:52 +0200 Subject: [PATCH 2/4] core/types: less overcomplicated errors --- core/types/transaction_signing.go | 36 +++++--------------------- core/types/transaction_signing_test.go | 4 +-- core/types/transaction_test.go | 10 +++---- 3 files changed, 13 insertions(+), 37 deletions(-) diff --git a/core/types/transaction_signing.go b/core/types/transaction_signing.go index 0a6ab088359..af3894032d3 100644 --- a/core/types/transaction_signing.go +++ b/core/types/transaction_signing.go @@ -27,31 +27,7 @@ import ( "github.com/ethereum/go-ethereum/params" ) -func NewInvalidChainIdError(id *big.Int) error { - return &ErrInvalidChainID{id} -} - -type ErrInvalidChainID struct { - id *big.Int -} - -func (e ErrInvalidChainID) Error() string { - if e.id == nil { - return "invalid chain id for signer" - } - return fmt.Sprintf("invalid chain id for signer: %v", e.id) -} - -func (e ErrInvalidChainID) Is(target error) bool { - t, ok := target.(*ErrInvalidChainID) - if !ok { - return false - } - if e.id == nil || t.id == nil { - return e.id == t.id - } - return e.id.Cmp(t.id) == 0 -} +var ErrInvalidChainId = errors.New("invalid chain id for signer") // sigCache is used to cache the derived sender and contains // the signer used to derive it. @@ -214,7 +190,7 @@ func (s londonSigner) Sender(tx *Transaction) (common.Address, error) { // id, add 27 to become equivalent to unprotected Homestead signatures. V = new(big.Int).Add(V, big.NewInt(27)) if tx.ChainId().Cmp(s.chainId) != 0 { - return common.Address{}, NewInvalidChainIdError(V) + return common.Address{}, fmt.Errorf("%w: %d", ErrInvalidChainId, V) } return recoverPlain(s.Hash(tx), R, S, V, true) } @@ -232,7 +208,7 @@ func (s londonSigner) SignatureValues(tx *Transaction, sig []byte) (R, S, V *big // Check that chain ID of tx matches the signer. We also accept ID zero here, // because it indicates that the chain ID was not specified in the tx. if txdata.ChainID.Sign() != 0 && txdata.ChainID.Cmp(s.chainId) != 0 { - return nil, nil, nil, NewInvalidChainIdError(txdata.ChainID) + return nil, nil, nil, fmt.Errorf("%w: %d", ErrInvalidChainId, txdata.chainID()) } R, S, _ = decodeSignature(sig) V = big.NewInt(int64(sig[64])) @@ -294,7 +270,7 @@ func (s eip2930Signer) Sender(tx *Transaction) (common.Address, error) { return common.Address{}, ErrTxTypeNotSupported } if tx.ChainId().Cmp(s.chainId) != 0 { - return common.Address{}, NewInvalidChainIdError(tx.ChainId()) + return common.Address{}, fmt.Errorf("%w: %d", ErrInvalidChainId, tx.ChainId()) } return recoverPlain(s.Hash(tx), R, S, V, true) } @@ -307,7 +283,7 @@ func (s eip2930Signer) SignatureValues(tx *Transaction, sig []byte) (R, S, V *bi // Check that chain ID of tx matches the signer. We also accept ID zero here, // because it indicates that the chain ID was not specified in the tx. if txdata.ChainID.Sign() != 0 && txdata.ChainID.Cmp(s.chainId) != 0 { - return nil, nil, nil, NewInvalidChainIdError(txdata.ChainID) + return nil, nil, nil, fmt.Errorf("%w: %d", ErrInvalidChainId, txdata.chainID()) } R, S, _ = decodeSignature(sig) V = big.NewInt(int64(sig[64])) @@ -388,7 +364,7 @@ func (s EIP155Signer) Sender(tx *Transaction) (common.Address, error) { return HomesteadSigner{}.Sender(tx) } if tx.ChainId().Cmp(s.chainId) != 0 { - return common.Address{}, NewInvalidChainIdError(tx.ChainId()) + return common.Address{}, fmt.Errorf("%w: %d", ErrInvalidChainId, tx.ChainId()) } V, R, S := tx.RawSignatureValues() V = new(big.Int).Sub(V, s.chainIdMul) diff --git a/core/types/transaction_signing_test.go b/core/types/transaction_signing_test.go index db0cb88aade..e392802c5c0 100644 --- a/core/types/transaction_signing_test.go +++ b/core/types/transaction_signing_test.go @@ -128,8 +128,8 @@ func TestChainId(t *testing.T) { } _, err = Sender(NewEIP155Signer(big.NewInt(2)), tx) - if !errors.Is(err, NewInvalidChainIdError(big.NewInt(1))) { - t.Error("expected error:", NewInvalidChainIdError(big.NewInt(1)), err) + if !errors.Is(err, ErrInvalidChainId) { + t.Error("expected error:", ErrInvalidChainId, err) } _, err = Sender(NewEIP155Signer(big.NewInt(1)), tx) diff --git a/core/types/transaction_test.go b/core/types/transaction_test.go index 2106c8c2823..d9bd79ef40c 100644 --- a/core/types/transaction_test.go +++ b/core/types/transaction_test.go @@ -138,7 +138,7 @@ func TestEIP2930Signer(t *testing.T) { tx: tx0, signer: signer1, wantSignerHash: common.HexToHash("846ad7672f2a3a40c1f959cd4a8ad21786d620077084d84c8d7c077714caa139"), - wantSenderErr: NewInvalidChainIdError(big.NewInt(0)), + wantSenderErr: ErrInvalidChainId, wantHash: common.HexToHash("1ccd12d8bbdb96ea391af49a35ab641e219b2dd638dea375f2bc94dd290f2549"), }, { @@ -152,17 +152,17 @@ func TestEIP2930Signer(t *testing.T) { // This checks what happens when trying to sign an unsigned tx for the wrong chain. tx: tx1, signer: signer2, - wantSenderErr: NewInvalidChainIdError(big.NewInt(1)), + wantSenderErr: ErrInvalidChainId, wantSignerHash: common.HexToHash("367967247499343401261d718ed5aa4c9486583e4d89251afce47f4a33c33362"), - wantSignErr: NewInvalidChainIdError(big.NewInt(1)), + wantSignErr: ErrInvalidChainId, }, { // This checks what happens when trying to re-sign a signed tx for the wrong chain. tx: tx2, signer: signer1, - wantSenderErr: NewInvalidChainIdError(big.NewInt(2)), + wantSenderErr: ErrInvalidChainId, wantSignerHash: common.HexToHash("846ad7672f2a3a40c1f959cd4a8ad21786d620077084d84c8d7c077714caa139"), - wantSignErr: NewInvalidChainIdError(big.NewInt(2)), + wantSignErr: ErrInvalidChainId, }, } From 180d33cfdf37a458fab64d7b7746f48b2817d193 Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Wed, 7 Sep 2022 08:26:21 +0200 Subject: [PATCH 3/4] core: fix tests --- core/blockchain_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/blockchain_test.go b/core/blockchain_test.go index 4fbf411a585..c1e1d6371e0 100644 --- a/core/blockchain_test.go +++ b/core/blockchain_test.go @@ -1512,7 +1512,7 @@ func TestEIP155Transition(t *testing.T) { } }) _, err := blockchain.InsertChain(blocks) - if have, want := err, types.NewInvalidChainIdError(big.NewInt(2)); !errors.Is(have, want) { + if have, want := err, types.ErrInvalidChainId; !errors.Is(have, want) { t.Errorf("have %v, want %v", have, want) } } From ea6609a996d06908731283bd71b3ecc09eff45b0 Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Wed, 7 Sep 2022 08:47:46 +0200 Subject: [PATCH 4/4] core/types: better error output --- core/types/transaction_signing.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/core/types/transaction_signing.go b/core/types/transaction_signing.go index af3894032d3..f42d031d639 100644 --- a/core/types/transaction_signing.go +++ b/core/types/transaction_signing.go @@ -190,7 +190,7 @@ func (s londonSigner) Sender(tx *Transaction) (common.Address, error) { // id, add 27 to become equivalent to unprotected Homestead signatures. V = new(big.Int).Add(V, big.NewInt(27)) if tx.ChainId().Cmp(s.chainId) != 0 { - return common.Address{}, fmt.Errorf("%w: %d", ErrInvalidChainId, V) + return common.Address{}, fmt.Errorf("%w: have %d want %d", ErrInvalidChainId, tx.ChainId(), s.chainId) } return recoverPlain(s.Hash(tx), R, S, V, true) } @@ -208,7 +208,7 @@ func (s londonSigner) SignatureValues(tx *Transaction, sig []byte) (R, S, V *big // Check that chain ID of tx matches the signer. We also accept ID zero here, // because it indicates that the chain ID was not specified in the tx. if txdata.ChainID.Sign() != 0 && txdata.ChainID.Cmp(s.chainId) != 0 { - return nil, nil, nil, fmt.Errorf("%w: %d", ErrInvalidChainId, txdata.chainID()) + return nil, nil, nil, fmt.Errorf("%w: have %d want %d", ErrInvalidChainId, txdata.ChainID, s.chainId) } R, S, _ = decodeSignature(sig) V = big.NewInt(int64(sig[64])) @@ -270,7 +270,7 @@ func (s eip2930Signer) Sender(tx *Transaction) (common.Address, error) { return common.Address{}, ErrTxTypeNotSupported } if tx.ChainId().Cmp(s.chainId) != 0 { - return common.Address{}, fmt.Errorf("%w: %d", ErrInvalidChainId, tx.ChainId()) + return common.Address{}, fmt.Errorf("%w: have %d want %d", ErrInvalidChainId, tx.ChainId(), s.chainId) } return recoverPlain(s.Hash(tx), R, S, V, true) } @@ -283,7 +283,7 @@ func (s eip2930Signer) SignatureValues(tx *Transaction, sig []byte) (R, S, V *bi // Check that chain ID of tx matches the signer. We also accept ID zero here, // because it indicates that the chain ID was not specified in the tx. if txdata.ChainID.Sign() != 0 && txdata.ChainID.Cmp(s.chainId) != 0 { - return nil, nil, nil, fmt.Errorf("%w: %d", ErrInvalidChainId, txdata.chainID()) + return nil, nil, nil, fmt.Errorf("%w: have %d want %d", ErrInvalidChainId, txdata.ChainID, s.chainId) } R, S, _ = decodeSignature(sig) V = big.NewInt(int64(sig[64])) @@ -364,7 +364,7 @@ func (s EIP155Signer) Sender(tx *Transaction) (common.Address, error) { return HomesteadSigner{}.Sender(tx) } if tx.ChainId().Cmp(s.chainId) != 0 { - return common.Address{}, fmt.Errorf("%w: %d", ErrInvalidChainId, tx.ChainId()) + return common.Address{}, fmt.Errorf("%w: have %d want %d", ErrInvalidChainId, tx.ChainId(), s.chainId) } V, R, S := tx.RawSignatureValues() V = new(big.Int).Sub(V, s.chainIdMul)