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

import (
"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"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
)

// 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 {
evan-forbes marked this conversation as resolved.
Show resolved Hide resolved
if len(ctx.TxBytes()) >= appconsts.MaxTxBytes(ctx.BlockHeader().Version.App) {
return ctx, sdkerrors.ErrTxTooLarge
ninabarbakadze marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Correct the transaction size comparison operator.

The current condition rejects transactions that are exactly the maximum allowed size:

if len(ctx.TxBytes()) >= appconsts.MaxTxBytes(ctx.BlockHeader().Version.App) {

If the intention is to allow transactions up to and including the maximum size, consider changing the comparison operator to >:

-if len(ctx.TxBytes()) >= appconsts.MaxTxBytes(ctx.BlockHeader().Version.App) {
+if len(ctx.TxBytes()) > appconsts.MaxTxBytes(ctx.BlockHeader().Version.App) {

This change ensures that transactions equal to the maximum size are accepted, and only those exceeding it are rejected.

Copy link
Member

Choose a reason for hiding this comment

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

this is good to note, whichever is used, we simply need to document it

using strictly greater than makes sense to me, as we can just use that constant everywhere instead of subtracting 1 everywhere as well

Copy link
Contributor

Choose a reason for hiding this comment

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

I second this that it would be better if it were strictly greater than 2MB

}
}
return next(ctx, tx, simulate)
}
72 changes: 72 additions & 0 deletions app/ante/max_tx_size_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
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"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
"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: "bad tx; equal to max tx bytes threshold",
txSize: v3.MaxTxBytes,
appVersion: v3.Version,
expectError: true,
},
{
name: "only applies to v3 and above",
txSize: v3.MaxTxBytes,
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(), sdkerrors.ErrTxTooLarge.Error())
} else {
require.NoError(t, err)
}
})
}
}
File renamed without changes.
File renamed without changes.
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
Loading