From dcea3132d85eac25c0422a956dd99d17a3f22d9f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?nina=20/=20=E1=83=9C=E1=83=98=E1=83=9C=E1=83=90?= Date: Fri, 11 Oct 2024 13:00:16 -0400 Subject: [PATCH] feat!: limit the max tx size to 2 MiB (#3909) ## Overview Fixes [#3686](https://github.com/celestiaorg/celestia-app/issues/3686) --------- Co-authored-by: Rootul P --- app/ante/ante.go | 2 + app/ante/max_tx_size.go | 33 ++++++++ app/ante/max_tx_size_test.go | 78 +++++++++++++++++++ app/ante/{tx_size.go => tx_size_gas.go} | 0 .../{tx_size_test.go => tx_size_gas_test.go} | 0 app/process_proposal.go | 5 ++ app/test/prepare_proposal_test.go | 33 ++++++++ app/test/process_proposal_test.go | 41 ++++++++++ app/validate_txs.go | 8 ++ pkg/appconsts/v3/app_consts.go | 1 + pkg/appconsts/versioned_consts.go | 4 + pkg/appconsts/versioned_consts_test.go | 6 ++ specs/src/ante_handler_v3.md | 1 + test/util/blobfactory/payforblob_factory.go | 2 +- 14 files changed, 213 insertions(+), 1 deletion(-) create mode 100644 app/ante/max_tx_size.go create mode 100644 app/ante/max_tx_size_test.go rename app/ante/{tx_size.go => tx_size_gas.go} (100%) rename app/ante/{tx_size_test.go => tx_size_gas_test.go} (100%) diff --git a/app/ante/ante.go b/app/ante/ante.go index 89b6c1d050..c82313b65e 100644 --- a/app/ante/ante.go +++ b/app/ante/ante.go @@ -32,6 +32,8 @@ func NewAnteHandler( // Set up the context with a gas meter. // Must be called before gas consumption occurs in any other decorator. ante.NewSetUpContextDecorator(), + // Ensure the tx is not larger than the configured threshold. + NewMaxTxSizeDecorator(), // Ensure the tx does not contain any extension options. ante.NewExtensionOptionsDecorator(nil), // Ensure the tx passes ValidateBasic. diff --git a/app/ante/max_tx_size.go b/app/ante/max_tx_size.go new file mode 100644 index 0000000000..2936081aca --- /dev/null +++ b/app/ante/max_tx_size.go @@ -0,0 +1,33 @@ +package ante + +import ( + "fmt" + + "github.com/celestiaorg/celestia-app/v3/pkg/appconsts" + v3 "github.com/celestiaorg/celestia-app/v3/pkg/appconsts/v3" + sdk "github.com/cosmos/cosmos-sdk/types" +) + +// MaxTxSizeDecorator ensures that a tx can not be larger than +// application's configured versioned constant. +type MaxTxSizeDecorator struct{} + +func NewMaxTxSizeDecorator() MaxTxSizeDecorator { + return MaxTxSizeDecorator{} +} + +// AnteHandle implements the AnteHandler interface. It ensures that tx size is under application's configured threshold. +func (d MaxTxSizeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) { + // Tx size rule applies to app versions v3 and onwards. + if ctx.BlockHeader().Version.App < v3.Version { + return next(ctx, tx, simulate) + } + + currentTxSize := len(ctx.TxBytes()) + maxTxBytes := appconsts.MaxTxBytes(ctx.BlockHeader().Version.App) + if currentTxSize > maxTxBytes { + bytesOverLimit := currentTxSize - maxTxBytes + return ctx, fmt.Errorf("tx size %d bytes is larger than the application's configured threshold of %d bytes. Please reduce the size by %d bytes", currentTxSize, maxTxBytes, bytesOverLimit) + } + return next(ctx, tx, simulate) +} diff --git a/app/ante/max_tx_size_test.go b/app/ante/max_tx_size_test.go new file mode 100644 index 0000000000..3847a0b6a1 --- /dev/null +++ b/app/ante/max_tx_size_test.go @@ -0,0 +1,78 @@ +package ante_test + +import ( + "testing" + + "github.com/celestiaorg/celestia-app/v3/app/ante" + v2 "github.com/celestiaorg/celestia-app/v3/pkg/appconsts/v2" + v3 "github.com/celestiaorg/celestia-app/v3/pkg/appconsts/v3" + sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/stretchr/testify/require" + tmproto "github.com/tendermint/tendermint/proto/tendermint/types" + version "github.com/tendermint/tendermint/proto/tendermint/version" +) + +func TestMaxTxSizeDecorator(t *testing.T) { + decorator := ante.NewMaxTxSizeDecorator() + anteHandler := sdk.ChainAnteDecorators(decorator) + + testCases := []struct { + name string + txSize int + expectError bool + appVersion uint64 + isCheckTx []bool + }{ + { + name: "good tx; under max tx bytes threshold", + txSize: v3.MaxTxBytes - 1, + appVersion: v3.Version, + expectError: false, + isCheckTx: []bool{true, false}, + }, + { + name: "bad tx; over max tx bytes threshold", + txSize: v3.MaxTxBytes + 1, + appVersion: v3.Version, + expectError: true, + isCheckTx: []bool{true, false}, + }, + { + name: "good tx; equal to max tx bytes threshold", + txSize: v3.MaxTxBytes, + appVersion: v3.Version, + expectError: false, + isCheckTx: []bool{true, false}, + }, + { + name: "good tx; limit only applies to v3 and above", + txSize: v3.MaxTxBytes + 10, + appVersion: v2.Version, + expectError: false, + isCheckTx: []bool{true, false}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + for _, isCheckTx := range tc.isCheckTx { + + ctx := sdk.NewContext(nil, tmproto.Header{ + Version: version.Consensus{ + App: tc.appVersion, + }, + }, isCheckTx, nil) + + txBytes := make([]byte, tc.txSize) + + ctx = ctx.WithTxBytes(txBytes) + _, err := anteHandler(ctx, nil, false) + if tc.expectError { + require.Error(t, err) + } else { + require.NoError(t, err) + } + } + }) + } +} diff --git a/app/ante/tx_size.go b/app/ante/tx_size_gas.go similarity index 100% rename from app/ante/tx_size.go rename to app/ante/tx_size_gas.go diff --git a/app/ante/tx_size_test.go b/app/ante/tx_size_gas_test.go similarity index 100% rename from app/ante/tx_size_test.go rename to app/ante/tx_size_gas_test.go diff --git a/app/process_proposal.go b/app/process_proposal.go index 2d091b6e81..68e574f67d 100644 --- a/app/process_proposal.go +++ b/app/process_proposal.go @@ -71,6 +71,11 @@ func (app *App) ProcessProposal(req abci.RequestProcessProposal) (resp abci.Resp // sdkCtx = sdkCtx.WithTxBytes(tx) sdkTx, err := app.txConfig.TxDecoder()(tx) + // Set the tx bytes in the context for app version v3 and greater + if sdkCtx.BlockHeader().Version.App >= 3 { + sdkCtx = sdkCtx.WithTxBytes(tx) + } + if err != nil { if req.Header.Version.App == v1 { // For appVersion 1, there was no block validity rule that all diff --git a/app/test/prepare_proposal_test.go b/app/test/prepare_proposal_test.go index 73bc5dfe5c..58ebc0daae 100644 --- a/app/test/prepare_proposal_test.go +++ b/app/test/prepare_proposal_test.go @@ -1,6 +1,7 @@ package app_test import ( + "strings" "testing" "time" @@ -16,6 +17,7 @@ import ( "github.com/celestiaorg/celestia-app/v3/app" "github.com/celestiaorg/celestia-app/v3/app/encoding" "github.com/celestiaorg/celestia-app/v3/pkg/appconsts" + "github.com/celestiaorg/celestia-app/v3/pkg/user" testutil "github.com/celestiaorg/celestia-app/v3/test/util" "github.com/celestiaorg/celestia-app/v3/test/util/blobfactory" "github.com/celestiaorg/celestia-app/v3/test/util/testfactory" @@ -39,6 +41,7 @@ func TestPrepareProposalPutsPFBsAtEnd(t *testing.T) { accnts[:numBlobTxs], infos[:numBlobTxs], testfactory.Repeat([]*share.Blob{protoBlob}, numBlobTxs), + blobfactory.DefaultTxOpts()..., ) normalTxs := testutil.SendTxsWithAccounts( @@ -96,6 +99,7 @@ func TestPrepareProposalFiltering(t *testing.T) { testfactory.RandomBlobNamespaces(tmrand.NewRand(), 3), [][]int{{100}, {1000}, {420}}, ), + blobfactory.DefaultTxOpts()..., ) // create 3 MsgSend transactions that are signed with valid account numbers @@ -153,6 +157,28 @@ func TestPrepareProposalFiltering(t *testing.T) { ), )[0] + // memo is 2 MiB resulting in the transaction being over limit + largeString := strings.Repeat("a", 2*1024*1024) + + // 3 transactions over MaxTxBytes limit + largeTxs := coretypes.Txs(testutil.SendTxsWithAccounts(t, testApp, encConf.TxConfig, kr, 1000, accounts[0], accounts[:3], testutil.ChainID, user.SetMemo(largeString))).ToSliceOfBytes() + + // 3 blobTxs over MaxTxBytes limit + largeBlobTxs := blobfactory.ManyMultiBlobTx( + t, + encConf.TxConfig, + kr, + testutil.ChainID, + accounts[:3], + infos[:3], + blobfactory.NestedBlobs( + t, + testfactory.RandomBlobNamespaces(tmrand.NewRand(), 3), + [][]int{{100}, {1000}, {420}}, + ), + user.SetMemo(largeString), + ) + type test struct { name string txs func() [][]byte @@ -204,6 +230,13 @@ func TestPrepareProposalFiltering(t *testing.T) { }, prunedTxs: [][]byte{tooManyShareBtx}, }, + { + name: "blobTxs and sendTxs that exceed MaxTxBytes limit", + txs: func() [][]byte { + return append(largeTxs, largeBlobTxs...) // All txs are over MaxTxBytes limit + }, + prunedTxs: append(largeTxs, largeBlobTxs...), + }, } for _, tt := range tests { diff --git a/app/test/process_proposal_test.go b/app/test/process_proposal_test.go index f890cfe0f1..3ec0756b67 100644 --- a/app/test/process_proposal_test.go +++ b/app/test/process_proposal_test.go @@ -3,6 +3,7 @@ package app_test import ( "bytes" "fmt" + "strings" "testing" "time" @@ -50,6 +51,26 @@ func TestProcessProposal(t *testing.T) { testfactory.RandomBlobNamespaces(tmrand.NewRand(), 4), [][]int{{100}, {1000}, {420}, {300}}, ), + blobfactory.DefaultTxOpts()..., + ) + + largeMemo := strings.Repeat("a", appconsts.MaxTxBytes(appconsts.LatestVersion)) + + // create 2 single blobTxs that include a large memo making the transaction + // larger than the configured max tx bytes + largeBlobTxs := blobfactory.ManyMultiBlobTx( + t, enc, kr, testutil.ChainID, accounts[3:], infos[3:], + blobfactory.NestedBlobs( + t, + testfactory.RandomBlobNamespaces(tmrand.NewRand(), 4), + [][]int{{100}, {1000}, {420}, {300}}, + ), + user.SetMemo(largeMemo)) + + // create 1 large sendTx that includes a large memo making the + // transaction over the configured max tx bytes limit + largeSendTx := testutil.SendTxsWithAccounts( + t, testApp, enc, kr, 1000, accounts[0], accounts[1:2], testutil.ChainID, user.SetMemo(largeMemo), ) // create 3 MsgSend transactions that are signed with valid account numbers @@ -327,6 +348,26 @@ func TestProcessProposal(t *testing.T) { appVersion: v3.Version, expectedResult: abci.ResponseProcessProposal_REJECT, }, + { + name: "blob txs larger than configured max tx bytes", + input: validData(), + mutator: func(d *tmproto.Data) { + d.Txs = append(d.Txs, largeBlobTxs...) + d.Hash = calculateNewDataHash(t, d.Txs) + }, + appVersion: appconsts.LatestVersion, + expectedResult: abci.ResponseProcessProposal_REJECT, + }, + { + name: "send tx larger than configured max tx bytes", + input: validData(), + mutator: func(d *tmproto.Data) { + d.Txs = append(coretypes.Txs(largeSendTx).ToSliceOfBytes(), d.Txs...) + d.Hash = calculateNewDataHash(t, d.Txs) + }, + appVersion: appconsts.LatestVersion, + expectedResult: abci.ResponseProcessProposal_REJECT, + }, } for _, tt := range tests { diff --git a/app/validate_txs.go b/app/validate_txs.go index c524fe25e6..d8f9c76611 100644 --- a/app/validate_txs.go +++ b/app/validate_txs.go @@ -51,6 +51,10 @@ func filterStdTxs(logger log.Logger, dec sdk.TxDecoder, ctx sdk.Context, handler logger.Error("decoding already checked transaction", "tx", tmbytes.HexBytes(coretypes.Tx(tx).Hash()), "error", err) continue } + + // Set the tx size on the context before calling the AnteHandler + ctx = ctx.WithTxBytes(tx) + 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 @@ -85,6 +89,10 @@ func filterBlobTxs(logger log.Logger, dec sdk.TxDecoder, ctx sdk.Context, handle logger.Error("decoding already checked blob transaction", "tx", tmbytes.HexBytes(coretypes.Tx(tx.Tx).Hash()), "error", err) continue } + + // Set the tx size on the context before calling the AnteHandler + ctx = ctx.WithTxBytes(tx.Tx) + 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 diff --git a/pkg/appconsts/v3/app_consts.go b/pkg/appconsts/v3/app_consts.go index 3fa92f49a0..2d16d59993 100644 --- a/pkg/appconsts/v3/app_consts.go +++ b/pkg/appconsts/v3/app_consts.go @@ -6,4 +6,5 @@ const ( SubtreeRootThreshold int = 64 TxSizeCostPerByte uint64 = 10 GasPerBlobByte uint32 = 8 + MaxTxBytes int = 2097152 // 2 MiB in bytes ) diff --git a/pkg/appconsts/versioned_consts.go b/pkg/appconsts/versioned_consts.go index c5ffb952a6..6a186afe71 100644 --- a/pkg/appconsts/versioned_consts.go +++ b/pkg/appconsts/versioned_consts.go @@ -42,6 +42,10 @@ func GasPerBlobByte(_ uint64) uint32 { return v3.GasPerBlobByte } +func MaxTxBytes(_ uint64) int { + return v3.MaxTxBytes +} + var ( DefaultSubtreeRootThreshold = SubtreeRootThreshold(LatestVersion) DefaultSquareSizeUpperBound = SquareSizeUpperBound(LatestVersion) diff --git a/pkg/appconsts/versioned_consts_test.go b/pkg/appconsts/versioned_consts_test.go index da29d71241..90a681b627 100644 --- a/pkg/appconsts/versioned_consts_test.go +++ b/pkg/appconsts/versioned_consts_test.go @@ -66,6 +66,12 @@ func TestVersionedConsts(t *testing.T) { expectedConstant: v3.GasPerBlobByte, got: appconsts.GasPerBlobByte(v3.Version), }, + { + name: "MaxTxBytes v3", + version: v3.Version, + expectedConstant: v3.MaxTxBytes, + got: appconsts.MaxTxBytes(v3.Version), + }, } for _, tc := range testCases { diff --git a/specs/src/ante_handler_v3.md b/specs/src/ante_handler_v3.md index 59f32d45d2..f379ef2d00 100644 --- a/specs/src/ante_handler_v3.md +++ b/specs/src/ante_handler_v3.md @@ -3,6 +3,7 @@ The AnteHandler chains together several decorators to ensure the following criteria are met for app version 3: - The tx does not contain any messages that are unsupported by the current app version. See `MsgVersioningGateKeeper`. +- The tx size is not larger than the application's configured versioned constant MaxTxBytes. - The tx does not contain any [extension options](https://github.com/cosmos/cosmos-sdk/blob/22c28366466e64ebf0df1ce5bec8b1130523552c/proto/cosmos/tx/v1beta1/tx.proto#L119-L122). - The tx passes `ValidateBasic()`. - The tx's [timeout_height](https://github.com/cosmos/cosmos-sdk/blob/22c28366466e64ebf0df1ce5bec8b1130523552c/proto/cosmos/tx/v1beta1/tx.proto#L115-L117) has not been reached if one is specified. diff --git a/test/util/blobfactory/payforblob_factory.go b/test/util/blobfactory/payforblob_factory.go index 1a4aa6d10c..f9b05359bc 100644 --- a/test/util/blobfactory/payforblob_factory.go +++ b/test/util/blobfactory/payforblob_factory.go @@ -245,10 +245,10 @@ func ManyMultiBlobTx( accounts []string, accInfos []AccountInfo, blobs [][]*share.Blob, + opts ...user.TxOption, ) [][]byte { t.Helper() txs := make([][]byte, len(accounts)) - opts := DefaultTxOpts() for i, acc := range accounts { signer, err := user.NewSigner(kr, enc, chainid, appconsts.LatestVersion, user.NewAccount(acc, accInfos[i].AccountNum, accInfos[i].Sequence)) require.NoError(t, err)