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 8 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
4 changes: 2 additions & 2 deletions baseapp/baseapp_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1118,13 +1118,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
31 changes: 27 additions & 4 deletions baseapp/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,42 @@ package baseapp
import (
tmproto "github.com/tendermint/tendermint/proto/tendermint/types"

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

// txEncoder creates a proto TxEncoder for testing purposes.
func txEncoder(interfaceRegistry codectypes.InterfaceRegistry) sdk.TxEncoder {
marshaler := codec.NewProtoCodec(interfaceRegistry)
txCfg := tx.NewTxConfig(marshaler, tx.DefaultSignModes)

return txCfg.TxEncoder()
}
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved

func (app *BaseApp) Check(tx sdk.Tx) (sdk.GasInfo, *sdk.Result, error) {
return app.runTx(runTxModeCheck, nil, tx)
// 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(app.interfaceRegistry)(tx)
if err != nil {
return sdk.GasInfo{}, nil, err
}
return app.runTx(runTxModeCheck, bz)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest to put all these functions inside this file into a baseapp/testutil package (in a follow-up PR).

}

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

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

// Context with current {check, deliver}State of the app used by tests.
Expand Down
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
2 changes: 1 addition & 1 deletion simapp/test_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,7 @@ func SignCheckDeliver(
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 Down
58 changes: 58 additions & 0 deletions x/auth/client/cli/cli_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ import (
"github.com/cosmos/cosmos-sdk/testutil/network"
"github.com/cosmos/cosmos-sdk/testutil/testdata"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/tx"
"github.com/cosmos/cosmos-sdk/types/tx/signing"
authcli "github.com/cosmos/cosmos-sdk/x/auth/client/cli"
authtest "github.com/cosmos/cosmos-sdk/x/auth/client/testutil"
authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
Expand Down Expand Up @@ -833,6 +835,62 @@ func (s *IntegrationTestSuite) TestQueryParamsCmd() {
}
}

// TestTxWithoutPublicKey makes sure sending a proto tx message without the
// public key doesn't cause any error in the RPC layer (broadcast).
// See https://github.com/cosmos/cosmos-sdk/issues/7585 for more details.
func (s *IntegrationTestSuite) TestTxWithoutPublicKey() {
val1 := s.network.Validators[0]
txCfg := val1.ClientCtx.TxConfig

// Create a txBuilder with an unsigned tx.
txBuilder := txCfg.NewTxBuilder()
msg := banktypes.NewMsgSend(val1.Address, val1.Address, sdk.NewCoins(
sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10)),
))
err := txBuilder.SetMsgs(msg)
s.Require().NoError(err)
txBuilder.SetFeeAmount(testdata.NewTestFeeAmount())
txBuilder.SetGasLimit(testdata.NewTestGasLimit())
// Set empty signature to set signer infos.
sigV2 := signing.SignatureV2{
PubKey: val1.PubKey,
Data: &signing.SingleSignatureData{
SignMode: txCfg.SignModeHandler().DefaultMode(),
Signature: nil,
},
}
err = txBuilder.SetSignatures(sigV2)
s.Require().NoError(err)

// Create a file with the unsigned tx.
txJSON, err := txCfg.TxJSONEncoder()(txBuilder.GetTx())
s.Require().NoError(err)
unsignedTxFile, cleanup := testutil.WriteToNewTempFile(s.T(), string(txJSON))
defer cleanup()

// Sign the file with the unsignedTx.
signedTx, err := authtest.TxSignExec(val1.ClientCtx, val1.Address, unsignedTxFile.Name())
s.Require().NoError(err)

// Remove the signerInfo's `public_key` field manually from the signedTx.
// Note: this method is only used for test purposes! In general, one should
// use txBuilder and TxEncoder/TxDecoder to manipulate txs.
var tx tx.Tx
err = val1.ClientCtx.JSONMarshaler.UnmarshalJSON(signedTx.Bytes(), &tx)
s.Require().NoError(err)
tx.AuthInfo.SignerInfos[0].PublicKey = nil
// Re-encode the tx again, to another file.
txJSON, err = val1.ClientCtx.JSONMarshaler.MarshalJSON(&tx)
s.Require().NoError(err)
signedTxFile, cleanup2 := testutil.WriteToNewTempFile(s.T(), string(txJSON))
defer cleanup2()

// Broadcast tx, test that it shouldn't panic.
val1.ClientCtx.BroadcastMode = flags.BroadcastSync
_, err = authtest.TxBroadcastExec(val1.ClientCtx, signedTxFile.Name())
s.Require().EqualError(err, "TODO some error")
}

func TestIntegrationTestSuite(t *testing.T) {
suite.Run(t, new(IntegrationTestSuite))
}