Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: reject BlobTxs larger than 2 MiB #4084

Merged
merged 17 commits into from
Dec 11, 2024
Merged

fix: reject BlobTxs larger than 2 MiB #4084

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
ab11446
fix: cap tx size
ninabarbakadze Dec 5, 2024
bc55122
Merge remote-tracking branch 'origin' into nina/fix-tx-size-cap
ninabarbakadze Dec 6, 2024
19b5053
test: prepare proposal filters blobs over limit
ninabarbakadze Dec 6, 2024
9cad25d
test: works for check tx
ninabarbakadze Dec 6, 2024
bcbfc60
fix: consistent apphash test
ninabarbakadze Dec 6, 2024
4350f35
test: remove tests for the logic that doesnt get hit
ninabarbakadze Dec 6, 2024
4943be3
test: fix big blobs test
ninabarbakadze Dec 6, 2024
c4fc1f6
chore: remove irrelevant comment
ninabarbakadze Dec 6, 2024
aa314e9
Update app/check_tx.go
ninabarbakadze Dec 9, 2024
951e78b
Update app/check_tx.go
ninabarbakadze Dec 9, 2024
826f56b
refactor: address comments
ninabarbakadze Dec 9, 2024
f0fca5b
style: lint
ninabarbakadze Dec 9, 2024
cbd099a
Merge branch 'main' into nina/fix-tx-size-cap
ninabarbakadze Dec 9, 2024
0e99e30
Update app/validate_txs.go
ninabarbakadze Dec 11, 2024
5cddecc
refactor: remove prepare proposal changes and cleanup check tx
ninabarbakadze Dec 11, 2024
8fd03d4
Merge remote-tracking branch 'origin' into nina/fix-tx-size-cap
ninabarbakadze Dec 11, 2024
7b109bd
merge
ninabarbakadze Dec 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions app/check_tx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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 threshold of %d bytes", currentTxSize, maxTxSize), 0, 0, []abci.Event{}, false)
}

// check if the transaction contains blobs
btx, isBlob, err := blobtx.UnmarshalBlobTx(tx)
if isBlob && err != nil {
Expand Down
12 changes: 12 additions & 0 deletions app/errors/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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")
ninabarbakadze marked this conversation as resolved.
Show resolved Hide resolved
)
42 changes: 39 additions & 3 deletions app/test/big_blob_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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()

Expand All @@ -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(),
},
}
Expand All @@ -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)
})
}
}
41 changes: 38 additions & 3 deletions app/test/check_tx_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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()
Expand All @@ -44,6 +45,7 @@ func TestCheckTx(t *testing.T) {
checkType abci.CheckTxType
getTx func() []byte
expectedABCICode uint32
expectedLog string
Copy link
Contributor

@cmwaters cmwaters Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Does the expected log really help if we now have error codes we can assert

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i guess it's redundant in check tx

}

tests := []test{
Expand Down Expand Up @@ -96,6 +98,7 @@ func TestCheckTx(t *testing.T) {
return bbtx
},
expectedABCICode: blobtypes.ErrNamespaceMismatch.ABCICode(),
expectedLog: blobtypes.ErrNamespaceMismatch.Error(),
},
{
name: "PFB with no blob, CheckTxType_New",
Expand All @@ -111,6 +114,7 @@ func TestCheckTx(t *testing.T) {
return dtx.Tx
},
expectedABCICode: blobtypes.ErrNoBlobs.ABCICode(),
expectedLog: blobtypes.ErrNoBlobs.Error(),
},
{
name: "normal blobTx w/ multiple blobs, CheckTxType_New",
Expand Down Expand Up @@ -173,16 +177,17 @@ 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)
Comment on lines 176 to +181
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[question] why does this test case have to change?

Do we need to make the checkTx invocation versioned?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CheckTx and PrepareProposal doesn't need version gating. please check me on this @evan-forbes

that needs to change because we want to hit ErrBlobsTooLarge ref #4084 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should be good not to version in this case yeah

