From c456d757fdf484fb910d6b4bf730d721f03a245e Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Thu, 12 Dec 2024 10:48:16 +0100 Subject: [PATCH] fix: reject BlobTxs larger than 2 MiB (backport #4084) (#4109) ## Overview Directly checks transaction sizes even before they're decoded and removes them if they exceed configured threshold. We should add MaxTxSize constraint in ProcessProposal directly and consider removing the AnteHandler in v4. Issue tracking this work: https://github.com/celestiaorg/celestia-app/issues/4087 ## Testing - Check tx test asserts with logs that the expected error gets hit - Getting logs from prepare proposal was more challenging so i'm inserting a screenshot of application logs when the tests are run. Screenshot 2024-12-06 at 12 27 03 Screenshot 2024-12-06 at 12 27 20 ## Proposal for improving robustness of our test suites - [ ] Open an issue to assert all logs in our integration tests.
This is an automatic backport of pull request #4084 done by [Mergify](https://mergify.com). Co-authored-by: Nina Barbakadze --- app/check_tx.go | 11 ++++++ app/errors/errors.go | 12 ++++++ app/test/big_blob_test.go | 42 +++++++++++++++++++-- app/test/check_tx_test.go | 33 ++++++++++++++-- app/test/consistent_apphash_test.go | 8 +++- app/test/prepare_proposal_test.go | 22 +---------- app/test/process_proposal_test.go | 41 -------------------- test/util/blobfactory/payforblob_factory.go | 3 +- test/util/blobfactory/test_util.go | 2 +- 9 files changed, 102 insertions(+), 72 deletions(-) create mode 100644 app/errors/errors.go diff --git a/app/check_tx.go b/app/check_tx.go index f76b37880f..68ff288caa 100644 --- a/app/check_tx.go +++ b/app/check_tx.go @@ -3,6 +3,9 @@ package app import ( "fmt" + "cosmossdk.io/errors" + + apperr "github.com/celestiaorg/celestia-app/v3/app/errors" "github.com/celestiaorg/celestia-app/v3/pkg/appconsts" blobtypes "github.com/celestiaorg/celestia-app/v3/x/blob/types" blobtx "github.com/celestiaorg/go-square/v2/tx" @@ -15,6 +18,14 @@ import ( // transactions that contain blobs. func (app *App) CheckTx(req abci.RequestCheckTx) abci.ResponseCheckTx { tx := req.Tx + + // all txs must be less than or equal to the max tx size limit + maxTxSize := appconsts.MaxTxSize(app.AppVersion()) + currentTxSize := len(tx) + if currentTxSize > maxTxSize { + return sdkerrors.ResponseCheckTxWithEvents(errors.Wrapf(apperr.ErrTxExceedsMaxSize, "tx size %d bytes is larger than the application's configured MaxTxSize of %d bytes for version %d", currentTxSize, maxTxSize, app.AppVersion()), 0, 0, []abci.Event{}, false) + } + // check if the transaction contains blobs btx, isBlob, err := blobtx.UnmarshalBlobTx(tx) if isBlob && err != nil { diff --git a/app/errors/errors.go b/app/errors/errors.go new file mode 100644 index 0000000000..9bbff18b42 --- /dev/null +++ b/app/errors/errors.go @@ -0,0 +1,12 @@ +package errors + +import ( + "cosmossdk.io/errors" +) + +const AppErrorsCodespace = "app" + +// general application errors +var ( + ErrTxExceedsMaxSize = errors.Register(AppErrorsCodespace, 11142, "exceeds max tx size limit") +) diff --git a/app/test/big_blob_test.go b/app/test/big_blob_test.go index cede1ae65d..bbce473016 100644 --- a/app/test/big_blob_test.go +++ b/app/test/big_blob_test.go @@ -7,6 +7,7 @@ import ( "github.com/celestiaorg/celestia-app/v3/app" "github.com/celestiaorg/celestia-app/v3/app/encoding" + apperrors "github.com/celestiaorg/celestia-app/v3/app/errors" "github.com/celestiaorg/celestia-app/v3/pkg/appconsts" "github.com/celestiaorg/celestia-app/v3/pkg/user" "github.com/celestiaorg/celestia-app/v3/test/util/testfactory" @@ -55,7 +56,7 @@ func (s *BigBlobSuite) SetupSuite() { require.NoError(t, cctx.WaitForNextBlock()) } -// TestErrBlobsTooLarge verifies that submitting a 2 MiB blob hits ErrBlobsTooLarge. +// TestErrBlobsTooLarge verifies that submitting a ~1.9 MiB blob hits ErrBlobsTooLarge. func (s *BigBlobSuite) TestErrBlobsTooLarge() { t := s.T() @@ -67,8 +68,8 @@ func (s *BigBlobSuite) TestErrBlobsTooLarge() { } testCases := []testCase{ { - name: "2 mebibyte blob", - blob: newBlobWithSize(2 * mebibyte), + name: "~ 1.9 MiB blob", + blob: newBlobWithSize(2_000_000), want: blobtypes.ErrBlobsTooLarge.ABCICode(), }, } @@ -88,3 +89,38 @@ func (s *BigBlobSuite) TestErrBlobsTooLarge() { }) } } + +// TestBlobExceedsMaxTxSize verifies that submitting a 2 MiB blob hits ErrTxExceedsMaxSize. +func (s *BigBlobSuite) TestBlobExceedsMaxTxSize() { + t := s.T() + + type testCase struct { + name string + blob *share.Blob + expectedCode uint32 + expectedErr string + } + testCases := []testCase{ + { + name: "2 MiB blob", + blob: newBlobWithSize(2097152), + expectedCode: apperrors.ErrTxExceedsMaxSize.ABCICode(), + expectedErr: apperrors.ErrTxExceedsMaxSize.Error(), + }, + } + + txClient, err := testnode.NewTxClientFromContext(s.cctx) + require.NoError(t, err) + + for _, tc := range testCases { + s.Run(tc.name, func() { + subCtx, cancel := context.WithTimeout(s.cctx.GoContext(), 30*time.Second) + defer cancel() + res, err := txClient.SubmitPayForBlob(subCtx, []*share.Blob{tc.blob}, user.SetGasLimitAndGasPrice(1e9, appconsts.DefaultMinGasPrice)) + require.Error(t, err) + require.Nil(t, res) + code := err.(*user.BroadcastTxError).Code + require.Equal(t, tc.expectedCode, code, err.Error(), tc.expectedErr) + }) + } +} diff --git a/app/test/check_tx_test.go b/app/test/check_tx_test.go index aba7720547..87c7cc9e01 100644 --- a/app/test/check_tx_test.go +++ b/app/test/check_tx_test.go @@ -11,6 +11,7 @@ import ( "github.com/celestiaorg/celestia-app/v3/app" "github.com/celestiaorg/celestia-app/v3/app/encoding" + apperr "github.com/celestiaorg/celestia-app/v3/app/errors" "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" @@ -32,7 +33,7 @@ func TestCheckTx(t *testing.T) { ns1, err := share.NewV0Namespace(bytes.Repeat([]byte{1}, share.NamespaceVersionZeroIDSize)) require.NoError(t, err) - accs := []string{"a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k"} + accs := []string{"a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k", "l", "m"} testApp, kr := testutil.SetupTestAppWithGenesisValSet(app.DefaultConsensusParams(), accs...) testApp.Commit() @@ -173,11 +174,11 @@ func TestCheckTx(t *testing.T) { expectedABCICode: abci.CodeTypeOK, }, { - name: "10,000,000 byte blob", + name: "2,000,000 byte blob", checkType: abci.CheckTxType_New, getTx: func() []byte { signer := createSigner(t, kr, accs[9], encCfg.TxConfig, 10) - _, blobs := blobfactory.RandMsgPayForBlobsWithSigner(tmrand.NewRand(), signer.Account(accs[9]).Address().String(), 10_000_000, 1) + _, blobs := blobfactory.RandMsgPayForBlobsWithSigner(tmrand.NewRand(), signer.Account(accs[9]).Address().String(), 2_000_000, 1) tx, _, err := signer.CreatePayForBlobs(accs[9], blobs, opts...) require.NoError(t, err) return tx @@ -217,6 +218,32 @@ func TestCheckTx(t *testing.T) { }, expectedABCICode: abci.CodeTypeOK, }, + { + name: "v1 blob over 2MiB", + checkType: abci.CheckTxType_New, + getTx: func() []byte { + signer := createSigner(t, kr, accs[11], encCfg.TxConfig, 12) + blob, err := share.NewV1Blob(share.RandomBlobNamespace(), bytes.Repeat([]byte{1}, 2097152), signer.Account(accs[11]).Address()) + require.NoError(t, err) + blobTx, _, err := signer.CreatePayForBlobs(accs[11], []*share.Blob{blob}, opts...) + require.NoError(t, err) + return blobTx + }, + expectedABCICode: apperr.ErrTxExceedsMaxSize.ABCICode(), + }, + { + name: "v0 blob over 2MiB", + checkType: abci.CheckTxType_New, + getTx: func() []byte { + signer := createSigner(t, kr, accs[12], encCfg.TxConfig, 13) + blob, err := share.NewV0Blob(share.RandomBlobNamespace(), bytes.Repeat([]byte{1}, 2097152)) + require.NoError(t, err) + blobTx, _, err := signer.CreatePayForBlobs(accs[12], []*share.Blob{blob}, opts...) + require.NoError(t, err) + return blobTx + }, + expectedABCICode: apperr.ErrTxExceedsMaxSize.ABCICode(), + }, } for _, tt := range tests { diff --git a/app/test/consistent_apphash_test.go b/app/test/consistent_apphash_test.go index 9a108e4dd1..2acc1030ae 100644 --- a/app/test/consistent_apphash_test.go +++ b/app/test/consistent_apphash_test.go @@ -62,6 +62,10 @@ type appHashTest struct { expectedAppHash []byte } +func DefaultTxOpts() []user.TxOption { + return blobfactory.FeeTxOpts(10_000_000) +} + // TestConsistentAppHash executes all state machine messages on all app versions, generates an app hash, // and compares it against a previously generated hash from the same set of transactions. // App hashes across different commits should be consistent. @@ -377,7 +381,7 @@ func createEncodedBlobTx(t *testing.T, signer *user.Signer, accountAddresses []s blobTx := blobTx{ author: senderAcc.Name(), blobs: []*share.Blob{blob}, - txOptions: blobfactory.DefaultTxOpts(), + txOptions: DefaultTxOpts(), } encodedBlobTx, _, err := signer.CreatePayForBlobs(blobTx.author, blobTx.blobs, blobTx.txOptions...) require.NoError(t, err) @@ -429,7 +433,7 @@ func deterministicKeyRing(cdc codec.Codec) (keyring.Keyring, []types.PubKey) { func processSdkMessages(signer *user.Signer, sdkMessages []sdk.Msg) ([][]byte, error) { encodedTxs := make([][]byte, 0, len(sdkMessages)) for _, msg := range sdkMessages { - encodedTx, err := signer.CreateTx([]sdk.Msg{msg}, blobfactory.DefaultTxOpts()...) + encodedTx, err := signer.CreateTx([]sdk.Msg{msg}, DefaultTxOpts()...) if err != nil { return nil, err } diff --git a/app/test/prepare_proposal_test.go b/app/test/prepare_proposal_test.go index f274d9da6a..b167419a15 100644 --- a/app/test/prepare_proposal_test.go +++ b/app/test/prepare_proposal_test.go @@ -51,7 +51,6 @@ func TestPrepareProposalPutsPFBsAtEnd(t *testing.T) { accnts[:numBlobTxs], infos[:numBlobTxs], testfactory.Repeat([]*share.Blob{protoBlob}, numBlobTxs), - blobfactory.DefaultTxOpts()..., ) normalTxs := testutil.SendTxsWithAccounts( @@ -109,7 +108,6 @@ 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 @@ -173,22 +171,6 @@ func TestPrepareProposalFiltering(t *testing.T) { // 3 transactions over MaxTxSize 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 MaxTxSize 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 @@ -243,9 +225,9 @@ func TestPrepareProposalFiltering(t *testing.T) { { name: "blobTxs and sendTxs that exceed MaxTxSize limit", txs: func() [][]byte { - return append(largeTxs, largeBlobTxs...) // All txs are over MaxTxSize limit + return largeTxs // All txs are over MaxTxSize limit }, - prunedTxs: append(largeTxs, largeBlobTxs...), + prunedTxs: largeTxs, }, } diff --git a/app/test/process_proposal_test.go b/app/test/process_proposal_test.go index 33b1d406c5..093116a85b 100644 --- a/app/test/process_proposal_test.go +++ b/app/test/process_proposal_test.go @@ -3,7 +3,6 @@ package app_test import ( "bytes" "fmt" - "strings" "testing" "time" @@ -51,26 +50,6 @@ func TestProcessProposal(t *testing.T) { testfactory.RandomBlobNamespaces(tmrand.NewRand(), 4), [][]int{{100}, {1000}, {420}, {300}}, ), - blobfactory.DefaultTxOpts()..., - ) - - largeMemo := strings.Repeat("a", appconsts.MaxTxSize(appconsts.LatestVersion)) - - // create 2 single blobTxs that include a large memo making the transaction - // larger than the configured max tx size - 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 size 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 @@ -349,26 +328,6 @@ func TestProcessProposal(t *testing.T) { appVersion: v3.Version, expectedResult: abci.ResponseProcessProposal_REJECT, }, - { - name: "blob txs larger than configured max tx size", - 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 size", - 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/test/util/blobfactory/payforblob_factory.go b/test/util/blobfactory/payforblob_factory.go index f9b05359bc..d15e7ca0fe 100644 --- a/test/util/blobfactory/payforblob_factory.go +++ b/test/util/blobfactory/payforblob_factory.go @@ -245,14 +245,13 @@ func ManyMultiBlobTx( accounts []string, accInfos []AccountInfo, blobs [][]*share.Blob, - opts ...user.TxOption, ) [][]byte { t.Helper() txs := make([][]byte, len(accounts)) 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) - txs[i], _, err = signer.CreatePayForBlobs(acc, blobs[i], opts...) + txs[i], _, err = signer.CreatePayForBlobs(acc, blobs[i], DefaultTxOpts()...) require.NoError(t, err) } return txs diff --git a/test/util/blobfactory/test_util.go b/test/util/blobfactory/test_util.go index 0496ba1735..90a0697d1b 100644 --- a/test/util/blobfactory/test_util.go +++ b/test/util/blobfactory/test_util.go @@ -11,7 +11,7 @@ import ( ) func DefaultTxOpts() []user.TxOption { - return FeeTxOpts(10_000_000) + return FeeTxOpts(10_000_000_000) } func FeeTxOpts(gas uint64) []user.TxOption {