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 8 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
9 changes: 9 additions & 0 deletions app/check_tx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,15 @@ import (
// transactions that contain blobs.
func (app *App) CheckTx(req abci.RequestCheckTx) abci.ResponseCheckTx {
tx := req.Tx

// all txs should be under the max tx size limit
ninabarbakadze marked this conversation as resolved.
Show resolved Hide resolved
ninabarbakadze marked this conversation as resolved.
Show resolved Hide resolved
maxTxSize := appconsts.MaxTxSize(app.AppVersion())
currentTxSize := len(tx)
if currentTxSize > appconsts.MaxTxSize(app.AppVersion()) {
ninabarbakadze marked this conversation as resolved.
Show resolved Hide resolved
err := fmt.Errorf("tx size %d bytes is larger than the application's configured threshold of %d bytes", currentTxSize, maxTxSize)
ninabarbakadze marked this conversation as resolved.
Show resolved Hide resolved
return sdkerrors.ResponseCheckTxWithEvents(err, 0, 0, []abci.Event{}, false)
}

// check if the transaction contains blobs
btx, isBlob, err := blobtx.UnmarshalBlobTx(tx)
if isBlob && err != nil {
Expand Down
4 changes: 2 additions & 2 deletions app/test/big_blob_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,8 @@ func (s *BigBlobSuite) TestErrBlobsTooLarge() {
}
testCases := []testCase{
{
name: "2 mebibyte blob",
blob: newBlobWithSize(2 * mebibyte),
name: "~ 1.9 mebibyte blob",
blob: newBlobWithSize(2_000_000),
ninabarbakadze marked this conversation as resolved.
Show resolved Hide resolved
want: blobtypes.ErrBlobsTooLarge.ABCICode(),
},
}
Expand Down
38 changes: 35 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 @@ -32,7 +32,9 @@ 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"}
var genericErrorCode uint32 = 1

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 +46,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 @@ -173,11 +176,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)
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
Expand Down Expand Up @@ -217,12 +220,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: "is larger than the application's configured threshold",
expectedABCICode: genericErrorCode,
},
{
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: "is larger than the application's configured threshold",
expectedABCICode: genericErrorCode,
},
}

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
17 changes: 16 additions & 1 deletion app/validate_txs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package app

import (
"fmt"

"github.com/celestiaorg/celestia-app/v3/pkg/appconsts"
"github.com/celestiaorg/go-square/v2/tx"
"github.com/cosmos/cosmos-sdk/client"
Expand Down Expand Up @@ -34,7 +36,20 @@ func separateTxs(_ client.TxConfig, rawTxs [][]byte) ([][]byte, []*tx.BlobTx) {
//
// Side-effect: arranges all normal transactions before all blob transactions.
func FilterTxs(logger log.Logger, ctx sdk.Context, handler sdk.AnteHandler, txConfig client.TxConfig, txs [][]byte) [][]byte {
normalTxs, blobTxs := separateTxs(txConfig, txs)
// all transactions should be below the max tx size
ninabarbakadze marked this conversation as resolved.
Show resolved Hide resolved
maxTxSize := appconsts.MaxTxSize(ctx.BlockHeader().Version.App)
//nolint:prealloc
var txsBelowLimit [][]byte
for idx, tx := range txs {
if len(tx) > maxTxSize {
err := fmt.Sprintf("tx size %d bytes at index %d exceeds the application's configured threshold of %d bytes", len(tx), idx, maxTxSize)
Copy link
Collaborator

Choose a reason for hiding this comment

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

[uber nit] in some places we call the max tx size a "limit". In other places we call it a "threshold". It would be nice if we always referred to this with the same vocabulary.

In my opinion it would be sufficient to call it MaxTxSize and not use "limit" or "threshold" because the prefix "max" already conveys that.

Copy link
Member Author

Choose a reason for hiding this comment

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

5cddecc

logger.Error(err)
continue
}
txsBelowLimit = append(txsBelowLimit, tx)
}
Copy link
Member

Choose a reason for hiding this comment

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

why not check sizes of PFBs since that's what we care about? This would also allow us to keep logic in antehandlers instead of making a special case and having to remember that we have this logic hidden in this function.

side note: in theory, a consensus breaking version would be handled in the ValidateBasic method since this is a stateless check on PFBs only. We can't do that now without also passing the app version to all validate basic methods, but we need to that eventually anyway ref #4088

Copy link
Member Author

Choose a reason for hiding this comment

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

@cmwaters suggested that we should reject encoded transactions directly if the size is over 2MiB. This makes sense to me since we care about transactions not being over limit not only the blobs right? because when this tx is getting gossiped it'd be encoded right?

I'm not sure we can change the AnteHandler in non breaking way. I see it as a temporary fix in place until we can clean this up and break things in v4.

Copy link
Contributor

@cmwaters cmwaters Dec 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We could have the check in the antehandler, we would just need to have specific logic for the case of a PFB where we count the the blob sizes as well as the the tx bytes in the context.

This would also allow us to keep logic in antehandlers instead of making a special case and having to remember that we have this logic hidden in this function.

Antehandlers aren't the only source of validation, we do a lot of blob specific validity checks outside the antehandlers

Having it prior to being unmarshalled is the simplest and most likely safest way to implement a max tx size limit. There is no custom logic based on specific messages

Copy link
Member

Choose a reason for hiding this comment

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

I'm good with putting the logic in umarshaling or an antehandler 👍 I still don't like the idea of not being able to version validate basic logic.

I think its already a block validity that everything must be decodable. do we pass the app version to the encoding and decoding logic?

Copy link
Contributor

Choose a reason for hiding this comment

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

My view of validate basic was that it's a stateless (i.e. the function has no arguments). The app version is part of state. Having the app version being an argument to ValidateBasic seems incorrect (to me). The antehandler seems the right place to put version based checks in

I think its already a block validity that everything must be decodable. do we pass the app version to the encoding and decoding logic?

No, the codec currently contains all types, thus types across all versions can be marshalled or unmarshalled.

Copy link
Member

@evan-forbes evan-forbes Dec 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

if we checking this in checkTx, we don't need to add it here too, correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I think so. In theory we should be able to remove this and rely just on CheckTx. There might be an edge cases when we change version

Copy link
Member Author

Choose a reason for hiding this comment

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

5cddecc


normalTxs, blobTxs := separateTxs(txConfig, txsBelowLimit)
normalTxs, ctx = filterStdTxs(logger, txConfig.TxDecoder(), ctx, handler, normalTxs)
blobTxs, _ = filterBlobTxs(logger, txConfig.TxDecoder(), ctx, handler, blobTxs)
return append(normalTxs, encodeBlobTxs(blobTxs)...)
Expand Down
3 changes: 1 addition & 2 deletions test/util/blobfactory/payforblob_factory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion test/util/blobfactory/test_util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Loading