Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle nil *Any in UnpackAny and add panic handler for tx decoding #7594

Merged
merged 21 commits into from
Oct 19, 2020
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 3 additions & 18 deletions baseapp/abci.go
Original file line number Diff line number Diff line change
Expand Up @@ -213,11 +213,6 @@ func (app *BaseApp) EndBlock(req abci.RequestEndBlock) (res abci.ResponseEndBloc
func (app *BaseApp) CheckTx(req abci.RequestCheckTx) abci.ResponseCheckTx {
defer telemetry.MeasureSince(time.Now(), "abci", "check_tx")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can telemetry panic?


tx, err := app.txDecoder(req.Tx)
if err != nil {
return sdkerrors.ResponseCheckTx(err, 0, 0, app.trace)
}

var mode runTxMode

switch {
Expand All @@ -231,7 +226,7 @@ func (app *BaseApp) CheckTx(req abci.RequestCheckTx) abci.ResponseCheckTx {
panic(fmt.Sprintf("unknown RequestCheckTx type: %s", req.Type))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still want to panic here and crash the state machine? Maybe for a separate PR but it would be good to make this method as bulletproof as possible.

}

gInfo, result, err := app.runTx(mode, req.Tx, tx)
gInfo, result, err := app.runTx(mode, req.Tx)
if err != nil {
return sdkerrors.ResponseCheckTx(err, gInfo.GasWanted, gInfo.GasUsed, app.trace)
}
Expand All @@ -253,11 +248,6 @@ func (app *BaseApp) CheckTx(req abci.RequestCheckTx) abci.ResponseCheckTx {
func (app *BaseApp) DeliverTx(req abci.RequestDeliverTx) abci.ResponseDeliverTx {
defer telemetry.MeasureSince(time.Now(), "abci", "deliver_tx")

tx, err := app.txDecoder(req.Tx)
if err != nil {
return sdkerrors.ResponseDeliverTx(err, 0, 0, app.trace)
}

gInfo := sdk.GasInfo{}
resultStr := "successful"

Expand All @@ -268,7 +258,7 @@ func (app *BaseApp) DeliverTx(req abci.RequestDeliverTx) abci.ResponseDeliverTx
telemetry.SetGauge(float32(gInfo.GasWanted), "tx", "gas", "wanted")
}()

gInfo, result, err := app.runTx(runTxModeDeliver, req.Tx, tx)
gInfo, result, err := app.runTx(runTxModeDeliver, req.Tx)
if err != nil {
resultStr = "failed"
return sdkerrors.ResponseDeliverTx(err, gInfo.GasWanted, gInfo.GasUsed, app.trace)
Expand Down Expand Up @@ -673,12 +663,7 @@ func handleQueryApp(app *BaseApp, path []string, req abci.RequestQuery) abci.Res
case "simulate":
txBytes := req.Data

tx, err := app.txDecoder(txBytes)
if err != nil {
return sdkerrors.QueryResult(sdkerrors.Wrap(err, "failed to decode tx"))
}

gInfo, res, err := app.Simulate(txBytes, tx)
gInfo, res, err := app.Simulate(txBytes)
if err != nil {
return sdkerrors.QueryResult(sdkerrors.Wrap(err, "failed to simulate tx"))
}
Expand Down
7 changes: 6 additions & 1 deletion baseapp/baseapp.go
Original file line number Diff line number Diff line change
Expand Up @@ -557,7 +557,7 @@ func (app *BaseApp) cacheTxContext(ctx sdk.Context, txBytes []byte) (sdk.Context
// Note, gas execution info is always returned. A reference to a Result is
// returned if the tx does not run out of gas and if all the messages are valid
// and execute successfully. An error is returned otherwise.
func (app *BaseApp) runTx(mode runTxMode, txBytes []byte, tx sdk.Tx) (gInfo sdk.GasInfo, result *sdk.Result, err error) {
func (app *BaseApp) runTx(mode runTxMode, txBytes []byte) (gInfo sdk.GasInfo, result *sdk.Result, err error) {
// NOTE: GasWanted should be returned by the AnteHandler. GasUsed is
// determined by the GasMeter. We need access to the context to get the gas
// meter so we initialize upfront.
Expand Down Expand Up @@ -603,6 +603,11 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte, tx sdk.Tx) (gInfo sdk.
}
}()

tx, err := app.txDecoder(txBytes)
if err != nil {
return sdk.GasInfo{}, nil, err
}

msgs := tx.GetMsgs()
if err := validateBasicTxMsgs(msgs); err != nil {
return sdk.GasInfo{}, nil, err
Expand Down
41 changes: 25 additions & 16 deletions baseapp/baseapp_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
"github.com/cosmos/cosmos-sdk/testutil/testdata"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
"github.com/cosmos/cosmos-sdk/x/auth/legacy/legacytx"
)

var (
Expand Down Expand Up @@ -97,6 +98,14 @@ func registerTestCodec(cdc *codec.LegacyAmino) {
cdc.RegisterConcrete(&msgNoRoute{}, "cosmos-sdk/baseapp/msgNoRoute", nil)
}

// txEncoder creates a amino TxEncoder for testing purposes.
func txEncoder() sdk.TxEncoder {
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
cdc := codec.NewLegacyAmino()
registerTestCodec(cdc)

return legacytx.StdTxConfig{Cdc: cdc}.TxEncoder()
}

// simple one store baseapp
func setupBaseApp(t *testing.T, options ...func(*BaseApp)) *BaseApp {
app := newBaseApp(t.Name(), options...)
Expand Down Expand Up @@ -1118,13 +1127,13 @@ func TestSimulateTx(t *testing.T) {
require.Nil(t, err)

// simulate a message, check gas reported
gInfo, result, err := app.Simulate(txBytes, tx)
gInfo, result, err := app.Simulate(txBytes)
require.NoError(t, err)
require.NotNil(t, result)
require.Equal(t, gasConsumed, gInfo.GasUsed)

// simulate again, same result
gInfo, result, err = app.Simulate(txBytes, tx)
gInfo, result, err = app.Simulate(txBytes)
require.NoError(t, err)
require.NotNil(t, result)
require.Equal(t, gasConsumed, gInfo.GasUsed)
Expand Down Expand Up @@ -1171,7 +1180,7 @@ func TestRunInvalidTransaction(t *testing.T) {
// transaction with no messages
{
emptyTx := &txTest{}
_, result, err := app.Deliver(emptyTx)
_, result, err := app.Deliver(txEncoder(), emptyTx)
require.Error(t, err)
require.Nil(t, result)

Expand All @@ -1198,7 +1207,7 @@ func TestRunInvalidTransaction(t *testing.T) {

for _, testCase := range testCases {
tx := testCase.tx
_, result, err := app.Deliver(tx)
_, result, err := app.Deliver(txEncoder(), tx)

if testCase.fail {
require.Error(t, err)
Expand All @@ -1215,7 +1224,7 @@ func TestRunInvalidTransaction(t *testing.T) {
// transaction with no known route
{
unknownRouteTx := txTest{[]sdk.Msg{msgNoRoute{}}, 0, false}
_, result, err := app.Deliver(unknownRouteTx)
_, result, err := app.Deliver(txEncoder(), unknownRouteTx)
require.Error(t, err)
require.Nil(t, result)

Expand All @@ -1224,7 +1233,7 @@ func TestRunInvalidTransaction(t *testing.T) {
require.EqualValues(t, sdkerrors.ErrUnknownRequest.ABCICode(), code, err)

unknownRouteTx = txTest{[]sdk.Msg{msgCounter{}, msgNoRoute{}}, 0, false}
_, result, err = app.Deliver(unknownRouteTx)
_, result, err = app.Deliver(txEncoder(), unknownRouteTx)
require.Error(t, err)
require.Nil(t, result)

Expand Down Expand Up @@ -1274,7 +1283,7 @@ func TestTxGasLimits(t *testing.T) {
}
}()

count := tx.(*txTest).Counter
count := tx.(txTest).Counter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't explain why this change is needed, but it is.

We have a *txTest as input in Deliver(), it gets amino-encoded into bytes in Deliver(), and decoded in runTx. The decoded type is somehow txTest instead of *txTest

newCtx.GasMeter().ConsumeGas(uint64(count), "counter-ante")

return newCtx, nil
Expand All @@ -1284,7 +1293,7 @@ func TestTxGasLimits(t *testing.T) {

routerOpt := func(bapp *BaseApp) {
r := sdk.NewRoute(routeMsgCounter, func(ctx sdk.Context, msg sdk.Msg) (*sdk.Result, error) {
count := msg.(msgCounter).Counter
count := msg.(*msgCounter).Counter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto (or not?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol 🤷

ctx.GasMeter().ConsumeGas(uint64(count), "counter-handler")
return &sdk.Result{}, nil
})
Expand Down Expand Up @@ -1322,7 +1331,7 @@ func TestTxGasLimits(t *testing.T) {

for i, tc := range testCases {
tx := tc.tx
gInfo, result, err := app.Deliver(tx)
gInfo, result, err := app.Deliver(txEncoder(), tx)

// check gas used and wanted
require.Equal(t, tc.gasUsed, gInfo.GasUsed, fmt.Sprintf("tc #%d; gas: %v, result: %v, err: %s", i, gInfo, result, err))
Expand Down Expand Up @@ -1359,7 +1368,7 @@ func TestMaxBlockGasLimits(t *testing.T) {
}
}()

count := tx.(*txTest).Counter
count := tx.(txTest).Counter
newCtx.GasMeter().ConsumeGas(uint64(count), "counter-ante")

return
Expand All @@ -1368,7 +1377,7 @@ func TestMaxBlockGasLimits(t *testing.T) {

routerOpt := func(bapp *BaseApp) {
r := sdk.NewRoute(routeMsgCounter, func(ctx sdk.Context, msg sdk.Msg) (*sdk.Result, error) {
count := msg.(msgCounter).Counter
count := msg.(*msgCounter).Counter
ctx.GasMeter().ConsumeGas(uint64(count), "counter-handler")
return &sdk.Result{}, nil
})
Expand Down Expand Up @@ -1412,7 +1421,7 @@ func TestMaxBlockGasLimits(t *testing.T) {

// execute the transaction multiple times
for j := 0; j < tc.numDelivers; j++ {
_, result, err := app.Deliver(tx)
_, result, err := app.Deliver(txEncoder(), tx)

ctx := app.getState(runTxModeDeliver).ctx

Expand Down Expand Up @@ -1480,7 +1489,7 @@ func TestCustomRunTxPanicHandler(t *testing.T) {
{
tx := newTxCounter(0, 0)

require.PanicsWithValue(t, customPanicMsg, func() { app.Deliver(tx) })
require.PanicsWithValue(t, customPanicMsg, func() { app.Deliver(txEncoder(), tx) })
}
}

Expand Down Expand Up @@ -1589,7 +1598,7 @@ func TestGasConsumptionBadTx(t *testing.T) {

routerOpt := func(bapp *BaseApp) {
r := sdk.NewRoute(routeMsgCounter, func(ctx sdk.Context, msg sdk.Msg) (*sdk.Result, error) {
count := msg.(msgCounter).Counter
count := msg.(*msgCounter).Counter
ctx.GasMeter().ConsumeGas(uint64(count), "counter-handler")
return &sdk.Result{}, nil
})
Expand Down Expand Up @@ -1668,7 +1677,7 @@ func TestQuery(t *testing.T) {
require.Equal(t, 0, len(res.Value))

// query is still empty after a CheckTx
_, resTx, err := app.Check(tx)
_, resTx, err := app.Check(txEncoder(), tx)
require.NoError(t, err)
require.NotNil(t, resTx)
res = app.Query(query)
Expand All @@ -1678,7 +1687,7 @@ func TestQuery(t *testing.T) {
header := tmproto.Header{Height: app.LastBlockHeight() + 1}
app.BeginBlock(abci.RequestBeginBlock{Header: header})

_, resTx, err = app.Deliver(tx)
_, resTx, err = app.Deliver(txEncoder(), tx)
require.NoError(t, err)
require.NotNil(t, resTx)
res = app.Query(query)
Expand Down
33 changes: 0 additions & 33 deletions baseapp/helpers.go

This file was deleted.

46 changes: 46 additions & 0 deletions baseapp/test_helpers.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
package baseapp

import (
tmproto "github.com/tendermint/tendermint/proto/tendermint/types"

sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
)

func (app *BaseApp) Check(txEncoder sdk.TxEncoder, tx sdk.Tx) (sdk.GasInfo, *sdk.Result, error) {
// runTx expects tx bytes as argument, so we encode the tx argument into
// bytes. Note that runTx will actually decode those bytes again. But since
// this helper is only used in tests/simulation, it's fine.
bz, err := txEncoder(tx)
if err != nil {
return sdk.GasInfo{}, nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "%s", err)
}
return app.runTx(runTxModeCheck, bz)
}

func (app *BaseApp) Simulate(txBytes []byte) (sdk.GasInfo, *sdk.Result, error) {
return app.runTx(runTxModeSimulate, txBytes)
}

func (app *BaseApp) Deliver(txEncoder sdk.TxEncoder, tx sdk.Tx) (sdk.GasInfo, *sdk.Result, error) {
// See comment for Check().
bz, err := txEncoder(tx)
if err != nil {
return sdk.GasInfo{}, nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "%s", err)
}
return app.runTx(runTxModeDeliver, bz)
}

// Context with current {check, deliver}State of the app used by tests.
func (app *BaseApp) NewContext(isCheckTx bool, header tmproto.Header) sdk.Context {
if isCheckTx {
return sdk.NewContext(app.checkState.ms, header, true, app.logger).
WithMinGasPrices(app.minGasPrices)
}

return sdk.NewContext(app.deliverState.ms, header, false, app.logger)
}

func (app *BaseApp) NewUncachedContext(isCheckTx bool, header tmproto.Header) sdk.Context {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a doc string here?

return sdk.NewContext(app.cms, header, isCheckTx, app.logger)
}
6 changes: 2 additions & 4 deletions client/grpc/simulate/simulate.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,10 @@ import (

codectypes "github.com/cosmos/cosmos-sdk/codec/types"
sdk "github.com/cosmos/cosmos-sdk/types"
authtx "github.com/cosmos/cosmos-sdk/x/auth/tx"
)

// BaseAppSimulateFn is the signature of the Baseapp#Simulate function.
type BaseAppSimulateFn func(txBytes []byte, txtypes sdk.Tx) (sdk.GasInfo, *sdk.Result, error)
type BaseAppSimulateFn func(txBytes []byte) (sdk.GasInfo, *sdk.Result, error)

type simulateServer struct {
simulate BaseAppSimulateFn
Expand All @@ -39,13 +38,12 @@ func (s simulateServer) Simulate(ctx context.Context, req *SimulateRequest) (*Si
if err != nil {
return nil, err
}
txBuilder := authtx.WrapTx(req.Tx)
txBytes, err := req.Tx.Marshal()
if err != nil {
return nil, err
}

gasInfo, result, err := s.simulate(txBytes, txBuilder.GetTx())
gasInfo, result, err := s.simulate(txBytes)
if err != nil {
return nil, err
}
Expand Down
4 changes: 4 additions & 0 deletions codec/types/interface_registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,10 @@ func (registry *interfaceRegistry) ListImplementations(ifaceName string) []strin
}

func (registry *interfaceRegistry) UnpackAny(any *Any, iface interface{}) error {
if any == nil {
blushi marked this conversation as resolved.
Show resolved Hide resolved
return nil
}

if any.TypeUrl == "" {
// if TypeUrl is empty return nil because without it we can't actually unpack anything
return nil
Expand Down
10 changes: 5 additions & 5 deletions simapp/test_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -320,12 +320,12 @@ func CheckBalance(t *testing.T, app *SimApp, addr sdk.AccAddress, balances sdk.C
// the parameter 'expPass' against the result. A corresponding result is
// returned.
func SignCheckDeliver(
t *testing.T, txGen client.TxConfig, app *bam.BaseApp, header tmproto.Header, msgs []sdk.Msg,
t *testing.T, txCfg client.TxConfig, app *bam.BaseApp, header tmproto.Header, msgs []sdk.Msg,
chainID string, accNums, accSeqs []uint64, expSimPass, expPass bool, priv ...crypto.PrivKey,
) (sdk.GasInfo, *sdk.Result, error) {

tx, err := helpers.GenTx(
txGen,
txCfg,
msgs,
sdk.Coins{sdk.NewInt64Coin(sdk.DefaultBondDenom, 0)},
helpers.DefaultGenTxGas,
Expand All @@ -335,11 +335,11 @@ func SignCheckDeliver(
priv...,
)
require.NoError(t, err)
txBytes, err := txGen.TxEncoder()(tx)
txBytes, err := txCfg.TxEncoder()(tx)
require.Nil(t, err)

// Must simulate now as CheckTx doesn't run Msgs anymore
_, res, err := app.Simulate(txBytes, tx)
_, res, err := app.Simulate(txBytes)

if expSimPass {
require.NoError(t, err)
Expand All @@ -351,7 +351,7 @@ func SignCheckDeliver(

// Simulate a sending a transaction and committing a block
app.BeginBlock(abci.RequestBeginBlock{Header: header})
gInfo, res, err := app.Deliver(tx)
gInfo, res, err := app.Deliver(txCfg.TxEncoder(), tx)

if expPass {
require.NoError(t, err)
Expand Down
Loading