-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Changes from all commits
af3d5d8
978e2b9
9c876db
2451f2b
55bb84e
e77d85d
a8b8538
9405505
db112ba
ece41c5
da621d5
d468fb8
fa954f4
76df2e0
c22e713
42eaf62
28843ed
0a3eb4f
049dd5f
ddb88f3
aa020ab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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") | ||
|
||
tx, err := app.txDecoder(req.Tx) | ||
if err != nil { | ||
return sdkerrors.ResponseCheckTx(err, 0, 0, app.trace) | ||
} | ||
|
||
var mode runTxMode | ||
|
||
switch { | ||
|
@@ -231,7 +226,7 @@ func (app *BaseApp) CheckTx(req abci.RequestCheckTx) abci.ResponseCheckTx { | |
panic(fmt.Sprintf("unknown RequestCheckTx type: %s", req.Type)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
} | ||
|
@@ -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" | ||
|
||
|
@@ -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) | ||
|
@@ -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")) | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 ( | ||
|
@@ -97,6 +98,14 @@ func registerTestCodec(cdc *codec.LegacyAmino) { | |
cdc.RegisterConcrete(&msgNoRoute{}, "cosmos-sdk/baseapp/msgNoRoute", nil) | ||
} | ||
|
||
// aminoTxEncoder creates a amino TxEncoder for testing purposes. | ||
func aminoTxEncoder() sdk.TxEncoder { | ||
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...) | ||
|
@@ -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) | ||
|
@@ -1171,7 +1180,7 @@ func TestRunInvalidTransaction(t *testing.T) { | |
// transaction with no messages | ||
{ | ||
emptyTx := &txTest{} | ||
_, result, err := app.Deliver(emptyTx) | ||
_, result, err := app.Deliver(aminoTxEncoder(), emptyTx) | ||
require.Error(t, err) | ||
require.Nil(t, result) | ||
|
||
|
@@ -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(aminoTxEncoder(), tx) | ||
|
||
if testCase.fail { | ||
require.Error(t, err) | ||
|
@@ -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(aminoTxEncoder(), unknownRouteTx) | ||
require.Error(t, err) | ||
require.Nil(t, result) | ||
|
||
|
@@ -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(aminoTxEncoder(), unknownRouteTx) | ||
require.Error(t, err) | ||
require.Nil(t, result) | ||
|
||
|
@@ -1274,7 +1283,7 @@ func TestTxGasLimits(t *testing.T) { | |
} | ||
}() | ||
|
||
count := tx.(*txTest).Counter | ||
count := tx.(txTest).Counter | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
newCtx.GasMeter().ConsumeGas(uint64(count), "counter-ante") | ||
|
||
return newCtx, nil | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto (or not?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lol 🤷 |
||
ctx.GasMeter().ConsumeGas(uint64(count), "counter-handler") | ||
return &sdk.Result{}, nil | ||
}) | ||
|
@@ -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(aminoTxEncoder(), 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)) | ||
|
@@ -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 | ||
|
@@ -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 | ||
}) | ||
|
@@ -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(aminoTxEncoder(), tx) | ||
|
||
ctx := app.getState(runTxModeDeliver).ctx | ||
|
||
|
@@ -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(aminoTxEncoder(), tx) }) | ||
} | ||
} | ||
|
||
|
@@ -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 | ||
}) | ||
|
@@ -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(aminoTxEncoder(), tx) | ||
require.NoError(t, err) | ||
require.NotNil(t, resTx) | ||
res = app.Query(query) | ||
|
@@ -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(aminoTxEncoder(), tx) | ||
require.NoError(t, err) | ||
require.NotNil(t, resTx) | ||
res = app.Query(query) | ||
|
This file was deleted.
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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can telemetry panic?