tx, _, err := signer.CreatePayForBlobs(accs[9], blobs, opts...)
require.NoError(t, err)
return tx
},
expectedABCICode: blobtypes.ErrBlobsTooLarge.ABCICode(),
expectedLog: blobtypes.ErrBlobsTooLarge.Error(),
},
{
name: "v1 blob with invalid signer",
Expand All @@ -203,6 +208,7 @@ func TestCheckTx(t *testing.T) {
return blobTxBytes
},
expectedABCICode: blobtypes.ErrInvalidBlobSigner.ABCICode(),
expectedLog: blobtypes.ErrInvalidBlobSigner.Error(),
},
{
name: "v1 blob with valid signer",
Expand All @@ -217,12 +223,41 @@ 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
},
expectedLog: apperr.ErrTxExceedsMaxSize.Error(),
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
},
expectedLog: apperr.ErrTxExceedsMaxSize.Error(),
expectedABCICode: apperr.ErrTxExceedsMaxSize.ABCICode(),
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Standardize the blob size constants

There's an inconsistency in how the 2MB limit is specified:

  • Modified test uses 2,000,000 bytes
  • New tests use 2097152 bytes (2MiB)

Consider:

  1. Using a constant for the max size
  2. Standardizing between decimal (2,000,000) and binary (2097152) representations
+// Define at package level
+const (
+    // MaxBlobSize is 2MiB in bytes
+    MaxBlobSize = 2097152
+)

 // Use in test cases
-bytes.Repeat([]byte{1}, 2097152)
+bytes.Repeat([]byte{1}, MaxBlobSize)

Committable suggestion skipped: line range outside the PR's diff.

}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
resp := testApp.CheckTx(abci.RequestCheckTx{Type: tt.checkType, Tx: tt.getTx()})
assert.Equal(t, tt.expectedABCICode, resp.Code, resp.Log)
assert.Contains(t, resp.Log, tt.expectedLog)
})
}
}
Expand Down
8 changes: 6 additions & 2 deletions app/test/consistent_apphash_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
}
Expand Down
28 changes: 24 additions & 4 deletions app/test/prepare_proposal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ func TestPrepareProposalPutsPFBsAtEnd(t *testing.T) {
accnts[:numBlobTxs],
infos[:numBlobTxs],
testfactory.Repeat([]*share.Blob{protoBlob}, numBlobTxs),
blobfactory.DefaultTxOpts()...,
)

normalTxs := testutil.SendTxsWithAccounts(
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -173,6 +171,7 @@ 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()

maxTxSize := appconsts.MaxTxSize(testApp.AppVersion()) // max tx size for the latest version
// 3 blobTxs over MaxTxSize limit
largeBlobTxs := blobfactory.ManyMultiBlobTx(
t,
Expand All @@ -184,9 +183,23 @@ func TestPrepareProposalFiltering(t *testing.T) {
blobfactory.NestedBlobs(
t,
testfactory.RandomBlobNamespaces(tmrand.NewRand(), 3),
[][]int{{100}, {1000}, {420}},
ninabarbakadze marked this conversation as resolved.
Show resolved Hide resolved
[][]int{{maxTxSize + 1}, {maxTxSize + 1}, {maxTxSize + 1}},
),
)

// 3 blobTxs where one is over MaxTxSize limit
mixedSizeBlobTxs := blobfactory.ManyMultiBlobTx(
t,
encConf.TxConfig,
kr,
testutil.ChainID,
accounts[:3],
infos[:3],
blobfactory.NestedBlobs(
t,
testfactory.RandomBlobNamespaces(tmrand.NewRand(), 3),
[][]int{{2}, {2800}, {maxTxSize + 1}},
),
user.SetMemo(largeString),
)

type test struct {
Expand Down Expand Up @@ -247,6 +260,13 @@ func TestPrepareProposalFiltering(t *testing.T) {
},
prunedTxs: append(largeTxs, largeBlobTxs...),
},
{
name: "blobTxs with mixed sizes, one is over MaxTxSize limit",
txs: func() [][]byte {
return mixedSizeBlobTxs
},
prunedTxs: [][]byte{mixedSizeBlobTxs[2]},
},
}

for _, tt := range tests {
Expand Down
41 changes: 0 additions & 41 deletions app/test/process_proposal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package app_test
import (
"bytes"
"fmt"
"strings"
"testing"
"time"

Expand Down Expand Up @@ -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))
evan-forbes marked this conversation as resolved.
Show resolved Hide resolved

// 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
Expand Down Expand Up @@ -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,
evan-forbes marked this conversation as resolved.
Show resolved Hide resolved
},
{
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 {
Expand Down
Loading
Loading