From a4d46ffcd42244605df3ca42e84da7dad55f2626 Mon Sep 17 00:00:00 2001 From: Anil Kumar Kammari Date: Fri, 1 Apr 2022 01:43:33 +0530 Subject: [PATCH] fix: Improve gentx validation & error messages (#11500) ## Description Closes: #9304 --- ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable) --- CHANGELOG.md | 1 + x/genutil/collect.go | 4 +-- x/genutil/gentx.go | 6 ++--- x/genutil/gentx_test.go | 12 ++++----- x/genutil/types/genesis_state.go | 43 ++++++++++++++++++++------------ 5 files changed, 39 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ba140fcca..319ea996c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Features +* (x/genutil) [\#11500](https://github.com/cosmos/cosmos-sdk/pull/11500) Fix GenTx validation and adjust error messages * [\#11430](https://github.com/cosmos/cosmos-sdk/pull/11430) Introduce a new `grpc-only` flag, such that when enabled, will start the node in a query-only mode. Note, gRPC MUST be enabled with this flag. * (x/upgrade) [\#11116](https://github.com/cosmos/cosmos-sdk/pull/11116) `MsgSoftwareUpgrade` and has been added to support v1beta2 msgs-based gov proposals. * (x/bank) [\#11417](https://github.com/cosmos/cosmos-sdk/pull/11417) Introduce a new `SpendableBalances` gRPC query that retrieves an account's total (paginated) spendable balances. diff --git a/x/genutil/collect.go b/x/genutil/collect.go index 6ad5f50aa..8b06c12f7 100644 --- a/x/genutil/collect.go +++ b/x/genutil/collect.go @@ -111,8 +111,8 @@ func CollectTxs(cdc codec.JSONCodec, txJSONDecoder sdk.TxDecoder, moniker, genTx return appGenTxs, persistentPeers, err } - var genTx sdk.Tx - if genTx, err = txJSONDecoder(jsonRawTx); err != nil { + genTx, err := types.ValidateAndGetGenTx(jsonRawTx, txJSONDecoder) + if err != nil { return appGenTxs, persistentPeers, err } diff --git a/x/genutil/gentx.go b/x/genutil/gentx.go index 61b237151..39fa60075 100644 --- a/x/genutil/gentx.go +++ b/x/genutil/gentx.go @@ -100,17 +100,17 @@ func DeliverGenTxs( for _, genTx := range genTxs { tx, err := txEncodingConfig.TxJSONDecoder()(genTx) if err != nil { - panic(err) + return nil, fmt.Errorf("failed to decode GenTx '%s': %s", genTx, err) } bz, err := txEncodingConfig.TxEncoder()(tx) if err != nil { - panic(err) + return nil, fmt.Errorf("failed to encode GenTx '%s': %s", genTx, err) } res := deliverTx(abci.RequestDeliverTx{Tx: bz}) if !res.IsOK() { - panic(res.Log) + return nil, fmt.Errorf("failed to execute DelverTx for '%s': %s", genTx, res.Log) } } diff --git a/x/genutil/gentx_test.go b/x/genutil/gentx_test.go index 5d8289810..b08ecb975 100644 --- a/x/genutil/gentx_test.go +++ b/x/genutil/gentx_test.go @@ -269,12 +269,12 @@ func (suite *GenTxTestSuite) TestDeliverGenTxs() { ) }) } else { - suite.Require().Panics(func() { - genutil.DeliverGenTxs( - suite.ctx, genTxs, suite.app.StakingKeeper, suite.app.BaseApp.DeliverTx, - suite.encodingConfig.TxConfig, - ) - }) + _, err := genutil.DeliverGenTxs( + suite.ctx, genTxs, suite.app.StakingKeeper, suite.app.BaseApp.DeliverTx, + suite.encodingConfig.TxConfig, + ) + + suite.Require().Error(err) } }) } diff --git a/x/genutil/types/genesis_state.go b/x/genutil/types/genesis_state.go index 657f57a94..4a4dc9575 100644 --- a/x/genutil/types/genesis_state.go +++ b/x/genutil/types/genesis_state.go @@ -2,7 +2,6 @@ package types import ( "encoding/json" - "errors" "fmt" tmos "github.com/tendermint/tendermint/libs/os" @@ -96,24 +95,36 @@ func GenesisStateFromGenFile(genFile string) (genesisState map[string]json.RawMe // ValidateGenesis validates GenTx transactions func ValidateGenesis(genesisState *GenesisState, txJSONDecoder sdk.TxDecoder) error { - for i, genTx := range genesisState.GenTxs { - var tx sdk.Tx - tx, err := txJSONDecoder(genTx) + for _, genTx := range genesisState.GenTxs { + _, err := ValidateAndGetGenTx(genTx, txJSONDecoder) if err != nil { return err } - - msgs := tx.GetMsgs() - if len(msgs) != 1 { - return errors.New( - "must provide genesis Tx with exactly 1 CreateValidator message") - } - - // TODO: abstract back to staking - if _, ok := msgs[0].(*stakingtypes.MsgCreateValidator); !ok { - return fmt.Errorf( - "genesis transaction %v does not contain a MsgCreateValidator", i) - } } return nil } + +// ValidateAndGetGenTx validates the genesis transaction and returns GenTx if valid +// it cannot verify the signature as it is stateless validation +func ValidateAndGetGenTx(genTx json.RawMessage, txJSONDecoder sdk.TxDecoder) (sdk.Tx, error) { + tx, err := txJSONDecoder(genTx) + if err != nil { + return tx, fmt.Errorf("failed to decode gentx: %s, error: %s", genTx, err) + } + + msgs := tx.GetMsgs() + if len(msgs) != 1 { + return tx, fmt.Errorf("unexpected number of GenTx messages; got: %d, expected: 1", len(msgs)) + } + + // TODO: abstract back to staking + if _, ok := msgs[0].(*stakingtypes.MsgCreateValidator); !ok { + return tx, fmt.Errorf("unexpected GenTx message type; expected: MsgCreateValidator, got: %T", msgs[0]) + } + + if err := msgs[0].ValidateBasic(); err != nil { + return tx, fmt.Errorf("invalid GenTx '%s': %s", msgs[0], err) + } + + return tx, nil +}