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 all 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
33 changes: 33 additions & 0 deletions app/ante/max_tx_size.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
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)
}

currentTxSize := len(ctx.TxBytes())
maxTxBytes := appconsts.MaxTxBytes(ctx.BlockHeader().Version.App)
if currentTxSize > maxTxBytes {
bytesOverLimit := currentTxSize - maxTxBytes
return ctx, fmt.Errorf("tx size %d bytes is larger than the application's configured threshold of %d bytes. Please reduce the size by %d bytes", currentTxSize, maxTxBytes, bytesOverLimit)
}
return next(ctx, tx, simulate)
}
78 changes: 78 additions & 0 deletions app/ante/max_tx_size_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
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
isCheckTx []bool
}{
{
name: "good tx; under max tx bytes threshold",
txSize: v3.MaxTxBytes - 1,
appVersion: v3.Version,
expectError: false,
isCheckTx: []bool{true, false},
},
{
name: "bad tx; over max tx bytes threshold",
txSize: v3.MaxTxBytes + 1,
appVersion: v3.Version,
expectError: true,
isCheckTx: []bool{true, false},
},
{
name: "good tx; equal to max tx bytes threshold",
txSize: v3.MaxTxBytes,
appVersion: v3.Version,
expectError: false,
isCheckTx: []bool{true, false},
},
{
name: "good tx; limit only applies to v3 and above",
txSize: v3.MaxTxBytes + 10,
appVersion: v2.Version,
expectError: false,
isCheckTx: []bool{true, false},
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
for _, isCheckTx := range tc.isCheckTx {

ctx := sdk.NewContext(nil, tmproto.Header{
Version: version.Consensus{
App: tc.appVersion,
},
}, isCheckTx, nil)

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

ctx = ctx.WithTxBytes(txBytes)
_, err := anteHandler(ctx, nil, false)
if tc.expectError {
require.Error(t, err)
} else {
require.NoError(t, err)
}
}
})
}
}
File renamed without changes.
File renamed without changes.
5 changes: 5 additions & 0 deletions app/process_proposal.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,11 @@ func (app *App) ProcessProposal(req abci.RequestProcessProposal) (resp abci.Resp
// sdkCtx = sdkCtx.WithTxBytes(tx)

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)
}
Comment on lines +74 to +77
Copy link
Contributor

Choose a reason for hiding this comment

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

same nit here: this would be easier to follow if it was placed right after sdkCtx is initialised on line 54

Copy link
Member Author

Choose a reason for hiding this comment

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

it needs to be done while we're iterating over transactions in later lines no?

Copy link
Member Author

Choose a reason for hiding this comment

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

so you suggest i iterate over block txs another time after context is created and set WithTxBytes there?

Copy link
Contributor

Choose a reason for hiding this comment

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

ignore me. I misunderstood this


if err != nil {
if req.Header.Version.App == v1 {
// For appVersion 1, there was no block validity rule that all
Expand Down
33 changes: 33 additions & 0 deletions app/test/prepare_proposal_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package app_test

import (
"strings"
"testing"
"time"

Expand All @@ -16,6 +17,7 @@ import (
"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 +41,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 +99,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 @@ -153,6 +157,28 @@ func TestPrepareProposalFiltering(t *testing.T) {
),
)[0]

// memo is 2 MiB resulting in the transaction being over limit
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 @@ -204,6 +230,13 @@ func TestPrepareProposalFiltering(t *testing.T) {
},
prunedTxs: [][]byte{tooManyShareBtx},
},
{
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 @@ -50,6 +51,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 @@ -327,6 +348,26 @@ func TestProcessProposal(t *testing.T) {
appVersion: v3.Version,
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 @@ -51,6 +51,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 @@ -85,6 +89,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