Skip to content

Commit

Permalink
chore: log filtered transactions and fix gas limits in tests (#2518)
Browse files Browse the repository at this point in the history
## 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
  • Loading branch information
evan-forbes authored Sep 19, 2023
1 parent e271772 commit dd50ae7
Show file tree
Hide file tree
Showing 4 changed files with 39 additions and 17 deletions.
2 changes: 1 addition & 1 deletion app/prepare_proposal.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
3 changes: 3 additions & 0 deletions app/test/fuzz_abci_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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(
Expand All @@ -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...)

Expand Down
49 changes: 34 additions & 15 deletions app/validate_txs.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion test/util/malicious/out_of_order_prepare.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit dd50ae7

Please sign in to comment.