From ffe78da36e8b715d9b94f77618e806c3e43fdc91 Mon Sep 17 00:00:00 2001 From: yihuang Date: Tue, 19 Jul 2022 23:12:48 +0800 Subject: [PATCH] !fix(evm): Fix eth tx hashes in json-rpc responses (#1176) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix eth tx hashes in json-rpc responses Closes: #1175 - Remove Size_ field - Validate From/Hash fields in ante handler - Recompute tx hashes in json-rpc apis to cope with old blocks Update CHANGELOG.md remove Size_, validate Hash/From, add unit tests update spec Update CHANGELOG.md Update app/ante/eth.go populate From in SendRawTransaction Apply suggestions from code review keep Size_ field to avoid breaking tx format * move some validation to ValidateBasic * move validation to ValidateBasic * make ToTransaction returns a valid msg * restructure the protoTxProvider check * add comment * workaround tx hash issue in event parsing * fix integration test * fix unit test Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com> --- CHANGELOG.md | 2 + app/ante/ante_test.go | 61 ++++++++--- app/ante/eth.go | 121 +++++++++++---------- app/ante/utils_test.go | 1 + docs/api/proto-docs.md | 2 +- proto/ethermint/evm/v1/tx.proto | 2 +- rpc/backend/evm_backend.go | 1 + rpc/namespaces/ethereum/eth/filters/api.go | 4 +- rpc/types/events.go | 15 ++- rpc/types/events_test.go | 2 + rpc/types/utils.go | 1 + tests/e2e/integration_test.go | 2 + x/evm/keeper/integration_test.go | 2 + x/evm/spec/04_transactions.md | 2 +- x/evm/types/msg.go | 28 ++++- x/evm/types/msg_test.go | 89 ++++++++++++--- x/evm/types/tx.pb.go | 2 +- x/evm/types/tx_args.go | 4 +- x/evm/types/utils.go | 4 +- x/feemarket/keeper/integration_test.go | 1 + 20 files changed, 245 insertions(+), 101 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 43b894bf..2b058690 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,8 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (deps) [\#1159](https://github.com/evmos/ethermint/pull/1159) Bump Geth version to `v1.10.19`. * (deps) [#1167](https://github.com/evmos/ethermint/pull/1167) Upgrade ibc-go to v4. * (evm) [\#1174](https://github.com/evmos/ethermint/pull/1174) Don't allow eth txs with 0 in mempool. +* (ante) [#1176](https://github.com/evmos/ethermint/pull/1176) Fix invalid tx hashes; Remove `Size_` field and validate `Hash`/`From` fields in ante handler, + recompute eth tx hashes in JSON-RPC APIs to fix old blocks. ### Improvements diff --git a/app/ante/ante_test.go b/app/ante/ante_test.go index 1b090958..41b040e0 100644 --- a/app/ante/ante_test.go +++ b/app/ante/ante_test.go @@ -6,6 +6,7 @@ import ( "strings" "github.com/cosmos/cosmos-sdk/types/tx/signing" + authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" sdk "github.com/cosmos/cosmos-sdk/types" @@ -16,19 +17,22 @@ import ( ) func (suite AnteTestSuite) TestAnteHandler() { - suite.enableFeemarket = false - suite.SetupTest() // reset - + var acc authtypes.AccountI addr, privKey := tests.NewAddrKey() to := tests.GenerateAddress() - acc := suite.app.AccountKeeper.NewAccountWithAddress(suite.ctx, addr.Bytes()) - suite.Require().NoError(acc.SetSequence(1)) - suite.app.AccountKeeper.SetAccount(suite.ctx, acc) + setup := func() { + suite.enableFeemarket = false + suite.SetupTest() // reset - suite.app.EvmKeeper.SetBalance(suite.ctx, addr, big.NewInt(10000000000)) + acc = suite.app.AccountKeeper.NewAccountWithAddress(suite.ctx, addr.Bytes()) + suite.Require().NoError(acc.SetSequence(1)) + suite.app.AccountKeeper.SetAccount(suite.ctx, acc) - suite.app.FeeMarketKeeper.SetBaseFee(suite.ctx, big.NewInt(100)) + suite.app.EvmKeeper.SetBalance(suite.ctx, addr, big.NewInt(10000000000)) + + suite.app.FeeMarketKeeper.SetBaseFee(suite.ctx, big.NewInt(100)) + } testCases := []struct { name string @@ -63,7 +67,7 @@ func (suite AnteTestSuite) TestAnteHandler() { func() sdk.Tx { signedContractTx := evmtypes.NewTxContract( suite.app.EvmKeeper.ChainID(), - 2, + 1, big.NewInt(10), 100000, big.NewInt(150), @@ -84,7 +88,7 @@ func (suite AnteTestSuite) TestAnteHandler() { func() sdk.Tx { signedContractTx := evmtypes.NewTxContract( suite.app.EvmKeeper.ChainID(), - 3, + 1, big.NewInt(10), 100000, big.NewInt(150), @@ -105,7 +109,7 @@ func (suite AnteTestSuite) TestAnteHandler() { func() sdk.Tx { signedTx := evmtypes.NewTx( suite.app.EvmKeeper.ChainID(), - 4, + 1, &to, big.NewInt(10), 100000, @@ -127,7 +131,7 @@ func (suite AnteTestSuite) TestAnteHandler() { func() sdk.Tx { signedTx := evmtypes.NewTx( suite.app.EvmKeeper.ChainID(), - 5, + 1, &to, big.NewInt(10), 100000, @@ -149,7 +153,7 @@ func (suite AnteTestSuite) TestAnteHandler() { func() sdk.Tx { signedTx := evmtypes.NewTx( suite.app.EvmKeeper.ChainID(), - 6, + 1, &to, big.NewInt(10), 100000, @@ -170,7 +174,7 @@ func (suite AnteTestSuite) TestAnteHandler() { func() sdk.Tx { signedTx := evmtypes.NewTx( suite.app.EvmKeeper.ChainID(), - 7, + 1, &to, big.NewInt(10), 100000, @@ -189,7 +193,7 @@ func (suite AnteTestSuite) TestAnteHandler() { { "fail - CheckTx (cosmos tx is not valid)", func() sdk.Tx { - signedTx := evmtypes.NewTx(suite.app.EvmKeeper.ChainID(), 8, &to, big.NewInt(10), 100000, big.NewInt(1), nil, nil, nil, nil) + signedTx := evmtypes.NewTx(suite.app.EvmKeeper.ChainID(), 1, &to, big.NewInt(10), 100000, big.NewInt(1), nil, nil, nil, nil) signedTx.From = addr.Hex() txBuilder := suite.CreateTestTxBuilder(signedTx, privKey, 1, false) @@ -201,7 +205,7 @@ func (suite AnteTestSuite) TestAnteHandler() { { "fail - CheckTx (memo too long)", func() sdk.Tx { - signedTx := evmtypes.NewTx(suite.app.EvmKeeper.ChainID(), 5, &to, big.NewInt(10), 100000, big.NewInt(1), nil, nil, nil, nil) + signedTx := evmtypes.NewTx(suite.app.EvmKeeper.ChainID(), 1, &to, big.NewInt(10), 100000, big.NewInt(1), nil, nil, nil, nil) signedTx.From = addr.Hex() txBuilder := suite.CreateTestTxBuilder(signedTx, privKey, 1, false) @@ -212,7 +216,7 @@ func (suite AnteTestSuite) TestAnteHandler() { { "fail - CheckTx (ExtensionOptionsEthereumTx not set)", func() sdk.Tx { - signedTx := evmtypes.NewTx(suite.app.EvmKeeper.ChainID(), 5, &to, big.NewInt(10), 100000, big.NewInt(1), nil, nil, nil, nil) + signedTx := evmtypes.NewTx(suite.app.EvmKeeper.ChainID(), 1, &to, big.NewInt(10), 100000, big.NewInt(1), nil, nil, nil, nil) signedTx.From = addr.Hex() txBuilder := suite.CreateTestTxBuilder(signedTx, privKey, 1, false, true) @@ -390,10 +394,33 @@ func (suite AnteTestSuite) TestAnteHandler() { return txBuilder.GetTx() }, false, false, false, }, + { + "fails - invalid from", + func() sdk.Tx { + msg := evmtypes.NewTxContract( + suite.app.EvmKeeper.ChainID(), + 1, + big.NewInt(10), + 100000, + big.NewInt(150), + big.NewInt(200), + nil, + nil, + nil, + ) + msg.From = addr.Hex() + tx := suite.CreateTestTx(msg, privKey, 1, false) + msg = tx.GetMsgs()[0].(*evmtypes.MsgEthereumTx) + msg.From = addr.Hex() + return tx + }, true, false, false, + }, } for _, tc := range testCases { suite.Run(tc.name, func() { + setup() + suite.ctx = suite.ctx.WithIsCheckTx(tc.checkTx).WithIsReCheckTx(tc.reCheckTx) // expConsumed := params.TxGasContractCreation + params.TxGas diff --git a/app/ante/eth.go b/app/ante/eth.go index 66ac465c..bd0b1440 100644 --- a/app/ante/eth.go +++ b/app/ante/eth.go @@ -61,8 +61,7 @@ func (esvd EthSigVerificationDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, s if err != nil { return ctx, sdkerrors.Wrapf( sdkerrors.ErrorInvalidSigner, - "couldn't retrieve sender address ('%s') from the ethereum transaction: %s", - msgEthTx.From, + "couldn't retrieve sender address from the ethereum transaction: %s", err.Error(), ) } @@ -403,74 +402,82 @@ func (vbd EthValidateBasicDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simu // For eth type cosmos tx, some fields should be veified as zero values, // since we will only verify the signature against the hash of the MsgEthereumTx.Data - if wrapperTx, ok := tx.(protoTxProvider); ok { - protoTx := wrapperTx.GetProtoTx() - body := protoTx.Body - if body.Memo != "" || body.TimeoutHeight != uint64(0) || len(body.NonCriticalExtensionOptions) > 0 { - return ctx, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, - "for eth tx body Memo TimeoutHeight NonCriticalExtensionOptions should be empty") + wrapperTx, ok := tx.(protoTxProvider) + if !ok { + return ctx, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "invalid tx type %T, didn't implement interface protoTxProvider", tx) + } + + protoTx := wrapperTx.GetProtoTx() + body := protoTx.Body + if body.Memo != "" || body.TimeoutHeight != uint64(0) || len(body.NonCriticalExtensionOptions) > 0 { + return ctx, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, + "for eth tx body Memo TimeoutHeight NonCriticalExtensionOptions should be empty") + } + + if len(body.ExtensionOptions) != 1 { + return ctx, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "for eth tx length of ExtensionOptions should be 1") + } + + txFee := sdk.Coins{} + txGasLimit := uint64(0) + + params := vbd.evmKeeper.GetParams(ctx) + chainID := vbd.evmKeeper.ChainID() + ethCfg := params.ChainConfig.EthereumConfig(chainID) + baseFee := vbd.evmKeeper.GetBaseFee(ctx, ethCfg) + + for _, msg := range protoTx.GetMsgs() { + msgEthTx, ok := msg.(*evmtypes.MsgEthereumTx) + if !ok { + return ctx, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "invalid message type %T, expected %T", msg, (*evmtypes.MsgEthereumTx)(nil)) } - if len(body.ExtensionOptions) != 1 { - return ctx, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "for eth tx length of ExtensionOptions should be 1") + // Validate `From` field + if msgEthTx.From != "" { + return ctx, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "invalid From %s, expect empty string", msgEthTx.From) } - txFee := sdk.Coins{} - txGasLimit := uint64(0) + txGasLimit += msgEthTx.GetGas() - params := vbd.evmKeeper.GetParams(ctx) - chainID := vbd.evmKeeper.ChainID() - ethCfg := params.ChainConfig.EthereumConfig(chainID) - baseFee := vbd.evmKeeper.GetBaseFee(ctx, ethCfg) - - for _, msg := range protoTx.GetMsgs() { - msgEthTx, ok := msg.(*evmtypes.MsgEthereumTx) - if !ok { - return ctx, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "invalid message type %T, expected %T", msg, (*evmtypes.MsgEthereumTx)(nil)) - } - - txGasLimit += msgEthTx.GetGas() - - txData, err := evmtypes.UnpackTxData(msgEthTx.Data) - if err != nil { - return ctx, sdkerrors.Wrap(err, "failed to unpack MsgEthereumTx Data") - } - - // return error if contract creation or call are disabled through governance - if !params.EnableCreate && txData.GetTo() == nil { - return ctx, sdkerrors.Wrap(evmtypes.ErrCreateDisabled, "failed to create new contract") - } else if !params.EnableCall && txData.GetTo() != nil { - return ctx, sdkerrors.Wrap(evmtypes.ErrCallDisabled, "failed to call contract") - } - - if baseFee == nil && txData.TxType() == ethtypes.DynamicFeeTxType { - return ctx, sdkerrors.Wrap(ethtypes.ErrTxTypeNotSupported, "dynamic fee tx not supported") - } - - txFee = txFee.Add(sdk.NewCoin(params.EvmDenom, sdk.NewIntFromBigInt(txData.Fee()))) + txData, err := evmtypes.UnpackTxData(msgEthTx.Data) + if err != nil { + return ctx, sdkerrors.Wrap(err, "failed to unpack MsgEthereumTx Data") } - authInfo := protoTx.AuthInfo - if len(authInfo.SignerInfos) > 0 { - return ctx, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "for eth tx AuthInfo SignerInfos should be empty") + // return error if contract creation or call are disabled through governance + if !params.EnableCreate && txData.GetTo() == nil { + return ctx, sdkerrors.Wrap(evmtypes.ErrCreateDisabled, "failed to create new contract") + } else if !params.EnableCall && txData.GetTo() != nil { + return ctx, sdkerrors.Wrap(evmtypes.ErrCallDisabled, "failed to call contract") } - if authInfo.Fee.Payer != "" || authInfo.Fee.Granter != "" { - return ctx, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "for eth tx AuthInfo Fee payer and granter should be empty") + if baseFee == nil && txData.TxType() == ethtypes.DynamicFeeTxType { + return ctx, sdkerrors.Wrap(ethtypes.ErrTxTypeNotSupported, "dynamic fee tx not supported") } - if !authInfo.Fee.Amount.IsEqual(txFee) { - return ctx, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "invalid AuthInfo Fee Amount (%s != %s)", authInfo.Fee.Amount, txFee) - } + txFee = txFee.Add(sdk.NewCoin(params.EvmDenom, sdk.NewIntFromBigInt(txData.Fee()))) + } - if authInfo.Fee.GasLimit != txGasLimit { - return ctx, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "invalid AuthInfo Fee GasLimit (%d != %d)", authInfo.Fee.GasLimit, txGasLimit) - } + authInfo := protoTx.AuthInfo + if len(authInfo.SignerInfos) > 0 { + return ctx, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "for eth tx AuthInfo SignerInfos should be empty") + } - sigs := protoTx.Signatures - if len(sigs) > 0 { - return ctx, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "for eth tx Signatures should be empty") - } + if authInfo.Fee.Payer != "" || authInfo.Fee.Granter != "" { + return ctx, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "for eth tx AuthInfo Fee payer and granter should be empty") + } + + if !authInfo.Fee.Amount.IsEqual(txFee) { + return ctx, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "invalid AuthInfo Fee Amount (%s != %s)", authInfo.Fee.Amount, txFee) + } + + if authInfo.Fee.GasLimit != txGasLimit { + return ctx, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "invalid AuthInfo Fee GasLimit (%d != %d)", authInfo.Fee.GasLimit, txGasLimit) + } + + sigs := protoTx.Signatures + if len(sigs) > 0 { + return ctx, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "for eth tx Signatures should be empty") } return next(ctx, tx, simulate) diff --git a/app/ante/utils_test.go b/app/ante/utils_test.go index b789f128..4f0cabd7 100644 --- a/app/ante/utils_test.go +++ b/app/ante/utils_test.go @@ -190,6 +190,7 @@ func (suite *AnteTestSuite) CreateTestTxBuilder( err = msg.Sign(suite.ethSigner, tests.NewSigner(priv)) suite.Require().NoError(err) + msg.From = "" err = builder.SetMsgs(msg) suite.Require().NoError(err) diff --git a/docs/api/proto-docs.md b/docs/api/proto-docs.md index 15471f6b..6401c086 100644 --- a/docs/api/proto-docs.md +++ b/docs/api/proto-docs.md @@ -477,7 +477,7 @@ MsgEthereumTx encapsulates an Ethereum transaction as an SDK message. | `data` | [google.protobuf.Any](#google.protobuf.Any) | | inner transaction data caches | -| `size` | [double](#double) | | encoded storage size of the transaction | +| `size` | [double](#double) | | DEPRECATED: encoded storage size of the transaction | | `hash` | [string](#string) | | transaction hash in hex format | | `from` | [string](#string) | | ethereum signer address in hex format. This address value is checked against the address derived from the signature (V, R, S) using the secp256k1 elliptic curve | diff --git a/proto/ethermint/evm/v1/tx.proto b/proto/ethermint/evm/v1/tx.proto index 9dc53f2f..0898f8e0 100644 --- a/proto/ethermint/evm/v1/tx.proto +++ b/proto/ethermint/evm/v1/tx.proto @@ -25,7 +25,7 @@ message MsgEthereumTx { google.protobuf.Any data = 1; // caches - // encoded storage size of the transaction + // DEPRECATED: encoded storage size of the transaction double size = 2 [ (gogoproto.jsontag) = "-" ]; // transaction hash in hex format string hash = 3 [ (gogoproto.moretags) = "rlp:\"-\"" ]; diff --git a/rpc/backend/evm_backend.go b/rpc/backend/evm_backend.go index 21494cb3..e8b87801 100644 --- a/rpc/backend/evm_backend.go +++ b/rpc/backend/evm_backend.go @@ -1009,6 +1009,7 @@ func (b *Backend) GetEthereumMsgsFromTendermintBlock(resBlock *tmrpctypes.Result continue } + ethMsg.Hash = ethMsg.AsTransaction().Hash().Hex() result = append(result, ethMsg) } } diff --git a/rpc/namespaces/ethereum/eth/filters/api.go b/rpc/namespaces/ethereum/eth/filters/api.go index 10409fd8..9bc5a470 100644 --- a/rpc/namespaces/ethereum/eth/filters/api.go +++ b/rpc/namespaces/ethereum/eth/filters/api.go @@ -157,7 +157,7 @@ func (api *PublicFilterAPI) NewPendingTransactionFilter() rpc.ID { for _, msg := range tx.GetMsgs() { ethTx, ok := msg.(*evmtypes.MsgEthereumTx) if ok { - f.hashes = append(f.hashes, common.HexToHash(ethTx.Hash)) + f.hashes = append(f.hashes, ethTx.AsTransaction().Hash()) } } } @@ -221,7 +221,7 @@ func (api *PublicFilterAPI) NewPendingTransactions(ctx context.Context) (*rpc.Su for _, msg := range tx.GetMsgs() { ethTx, ok := msg.(*evmtypes.MsgEthereumTx) if ok { - _ = notifier.Notify(rpcSub.ID, common.HexToHash(ethTx.Hash)) + _ = notifier.Notify(rpcSub.ID, ethTx.AsTransaction().Hash()) } } case <-rpcSub.Err(): diff --git a/rpc/types/events.go b/rpc/types/events.go index db50301e..6b74d378 100644 --- a/rpc/types/events.go +++ b/rpc/types/events.go @@ -155,8 +155,21 @@ func (p *ParsedTxs) newTx(attrs []abci.EventAttribute) error { } // updateTx updates an exiting tx from events, called during parsing. +// In event format 2, we update the tx with the attributes of the second `ethereum_tx` event, +// Due to bug https://github.com/evmos/ethermint/issues/1175, the first `ethereum_tx` event may emit incorrect tx hash, +// so we prefer the second event and override the first one. func (p *ParsedTxs) updateTx(eventIndex int, attrs []abci.EventAttribute) error { - return fillTxAttributes(&p.Txs[eventIndex], attrs) + tx := NewParsedTx(eventIndex) + if err := fillTxAttributes(&tx, attrs); err != nil { + return err + } + if tx.Hash != p.Txs[eventIndex].Hash { + // if hash is different, index the new one too + p.TxHashes[tx.Hash] = eventIndex + } + // override the tx because the second event is more trustworthy + p.Txs[eventIndex] = tx + return nil } // GetTxByHash find ParsedTx by tx hash, returns nil if not exists. diff --git a/rpc/types/events_test.go b/rpc/types/events_test.go index c946a97f..68e097d3 100644 --- a/rpc/types/events_test.go +++ b/rpc/types/events_test.go @@ -107,6 +107,8 @@ func TestParseTxResult(t *testing.T) { }}, {Type: evmtypes.EventTypeEthereumTx, Attributes: []abci.EventAttribute{ {Key: []byte("amount"), Value: []byte("1000")}, + {Key: []byte("ethereumTxHash"), Value: []byte(txHash.Hex())}, + {Key: []byte("txIndex"), Value: []byte("0")}, {Key: []byte("txGasUsed"), Value: []byte("21000")}, {Key: []byte("txHash"), Value: []byte("14A84ED06282645EFBF080E0B7ED80D8D8D6A36337668A12B5F229F81CDD3F57")}, {Key: []byte("recipient"), Value: []byte("0x775b87ef5D82ca211811C1a02CE0fE0CA3a455d7")}, diff --git a/rpc/types/utils.go b/rpc/types/utils.go index 15da2448..f8a0c7e1 100644 --- a/rpc/types/utils.go +++ b/rpc/types/utils.go @@ -34,6 +34,7 @@ func RawTxToEthTx(clientCtx client.Context, txBz tmtypes.Tx) ([]*evmtypes.MsgEth if !ok { return nil, fmt.Errorf("invalid message type %T, expected %T", msg, &evmtypes.MsgEthereumTx{}) } + ethTx.Hash = ethTx.AsTransaction().Hash().Hex() ethTxs[i] = ethTx } return ethTxs, nil diff --git a/tests/e2e/integration_test.go b/tests/e2e/integration_test.go index 6ae14a5f..3b36acc9 100644 --- a/tests/e2e/integration_test.go +++ b/tests/e2e/integration_test.go @@ -794,6 +794,8 @@ func (s *IntegrationTestSuite) TestBatchETHTransactions() { err = msgTx.Sign(s.ethSigner, s.network.Validators[0].ClientCtx.Keyring) s.Require().NoError(err) + // A valid msg should have empty `From` + msgTx.From = "" msgs = append(msgs, msgTx.GetMsgs()...) txData, err := evmtypes.UnpackTxData(msgTx.Data) s.Require().NoError(err) diff --git a/x/evm/keeper/integration_test.go b/x/evm/keeper/integration_test.go index 4348d6ed..01904400 100644 --- a/x/evm/keeper/integration_test.go +++ b/x/evm/keeper/integration_test.go @@ -257,6 +257,8 @@ func prepareEthTx(priv *ethsecp256k1.PrivKey, msgEthereumTx *evmtypes.MsgEthereu err = msgEthereumTx.Sign(s.ethSigner, tests.NewSigner(priv)) s.Require().NoError(err) + // A valid msg should have empty `From` + msgEthereumTx.From = "" err = txBuilder.SetMsgs(msgEthereumTx) s.Require().NoError(err) diff --git a/x/evm/spec/04_transactions.md b/x/evm/spec/04_transactions.md index d3609dda..4a132cf3 100644 --- a/x/evm/spec/04_transactions.md +++ b/x/evm/spec/04_transactions.md @@ -14,7 +14,7 @@ An EVM state transition can be achieved by using the `MsgEthereumTx`. This messa type MsgEthereumTx struct { // inner transaction data Data *types.Any `protobuf:"bytes,1,opt,name=data,proto3" json:"data,omitempty"` - // encoded storage size of the transaction + // DEPRECATED: encoded storage size of the transaction Size_ float64 `protobuf:"fixed64,2,opt,name=size,proto3" json:"-"` // transaction hash in hex format Hash string `protobuf:"bytes,3,opt,name=hash,proto3" json:"hash,omitempty" rlp:"-"` diff --git a/x/evm/types/msg.go b/x/evm/types/msg.go index ba2f64ec..11134660 100644 --- a/x/evm/types/msg.go +++ b/x/evm/types/msg.go @@ -130,10 +130,12 @@ func newMsgEthereumTx( panic(err) } - return &MsgEthereumTx{Data: dataAny} + msg := MsgEthereumTx{Data: dataAny} + msg.Hash = msg.AsTransaction().Hash().Hex() + return &msg } -// fromEthereumTx populates the message fields from the given ethereum transaction +// FromEthereumTx populates the message fields from the given ethereum transaction func (msg *MsgEthereumTx) FromEthereumTx(tx *ethtypes.Transaction) error { txData, err := NewTxDataFromTx(tx) if err != nil { @@ -146,7 +148,6 @@ func (msg *MsgEthereumTx) FromEthereumTx(tx *ethtypes.Transaction) error { } msg.Data = anyTxData - msg.Size_ = float64(tx.Size()) msg.Hash = tx.Hash().Hex() return nil } @@ -166,6 +167,11 @@ func (msg MsgEthereumTx) ValidateBasic() error { } } + // Validate Size_ field, should be kept empty + if msg.Size_ != 0 { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "tx size is deprecated") + } + txData, err := UnpackTxData(msg.Data) if err != nil { return sdkerrors.Wrap(err, "failed to unpack tx data") @@ -176,7 +182,17 @@ func (msg MsgEthereumTx) ValidateBasic() error { return sdkerrors.Wrap(ErrInvalidGasLimit, "gas limit must not be zero") } - return txData.Validate() + if err := txData.Validate(); err != nil { + return err + } + + // Validate Hash field after validated txData to avoid panic + txHash := msg.AsTransaction().Hash().Hex() + if msg.Hash != txHash { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "invalid tx hash %s, expected: %s", msg.Hash, txHash) + } + + return nil } // GetMsgs returns a single MsgEthereumTx as an sdk.Msg. @@ -342,6 +358,10 @@ func (msg *MsgEthereumTx) BuildTx(b client.TxBuilder, evmDenom string) (signing. } builder.SetExtensionOptions(option) + + // A valid msg should have empty `From` + msg.From = "" + err = builder.SetMsgs(msg) if err != nil { return nil, err diff --git a/x/evm/types/msg_test.go b/x/evm/types/msg_test.go index 5556592d..0a31bb1b 100644 --- a/x/evm/types/msg_test.go +++ b/x/evm/types/msg_test.go @@ -372,24 +372,85 @@ func (suite *MsgsTestSuite) TestMsgEthereumTx_ValidateBasic() { }, } - for i, tc := range testCases { - to := common.HexToAddress(tc.from) + for _, tc := range testCases { + suite.Run(tc.msg, func() { + to := common.HexToAddress(tc.from) - tx := types.NewTx(tc.chainID, 1, &to, tc.amount, tc.gasLimit, tc.gasPrice, tc.gasFeeCap, tc.gasTipCap, nil, tc.accessList) - tx.From = tc.from + tx := types.NewTx(tc.chainID, 1, &to, tc.amount, tc.gasLimit, tc.gasPrice, tc.gasFeeCap, tc.gasTipCap, nil, tc.accessList) + tx.From = tc.from - // apply nil assignment here to test ValidateBasic function instead of NewTx - if strings.Contains(tc.msg, "nil tx.Data") { - tx.Data = nil - } + // apply nil assignment here to test ValidateBasic function instead of NewTx + if strings.Contains(tc.msg, "nil tx.Data") { + tx.Data = nil + } - err := tx.ValidateBasic() + err := tx.ValidateBasic() - if tc.expectPass { - suite.Require().NoError(err, "valid test %d failed: %s, %v", i, tc.msg) - } else { - suite.Require().Error(err, "invalid test %d passed: %s, %v", i, tc.msg) - } + if tc.expectPass { + suite.Require().NoError(err) + } else { + suite.Require().Error(err) + } + }) + } +} + +func (suite *MsgsTestSuite) TestMsgEthereumTx_ValidateBasicAdvanced() { + hundredInt := big.NewInt(100) + testCases := []struct { + msg string + msgBuilder func() *types.MsgEthereumTx + expectPass bool + }{ + { + "fails - invalid tx hash", + func() *types.MsgEthereumTx { + msg := types.NewTxContract( + hundredInt, + 1, + big.NewInt(10), + 100000, + big.NewInt(150), + big.NewInt(200), + nil, + nil, + nil, + ) + msg.Hash = "0x00" + return msg + }, + false, + }, + { + "fails - invalid size", + func() *types.MsgEthereumTx { + msg := types.NewTxContract( + hundredInt, + 1, + big.NewInt(10), + 100000, + big.NewInt(150), + big.NewInt(200), + nil, + nil, + nil, + ) + msg.Size_ = 1 + return msg + }, + false, + }, + } + + for _, tc := range testCases { + suite.Run(tc.msg, func() { + err := tc.msgBuilder().ValidateBasic() + if tc.expectPass { + suite.Require().NoError(err) + } else { + suite.Require().Error(err) + } + }) } } diff --git a/x/evm/types/tx.pb.go b/x/evm/types/tx.pb.go index a2e5fffa..883fa001 100644 --- a/x/evm/types/tx.pb.go +++ b/x/evm/types/tx.pb.go @@ -37,7 +37,7 @@ const _ = proto.GoGoProtoPackageIsVersion3 // please upgrade the proto package type MsgEthereumTx struct { // inner transaction data Data *types.Any `protobuf:"bytes,1,opt,name=data,proto3" json:"data,omitempty"` - // encoded storage size of the transaction + // DEPRECATED: encoded storage size of the transaction Size_ float64 `protobuf:"fixed64,2,opt,name=size,proto3" json:"-"` // transaction hash in hex format Hash string `protobuf:"bytes,3,opt,name=hash,proto3" json:"hash,omitempty" rlp:"-"` diff --git a/x/evm/types/tx_args.go b/x/evm/types/tx_args.go index 525a008f..5fcc89e8 100644 --- a/x/evm/types/tx_args.go +++ b/x/evm/types/tx_args.go @@ -143,10 +143,12 @@ func (args *TransactionArgs) ToTransaction() *MsgEthereumTx { from = args.From.Hex() } - return &MsgEthereumTx{ + msg := MsgEthereumTx{ Data: any, From: from, } + msg.Hash = msg.AsTransaction().Hash().Hex() + return &msg } // ToMessage converts the arguments to the Message type used by the core evm. diff --git a/x/evm/types/utils.go b/x/evm/types/utils.go index c30d2e65..5f5e02b0 100644 --- a/x/evm/types/utils.go +++ b/x/evm/types/utils.go @@ -62,7 +62,9 @@ func UnwrapEthereumMsg(tx *sdk.Tx, ethHash common.Hash) (*MsgEthereumTx, error) if !ok { return nil, fmt.Errorf("invalid tx type: %T", tx) } - if ethMsg.AsTransaction().Hash() == ethHash { + txHash := ethMsg.AsTransaction().Hash() + ethMsg.Hash = txHash.Hex() + if txHash == ethHash { return ethMsg, nil } } diff --git a/x/feemarket/keeper/integration_test.go b/x/feemarket/keeper/integration_test.go index c9304002..6f0b1355 100644 --- a/x/feemarket/keeper/integration_test.go +++ b/x/feemarket/keeper/integration_test.go @@ -567,6 +567,7 @@ func prepareEthTx(priv *ethsecp256k1.PrivKey, msgEthereumTx *evmtypes.MsgEthereu err = msgEthereumTx.Sign(s.ethSigner, tests.NewSigner(priv)) s.Require().NoError(err) + msgEthereumTx.From = "" err = txBuilder.SetMsgs(msgEthereumTx) s.Require().NoError(err)