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.

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

feat!: limit the max tx size to 2 MiB #3909

Merged
merged 20 commits into from
Oct 11, 2024
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
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
2 changes: 2 additions & 0 deletions app/ante/ante.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
31 changes: 31 additions & 0 deletions app/ante/max_tx_size.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
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)
}

maxTxBytes := appconsts.MaxTxBytes(ctx.BlockHeader().Version.App)
if len(ctx.TxBytes()) > maxTxBytes {
return ctx, fmt.Errorf("tx size is larger than the application's configured threshold: %d bytes", maxTxBytes)
Copy link
Collaborator

Choose a reason for hiding this comment

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

[nit] provide the current tx bytes in the error message so that a user knows how many bytes they need to trim from their tx.

Copy link
Member Author

Choose a reason for hiding this comment

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

}
return next(ctx, tx, simulate)
}
71 changes: 71 additions & 0 deletions app/ante/max_tx_size_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
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
}{
Copy link
Member

Choose a reason for hiding this comment

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

optional, but useful

we can add a bool to determine if this is checkTx here, then add that bool to the test sdk.Context as will with sdkCtx.WithIsCheckTx(true)

Copy link
Member Author

@ninabarbakadze ninabarbakadze Oct 10, 2024

Choose a reason for hiding this comment

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

both addressed 54b2b4d and a873099

{
name: "good tx; under max tx bytes threshold",
txSize: v3.MaxTxBytes - 1,
appVersion: v3.Version,
expectError: false,
},
{
name: "bad tx; over max tx bytes threshold",
txSize: v3.MaxTxBytes + 1,
appVersion: v3.Version,
expectError: true,
},
{
name: "good tx; equal to max tx bytes threshold",
txSize: v3.MaxTxBytes,
appVersion: v3.Version,
expectError: false,
},
{
name: "good tx; limit only applies to v3 and above",
txSize: v3.MaxTxBytes + 10,
appVersion: v2.Version,
expectError: false,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
ctx := sdk.NewContext(nil, tmproto.Header{
Version: version.Consensus{
App: tc.appVersion,
},
}, false, nil)

txBytes := make([]byte, tc.txSize)

ctx = ctx.WithTxBytes(txBytes)
_, err := anteHandler(ctx, nil, false)
if tc.expectError {
require.Error(t, err)
require.Contains(t, err.Error(), "tx size is larger than the application's configured threshold")
} else {
require.NoError(t, err)
}
})
}
}
File renamed without changes.
File renamed without changes.
6 changes: 6 additions & 0 deletions app/process_proposal.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,9 @@ func (app *App) ProcessProposal(req abci.RequestProcessProposal) (resp abci.Resp
return reject()
}

// Set the tx size on the context before calling the AnteHandler
sdkCtx = sdkCtx.WithTxBytes(tx)
Copy link
Collaborator

Choose a reason for hiding this comment

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

[question] does this need to be gated behind a version gate? @evan-forbes mentioned that it is consensus breaking

Suggested change
// Set the tx size on the context before calling the AnteHandler
sdkCtx = sdkCtx.WithTxBytes(tx)
// Set the tx bytes in the context for app version v3 and greater
if sdkCtx.BlockHeader().Version.App >= 3 {
sdkCtx = sdkCtx.WithTxBytes(tx)
}

Copy link
Member Author

Choose a reason for hiding this comment

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

me and @evan-forbes briefly discussed this yesterday but I don't think we reached a firm conclusion. ConsumeTxSizeGasDecorator also uses TxBytes set on the context so I guess it is consensus breaking? I could also reset it back to empty after MaxTxSizeDecorator is executed.

Copy link
Member Author

Choose a reason for hiding this comment

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

because even if we version gate it do we want it to be set during the ante handler execution in v3?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems like a bug that it wasn't set before ConsumeTxSizeGasDecorator.

do we want it to be set during the ante handler execution in v3?

IMO yes

Copy link
Member

Choose a reason for hiding this comment

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

I added this gate in #3960 (which we can close after this)

I think there's a chance it could be consensus breaking because of the ante handle that subtracts gas for the size of the tx. we don't currently set it in process proposal afaik, which would mean that's its possible to reject or accept a block if an account does or doesn't have enough gas. very unlikely, but I think it could happen.

Copy link
Member

@evan-forbes evan-forbes Oct 10, 2024

Choose a reason for hiding this comment

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

so we need a gate in processproposal

we don't need a gate in prepare proposal, we can start applying the logic whenever

Copy link
Member Author

Choose a reason for hiding this comment

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

makes sense! thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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


// we need to increment the sequence for every transaction so that
// the signature check below is accurate. this error only gets hit
// if the account in question doesn't exist.
Expand All @@ -115,6 +118,9 @@ func (app *App) ProcessProposal(req abci.RequestProcessProposal) (resp abci.Resp
return reject()
}

// Set the tx size on the context before calling the AnteHandler
sdkCtx = sdkCtx.WithTxBytes(tx)
Copy link
Collaborator

Choose a reason for hiding this comment

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

[same question] does this need to be gated behind a version check?

Can this be hoisted up so that it only needs to happen once in this file? Perhaps after this line:

		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)
		}
		// rest of file

