-
Notifications
You must be signed in to change notification settings - Fork 318
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
Changes from 17 commits
fb9066f
dacf9e8
334e789
f7b7d59
89a4e69
a31c0b8
d906239
941f55b
4c7f063
567d832
7d34dfd
172e29b
156dc32
6dd6a1f
c73e687
12dfb01
54b2b4d
a873099
70b4174
f600227
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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) | ||
} |
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) | ||
} | ||
} | ||
}) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ package app_test | |
import ( | ||
"bytes" | ||
"fmt" | ||
"strings" | ||
"testing" | ||
"time" | ||
|
||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's called There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
// create 3 MsgSend transactions that are signed with valid account numbers | ||
|
@@ -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 { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 afterMaxTxSizeDecorator
is executed.There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.IMO yes
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense! thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a873099