From dd50ae7b7ea35484aa99f8027d394315c361385a Mon Sep 17 00:00:00 2001 From: Evan Forbes <42654277+evan-forbes@users.noreply.github.com> Date: Tue, 19 Sep 2023 07:28:41 -0500 Subject: [PATCH] chore: log filtered transactions and fix gas limits in tests (#2518) ## Overview This PR adds loggers back to the filter functions (but actually uses them this time), and logs when transaction get filtered during `PrepareProposal`. It also re-adds an obtuse gas limit to the transactions in the `TestPrepareProposalConsistency` test, where those errors were getting caught by the filter functions. part of #2517 ## Checklist - [x] New and updated code has appropriate documentation - [x] New and updated code has new and/or updated testing - [x] Required CI checks are passing - [x] Visual proof for any user facing features like CLI or documentation updates - [x] Linked issues closed with keywords --- app/prepare_proposal.go | 2 +- app/test/fuzz_abci_test.go | 3 ++ app/validate_txs.go | 49 ++++++++++++++------- test/util/malicious/out_of_order_prepare.go | 2 +- 4 files changed, 39 insertions(+), 17 deletions(-) diff --git a/app/prepare_proposal.go b/app/prepare_proposal.go index cda4e61e8f..a6edb0aff5 100644 --- a/app/prepare_proposal.go +++ b/app/prepare_proposal.go @@ -53,7 +53,7 @@ func (app *App) PrepareProposal(req abci.RequestPrepareProposal) abci.ResponsePr if app.LastBlockHeight() == 0 { txs = make([][]byte, 0) } else { - txs = FilterTxs(sdkCtx, handler, app.txConfig, req.BlockData.Txs) + txs = FilterTxs(app.Logger(), sdkCtx, handler, app.txConfig, req.BlockData.Txs) } // build the square from the set of valid and prioritised transactions. diff --git a/app/test/fuzz_abci_test.go b/app/test/fuzz_abci_test.go index 8e3fae9ef9..e7b933a35d 100644 --- a/app/test/fuzz_abci_test.go +++ b/app/test/fuzz_abci_test.go @@ -7,6 +7,7 @@ import ( "github.com/celestiaorg/celestia-app/app" "github.com/celestiaorg/celestia-app/app/encoding" "github.com/celestiaorg/celestia-app/pkg/appconsts" + "github.com/celestiaorg/celestia-app/pkg/user" testutil "github.com/celestiaorg/celestia-app/test/util" "github.com/stretchr/testify/require" abci "github.com/tendermint/tendermint/abci/types" @@ -103,6 +104,7 @@ func TestPrepareProposalConsistency(t *testing.T) { true, testutil.ChainID, accounts[:tt.count], + user.SetGasLimitAndFee(1_000_000_000, 0.1), ) // create 100 send transactions sendTxs := testutil.SendTxsWithAccounts( @@ -114,6 +116,7 @@ func TestPrepareProposalConsistency(t *testing.T) { accounts[0], accounts[len(accounts)-sendTxCount:], testutil.ChainID, + user.SetGasLimitAndFee(1_000_000, 0.1), ) txs = append(txs, sendTxs...) diff --git a/app/validate_txs.go b/app/validate_txs.go index be48033c28..2c13f12f28 100644 --- a/app/validate_txs.go +++ b/app/validate_txs.go @@ -3,6 +3,8 @@ package app import ( "github.com/cosmos/cosmos-sdk/client" sdk "github.com/cosmos/cosmos-sdk/types" + tmbytes "github.com/tendermint/tendermint/libs/bytes" + "github.com/tendermint/tendermint/libs/log" tmproto "github.com/tendermint/tendermint/proto/tendermint/types" coretypes "github.com/tendermint/tendermint/types" ) @@ -23,25 +25,35 @@ func separateTxs(_ client.TxConfig, rawTxs [][]byte) ([][]byte, []tmproto.BlobTx } // FilterTxs applies the antehandler to all proposed transactions and removes transactions that return an error. -func FilterTxs(ctx sdk.Context, handler sdk.AnteHandler, txConfig client.TxConfig, txs [][]byte) [][]byte { +func FilterTxs(logger log.Logger, ctx sdk.Context, handler sdk.AnteHandler, txConfig client.TxConfig, txs [][]byte) [][]byte { normalTxs, blobTxs := separateTxs(txConfig, txs) - normalTxs, ctx = filterStdTxs(txConfig.TxDecoder(), ctx, handler, normalTxs) - blobTxs, _ = filterBlobTxs(txConfig.TxDecoder(), ctx, handler, blobTxs) + normalTxs, ctx = filterStdTxs(logger, txConfig.TxDecoder(), ctx, handler, normalTxs) + blobTxs, _ = filterBlobTxs(logger, txConfig.TxDecoder(), ctx, handler, blobTxs) return append(normalTxs, encodeBlobTxs(blobTxs)...) } // filterStdTxs applies the provided antehandler to each transaction and removes // transactions that return an error. Panics are caught by the checkTxValidity // function used to apply the ante handler. -func filterStdTxs(dec sdk.TxDecoder, ctx sdk.Context, handler sdk.AnteHandler, txs [][]byte) ([][]byte, sdk.Context) { +func filterStdTxs(logger log.Logger, dec sdk.TxDecoder, ctx sdk.Context, handler sdk.AnteHandler, txs [][]byte) ([][]byte, sdk.Context) { n := 0 - var err error for _, tx := range txs { - ctx, err = checkTxValidity(dec, ctx, handler, tx) + sdkTx, err := dec(tx) + if err != nil { + logger.Error("decoding already checked transaction", "tx", tmbytes.HexBytes(coretypes.Tx(tx).Hash()), "error", err) + continue + } + ctx, err = handler(ctx, sdkTx, false) // either the transaction is invalid (ie incorrect nonce) and we // simply want to remove this tx, or we're catching a panic from one // of the anteHanders which is logged. if err != nil { + logger.Error( + "filtering already checked transaction", + "tx", tmbytes.HexBytes(coretypes.Tx(tx).Hash()), + "error", err, + "msgs", msgTypes(sdkTx), + ) continue } txs[n] = tx @@ -55,15 +67,22 @@ func filterStdTxs(dec sdk.TxDecoder, ctx sdk.Context, handler sdk.AnteHandler, t // filterBlobTxs applies the provided antehandler to each transaction // and removes transactions that return an error. Panics are caught by the checkTxValidity // function used to apply the ante handler. -func filterBlobTxs(dec sdk.TxDecoder, ctx sdk.Context, handler sdk.AnteHandler, txs []tmproto.BlobTx) ([]tmproto.BlobTx, sdk.Context) { +func filterBlobTxs(logger log.Logger, dec sdk.TxDecoder, ctx sdk.Context, handler sdk.AnteHandler, txs []tmproto.BlobTx) ([]tmproto.BlobTx, sdk.Context) { n := 0 - var err error for _, tx := range txs { - ctx, err = checkTxValidity(dec, ctx, handler, tx.Tx) + sdkTx, err := dec(tx.Tx) + if err != nil { + logger.Error("decoding already checked blob transaction", "tx", tmbytes.HexBytes(coretypes.Tx(tx.Tx).Hash()), "error", err) + continue + } + ctx, err = handler(ctx, sdkTx, false) // either the transaction is invalid (ie incorrect nonce) and we // simply want to remove this tx, or we're catching a panic from one // of the anteHanders which is logged. if err != nil { + logger.Error( + "filtering already checked blob transaction", "tx", tmbytes.HexBytes(coretypes.Tx(tx.Tx).Hash()), "error", err, + ) continue } txs[n] = tx @@ -74,13 +93,13 @@ func filterBlobTxs(dec sdk.TxDecoder, ctx sdk.Context, handler sdk.AnteHandler, return txs[:n], ctx } -func checkTxValidity(dec sdk.TxDecoder, ctx sdk.Context, handler sdk.AnteHandler, tx []byte) (sdk.Context, error) { - sdkTx, err := dec(tx) - if err != nil { - return ctx, err +func msgTypes(sdkTx sdk.Tx) []string { + msgs := sdkTx.GetMsgs() + msgNames := make([]string, len(msgs)) + for i, msg := range msgs { + msgNames[i] = sdk.MsgTypeURL(msg) } - - return handler(ctx, sdkTx, false) + return msgNames } func encodeBlobTxs(blobTxs []tmproto.BlobTx) [][]byte { diff --git a/test/util/malicious/out_of_order_prepare.go b/test/util/malicious/out_of_order_prepare.go index 39fe14e58c..ebc23bc31b 100644 --- a/test/util/malicious/out_of_order_prepare.go +++ b/test/util/malicious/out_of_order_prepare.go @@ -32,7 +32,7 @@ func (a *App) OutOfOrderPrepareProposal(req abci.RequestPrepareProposal) abci.Re a.IBCKeeper, ) - txs := app.FilterTxs(sdkCtx, handler, a.GetTxConfig(), req.BlockData.Txs) + txs := app.FilterTxs(a.Logger(), sdkCtx, handler, a.GetTxConfig(), req.BlockData.Txs) // build the square from the set of valid and prioritised transactions. // The txs returned are the ones used in the square and block