Copy link
Member

Choose a reason for hiding this comment

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

referencing this comment #3909 (comment)

we need a gate for process proposal, we don't need a gate prepare proposal

Copy link
Member Author

Choose a reason for hiding this comment

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


// validated the PFB signature
sdkCtx, err = handler(sdkCtx, sdkTx, false)
if err != nil {
Expand Down
36 changes: 34 additions & 2 deletions app/test/prepare_proposal_test.go
Original file line number Diff line number Diff line change
@@ -1,21 +1,22 @@
package app_test

import (
"strings"
"testing"
"time"

tmrand "github.com/tendermint/tendermint/libs/rand"

"github.com/cosmos/cosmos-sdk/crypto/hd"
"github.com/cosmos/cosmos-sdk/crypto/keyring"
"github.com/stretchr/testify/require"
abci "github.com/tendermint/tendermint/abci/types"
tmrand "github.com/tendermint/tendermint/libs/rand"
tmproto "github.com/tendermint/tendermint/proto/tendermint/types"
coretypes "github.com/tendermint/tendermint/types"

"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"
Expand All @@ -39,6 +40,7 @@ func TestPrepareProposalPutsPFBsAtEnd(t *testing.T) {
accnts[:numBlobTxs],
infos[:numBlobTxs],
testfactory.Repeat([]*share.Blob{protoBlob}, numBlobTxs),
blobfactory.DefaultTxOpts()...,
)

normalTxs := testutil.SendTxsWithAccounts(
Expand Down Expand Up @@ -96,6 +98,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
Expand Down Expand Up @@ -137,6 +140,28 @@ func TestPrepareProposalFiltering(t *testing.T) {
require.NoError(t, err)
noAccountTx := []byte(testutil.SendTxWithManualSequence(t, encConf.TxConfig, kr, nilAccount, accounts[0], 1000, "", 0, 6))

// memo is 2MG resulting in the transaction being over limit
ninabarbakadze marked this conversation as resolved.
Show resolved Hide resolved
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
Expand Down Expand Up @@ -181,6 +206,13 @@ func TestPrepareProposalFiltering(t *testing.T) {
},
prunedTxs: [][]byte{noAccountTx},
},
{
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 {
Expand Down
41 changes: 41 additions & 0 deletions app/test/process_proposal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package app_test
import (
"bytes"
"fmt"
"strings"
"testing"
"time"

Expand Down Expand Up @@ -49,6 +50,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))
Comment on lines +59 to +68
Copy link
Collaborator

Choose a reason for hiding this comment

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

[question] how does this function create 2 single blobTxs? I don't see 2 used as an argument to ManyMultiBlobTx

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'll create as many blobs as the accounts you pass into the function


// 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),
)
Comment on lines +70 to 74
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the memo size is capped (256 bytes IIRC) so this will fail regardless

Copy link
Member Author

Choose a reason for hiding this comment

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

do you know where the memo check is? because it was failing with transaction too large error in the logs

Copy link
Contributor

Choose a reason for hiding this comment

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

It's called ValidateMemoDecorator

Copy link
Member Author

Choose a reason for hiding this comment

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

NewMaxTxSizeDecorator is called before so it'll be rejected for transaction being too large


// create 3 MsgSend transactions that are signed with valid account numbers
Expand Down Expand Up @@ -299,6 +320,26 @@ func TestProcessProposal(t *testing.T) {
appVersion: appconsts.LatestVersion,
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 {
Expand Down
8 changes: 8 additions & 0 deletions app/validate_txs.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,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)
Comment on lines +55 to +56
Copy link
Collaborator

Choose a reason for hiding this comment

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

[question] does this need to be version gated? In other words is it consensus breaking?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this would have been easier to follow if it was done once in PrepareProposal where the context actually is created

Copy link
Member

@evan-forbes evan-forbes Oct 11, 2024

Choose a reason for hiding this comment

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

we're setting the tx bytes twice here, but its not super important I don't think

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 think this is due to the merge. I'll update in a follow-up


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
Expand Down Expand Up @@ -83,6 +87,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)
Comment on lines +93 to +94
Copy link
Collaborator

Choose a reason for hiding this comment

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

[question] does this need to be version gated? In other words is it consensus breaking?

Copy link
Member Author

Choose a reason for hiding this comment

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


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
Expand Down
1 change: 1 addition & 0 deletions pkg/appconsts/v3/app_consts.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,5 @@ const (
SubtreeRootThreshold int = 64
TxSizeCostPerByte uint64 = 10
GasPerBlobByte uint32 = 8
MaxTxBytes int = 2097152 // 2 MiB in bytes
)
4 changes: 4 additions & 0 deletions pkg/appconsts/versioned_consts.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
6 changes: 6 additions & 0 deletions pkg/appconsts/versioned_consts_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
1 change: 1 addition & 0 deletions specs/src/ante_handler_v3.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

[not blocking] we should have considered naming it something else to avoid conflicting with the mempool parameter MaxTxBytes. For example MaxTxSize would've worked

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 can change it in a follow-up since i need to update the ante handler anyway :)

- 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.
Expand Down
2 changes: 1 addition & 1 deletion test/util/blobfactory/payforblob_factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Loading