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: #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. <img width="887" alt="Screenshot 2024-12-06 at 12 27 03" src="https://github.com/user-attachments/assets/bb701834-5a3d-4eef-85f2-07074ae18a27"> <img width="837" alt="Screenshot 2024-12-06 at 12 27 20" src="https://github.com/user-attachments/assets/651d9b87-3d65-43f4-a1a0-3874e03db455"> ## Proposal for improving robustness of our test suites - [ ] Open an issue to assert all logs in our integration tests. <hr>This is an automatic backport of pull request #4084 done by [Mergify](https://mergify.com). Co-authored-by: Nina Barbakadze <[email protected]>
celestiaorg · Dec 12, 2024 · c456d75 · c456d75
1 parent b9d48a6
commit c456d75
Show file tree

Hide file tree

Showing 9 changed files with 102 additions and 72 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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 {