From 2cd6059e51e823012f756112e6e9b2d2920bab74 Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Tue, 21 May 2019 13:58:06 +0200 Subject: [PATCH] tests: make transaction tests run again, fix #19033 (#19529) * tests: make transaction tests run again, fix #19033 * tests: refactor transaction tests --- tests/gen_tttransaction.go | 95 -------------------- tests/transaction_test.go | 38 ++++---- tests/transaction_test_util.go | 158 ++++++++++++++------------------- 3 files changed, 86 insertions(+), 205 deletions(-) delete mode 100644 tests/gen_tttransaction.go diff --git a/tests/gen_tttransaction.go b/tests/gen_tttransaction.go deleted file mode 100644 index 2948842d9..000000000 --- a/tests/gen_tttransaction.go +++ /dev/null @@ -1,95 +0,0 @@ -// Code generated by github.com/fjl/gencodec. DO NOT EDIT. - -package tests - -import ( - "encoding/json" - "errors" - "math/big" - - "github.com/ethereum/go-ethereum/common" - "github.com/ethereum/go-ethereum/common/hexutil" - "github.com/ethereum/go-ethereum/common/math" -) - -var _ = (*ttTransactionMarshaling)(nil) - -func (t ttTransaction) MarshalJSON() ([]byte, error) { - type ttTransaction struct { - Data hexutil.Bytes `gencodec:"required"` - GasLimit math.HexOrDecimal64 `gencodec:"required"` - GasPrice *math.HexOrDecimal256 `gencodec:"required"` - Nonce math.HexOrDecimal64 `gencodec:"required"` - Value *math.HexOrDecimal256 `gencodec:"required"` - R *math.HexOrDecimal256 `gencodec:"required"` - S *math.HexOrDecimal256 `gencodec:"required"` - V *math.HexOrDecimal256 `gencodec:"required"` - To common.Address `gencodec:"required"` - } - var enc ttTransaction - enc.Data = t.Data - enc.GasLimit = math.HexOrDecimal64(t.GasLimit) - enc.GasPrice = (*math.HexOrDecimal256)(t.GasPrice) - enc.Nonce = math.HexOrDecimal64(t.Nonce) - enc.Value = (*math.HexOrDecimal256)(t.Value) - enc.R = (*math.HexOrDecimal256)(t.R) - enc.S = (*math.HexOrDecimal256)(t.S) - enc.V = (*math.HexOrDecimal256)(t.V) - enc.To = t.To - return json.Marshal(&enc) -} - -func (t *ttTransaction) UnmarshalJSON(input []byte) error { - type ttTransaction struct { - Data *hexutil.Bytes `gencodec:"required"` - GasLimit *math.HexOrDecimal64 `gencodec:"required"` - GasPrice *math.HexOrDecimal256 `gencodec:"required"` - Nonce *math.HexOrDecimal64 `gencodec:"required"` - Value *math.HexOrDecimal256 `gencodec:"required"` - R *math.HexOrDecimal256 `gencodec:"required"` - S *math.HexOrDecimal256 `gencodec:"required"` - V *math.HexOrDecimal256 `gencodec:"required"` - To *common.Address `gencodec:"required"` - } - var dec ttTransaction - if err := json.Unmarshal(input, &dec); err != nil { - return err - } - if dec.Data == nil { - return errors.New("missing required field 'data' for ttTransaction") - } - t.Data = *dec.Data - if dec.GasLimit == nil { - return errors.New("missing required field 'gasLimit' for ttTransaction") - } - t.GasLimit = uint64(*dec.GasLimit) - if dec.GasPrice == nil { - return errors.New("missing required field 'gasPrice' for ttTransaction") - } - t.GasPrice = (*big.Int)(dec.GasPrice) - if dec.Nonce == nil { - return errors.New("missing required field 'nonce' for ttTransaction") - } - t.Nonce = uint64(*dec.Nonce) - if dec.Value == nil { - return errors.New("missing required field 'value' for ttTransaction") - } - t.Value = (*big.Int)(dec.Value) - if dec.R == nil { - return errors.New("missing required field 'r' for ttTransaction") - } - t.R = (*big.Int)(dec.R) - if dec.S == nil { - return errors.New("missing required field 's' for ttTransaction") - } - t.S = (*big.Int)(dec.S) - if dec.V == nil { - return errors.New("missing required field 'v' for ttTransaction") - } - t.V = (*big.Int)(dec.V) - if dec.To == nil { - return errors.New("missing required field 'to' for ttTransaction") - } - t.To = *dec.To - return nil -} diff --git a/tests/transaction_test.go b/tests/transaction_test.go index 42ad81877..0e3670d04 100644 --- a/tests/transaction_test.go +++ b/tests/transaction_test.go @@ -17,7 +17,6 @@ package tests import ( - "math/big" "testing" "github.com/ethereum/go-ethereum/params" @@ -27,26 +26,27 @@ func TestTransaction(t *testing.T) { t.Parallel() txt := new(testMatcher) - txt.config(`^Homestead/`, params.ChainConfig{ - HomesteadBlock: big.NewInt(0), - }) - txt.config(`^EIP155/`, params.ChainConfig{ - HomesteadBlock: big.NewInt(0), - EIP150Block: big.NewInt(0), - EIP155Block: big.NewInt(0), - EIP158Block: big.NewInt(0), - ChainID: big.NewInt(1), - }) - txt.config(`^Byzantium/`, params.ChainConfig{ - HomesteadBlock: big.NewInt(0), - EIP150Block: big.NewInt(0), - EIP155Block: big.NewInt(0), - EIP158Block: big.NewInt(0), - ByzantiumBlock: big.NewInt(0), - }) + // These can't be parsed, invalid hex in RLP + txt.skipLoad("^ttWrongRLP/.*") + // We don't allow more than uint64 in gas amount + // This is a pseudo-consensus vulnerability, but not in practice + // because of the gas limit + txt.skipLoad("^ttGasLimit/TransactionWithGasLimitxPriceOverflow.json") + // We _do_ allow more than uint64 in gas price, as opposed to the tests + // This is also not a concern, as long as tx.Cost() uses big.Int for + // calculating the final cozt + txt.skipLoad(".*TransactionWithGasPriceOverflow.*") + // The nonce is too large for uint64. Not a concern, it means geth won't + // accept transactions at a certain point in the distant future + txt.skipLoad("^ttNonce/TransactionWithHighNonce256.json") + + // The value is larger than uint64, which according to the test is invalid. + // Geth accepts it, which is not a consensus issue since we use big.Int's + // internally to calculate the cost + txt.skipLoad("^ttValue/TransactionWithHighValueOverflow.json") txt.walk(t, transactionTestDir, func(t *testing.T, name string, test *TransactionTest) { - cfg := txt.findConfig(name) + cfg := params.MainnetChainConfig if err := txt.checkFailure(t, name, test.Run(cfg)); err != nil { t.Error(err) } diff --git a/tests/transaction_test_util.go b/tests/transaction_test_util.go index 8c3dac088..12444720c 100644 --- a/tests/transaction_test_util.go +++ b/tests/transaction_test_util.go @@ -17,14 +17,11 @@ package tests import ( - "bytes" - "errors" "fmt" - "math/big" "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/common/hexutil" - "github.com/ethereum/go-ethereum/common/math" + "github.com/ethereum/go-ethereum/core" "github.com/ethereum/go-ethereum/core/types" "github.com/ethereum/go-ethereum/params" "github.com/ethereum/go-ethereum/rlp" @@ -32,101 +29,80 @@ import ( // TransactionTest checks RLP decoding and sender derivation of transactions. type TransactionTest struct { - json ttJSON + RLP hexutil.Bytes `json:"rlp"` + Byzantium ttFork + Constantinople ttFork + EIP150 ttFork + EIP158 ttFork + Frontier ttFork + Homestead ttFork } -type ttJSON struct { - BlockNumber math.HexOrDecimal64 `json:"blockNumber"` - RLP hexutil.Bytes `json:"rlp"` - Sender hexutil.Bytes `json:"sender"` - Transaction *ttTransaction `json:"transaction"` -} - -//go:generate gencodec -type ttTransaction -field-override ttTransactionMarshaling -out gen_tttransaction.go - -type ttTransaction struct { - Data []byte `gencodec:"required"` - GasLimit uint64 `gencodec:"required"` - GasPrice *big.Int `gencodec:"required"` - Nonce uint64 `gencodec:"required"` - Value *big.Int `gencodec:"required"` - R *big.Int `gencodec:"required"` - S *big.Int `gencodec:"required"` - V *big.Int `gencodec:"required"` - To common.Address `gencodec:"required"` -} - -type ttTransactionMarshaling struct { - Data hexutil.Bytes - GasLimit math.HexOrDecimal64 - GasPrice *math.HexOrDecimal256 - Nonce math.HexOrDecimal64 - Value *math.HexOrDecimal256 - R *math.HexOrDecimal256 - S *math.HexOrDecimal256 - V *math.HexOrDecimal256 +type ttFork struct { + Sender common.UnprefixedAddress `json:"sender"` + Hash common.UnprefixedHash `json:"hash"` } func (tt *TransactionTest) Run(config *params.ChainConfig) error { - tx := new(types.Transaction) - if err := rlp.DecodeBytes(tt.json.RLP, tx); err != nil { - if tt.json.Transaction == nil { - return nil - } - return fmt.Errorf("RLP decoding failed: %v", err) - } - // Check sender derivation. - signer := types.MakeSigner(config, new(big.Int).SetUint64(uint64(tt.json.BlockNumber))) - sender, err := types.Sender(signer, tx) - if err != nil { - return err - } - if sender != common.BytesToAddress(tt.json.Sender) { - return fmt.Errorf("Sender mismatch: got %x, want %x", sender, tt.json.Sender) - } - // Check decoded fields. - err = tt.json.Transaction.verify(signer, tx) - if tt.json.Sender == nil && err == nil { - return errors.New("field validations succeeded but should fail") - } - if tt.json.Sender != nil && err != nil { - return fmt.Errorf("field validations failed after RLP decoding: %s", err) - } - return nil -} -func (tt *ttTransaction) verify(signer types.Signer, tx *types.Transaction) error { - if !bytes.Equal(tx.Data(), tt.Data) { - return fmt.Errorf("Tx input data mismatch: got %x want %x", tx.Data(), tt.Data) - } - if tx.Gas() != tt.GasLimit { - return fmt.Errorf("GasLimit mismatch: got %d, want %d", tx.Gas(), tt.GasLimit) - } - if tx.GasPrice().Cmp(tt.GasPrice) != 0 { - return fmt.Errorf("GasPrice mismatch: got %v, want %v", tx.GasPrice(), tt.GasPrice) - } - if tx.Nonce() != tt.Nonce { - return fmt.Errorf("Nonce mismatch: got %v, want %v", tx.Nonce(), tt.Nonce) - } - v, r, s := tx.RawSignatureValues() - if r.Cmp(tt.R) != 0 { - return fmt.Errorf("R mismatch: got %v, want %v", r, tt.R) - } - if s.Cmp(tt.S) != 0 { - return fmt.Errorf("S mismatch: got %v, want %v", s, tt.S) - } - if v.Cmp(tt.V) != 0 { - return fmt.Errorf("V mismatch: got %v, want %v", v, tt.V) - } - if tx.To() == nil { - if tt.To != (common.Address{}) { - return fmt.Errorf("To mismatch when recipient is nil (contract creation): %x", tt.To) + validateTx := func(rlpData hexutil.Bytes, signer types.Signer, isHomestead bool) (*common.Address, *common.Hash, error) { + tx := new(types.Transaction) + if err := rlp.DecodeBytes(rlpData, tx); err != nil { + return nil, nil, err } - } else if *tx.To() != tt.To { - return fmt.Errorf("To mismatch: got %x, want %x", *tx.To(), tt.To) + sender, err := types.Sender(signer, tx) + if err != nil { + return nil, nil, err + } + // Intrinsic gas + requiredGas, err := core.IntrinsicGas(tx.Data(), tx.To() == nil, isHomestead) + if err != nil { + return nil, nil, err + } + if requiredGas > tx.Gas() { + return nil, nil, fmt.Errorf("insufficient gas ( %d < %d )", tx.Gas(), requiredGas) + } + h := tx.Hash() + return &sender, &h, nil } - if tx.Value().Cmp(tt.Value) != 0 { - return fmt.Errorf("Value mismatch: got %x, want %x", tx.Value(), tt.Value) + + for _, testcase := range []struct { + name string + signer types.Signer + fork ttFork + isHomestead bool + }{ + {"Frontier", types.FrontierSigner{}, tt.Frontier, false}, + {"Homestead", types.HomesteadSigner{}, tt.Homestead, true}, + {"EIP150", types.HomesteadSigner{}, tt.EIP150, true}, + {"EIP158", types.NewEIP155Signer(config.ChainID), tt.EIP158, true}, + {"Byzantium", types.NewEIP155Signer(config.ChainID), tt.Byzantium, true}, + {"Constantinople", types.NewEIP155Signer(config.ChainID), tt.Constantinople, true}, + } { + sender, txhash, err := validateTx(tt.RLP, testcase.signer, testcase.isHomestead) + + if testcase.fork.Sender == (common.UnprefixedAddress{}) { + if err == nil { + return fmt.Errorf("Expected error, got none (address %v)", sender.String()) + } + continue + } + // Should resolve the right address + if err != nil { + return fmt.Errorf("Got error, expected none: %v", err) + } + if sender == nil { + return fmt.Errorf("sender was nil, should be %x", common.Address(testcase.fork.Sender)) + } + if *sender != common.Address(testcase.fork.Sender) { + return fmt.Errorf("Sender mismatch: got %x, want %x", sender, testcase.fork.Sender) + } + if txhash == nil { + return fmt.Errorf("txhash was nil, should be %x", common.Hash(testcase.fork.Hash)) + } + if *txhash != common.Hash(testcase.fork.Hash) { + return fmt.Errorf("Hash mismatch: got %x, want %x", *txhash, testcase.fork.Hash) + } } return nil }