From ca222a8615849634930d8947603063921771c7c6 Mon Sep 17 00:00:00 2001 From: Evan Forbes <42654277+evan-forbes@users.noreply.github.com> Date: Thu, 10 Oct 2024 18:18:31 -0500 Subject: [PATCH] chore: optimize checkTx (#3954) ## Overview this PR makes a simple optimization for checkTx --------- Co-authored-by: Rootul P Co-authored-by: CHAMI Rachid --- app/process_proposal.go | 3 + app/test/prepare_proposal_test.go | 32 ++++ app/test/process_proposal_test.go | 28 ++++ app/validate_txs.go | 2 + x/blob/ante/blob_share_decorator.go | 13 +- x/blob/ante/blob_share_decorator_test.go | 189 +++++++++++------------ 6 files changed, 162 insertions(+), 105 deletions(-) diff --git a/app/process_proposal.go b/app/process_proposal.go index fc10bb88e2..2d091b6e81 100644 --- a/app/process_proposal.go +++ b/app/process_proposal.go @@ -67,6 +67,9 @@ func (app *App) ProcessProposal(req abci.RequestProcessProposal) (resp abci.Resp tx = blobTx.Tx } + // todo: uncomment once we're sure this isn't consensus breaking + // sdkCtx = sdkCtx.WithTxBytes(tx) + sdkTx, err := app.txConfig.TxDecoder()(tx) if err != nil { if req.Header.Version.App == v1 { diff --git a/app/test/prepare_proposal_test.go b/app/test/prepare_proposal_test.go index 8bfcc281f8..73bc5dfe5c 100644 --- a/app/test/prepare_proposal_test.go +++ b/app/test/prepare_proposal_test.go @@ -137,6 +137,22 @@ func TestPrepareProposalFiltering(t *testing.T) { require.NoError(t, err) noAccountTx := []byte(testutil.SendTxWithManualSequence(t, encConf.TxConfig, kr, nilAccount, accounts[0], 1000, "", 0, 6)) + // create a tx that can't be included in a 64 x 64 when accounting for the + // pfb along with the shares + tooManyShareBtx := blobfactory.ManyMultiBlobTx( + t, + encConf.TxConfig, + kr, + testutil.ChainID, + accounts[3:4], + infos[3:4], + blobfactory.NestedBlobs( + t, + testfactory.RandomBlobNamespaces(tmrand.NewRand(), 4000), + [][]int{repeat(4000, 1)}, + ), + )[0] + type test struct { name string txs func() [][]byte @@ -181,6 +197,13 @@ func TestPrepareProposalFiltering(t *testing.T) { }, prunedTxs: [][]byte{noAccountTx}, }, + { + name: "blob tx with too many shares", + txs: func() [][]byte { + return [][]byte{tooManyShareBtx} + }, + prunedTxs: [][]byte{tooManyShareBtx}, + }, } for _, tt := range tests { @@ -216,3 +239,12 @@ func queryAccountInfo(capp *app.App, accs []string, kr keyring.Keyring) []blobfa } return infos } + +// repeat returns a slice of length n with each element set to val. +func repeat[T any](n int, val T) []T { + result := make([]T, n) + for i := range result { + result[i] = val + } + return result +} diff --git a/app/test/process_proposal_test.go b/app/test/process_proposal_test.go index df390dcdfa..f890cfe0f1 100644 --- a/app/test/process_proposal_test.go +++ b/app/test/process_proposal_test.go @@ -20,6 +20,7 @@ import ( "github.com/celestiaorg/celestia-app/v3/pkg/appconsts" v1 "github.com/celestiaorg/celestia-app/v3/pkg/appconsts/v1" v2 "github.com/celestiaorg/celestia-app/v3/pkg/appconsts/v2" + v3 "github.com/celestiaorg/celestia-app/v3/pkg/appconsts/v3" "github.com/celestiaorg/celestia-app/v3/pkg/da" "github.com/celestiaorg/celestia-app/v3/pkg/user" testutil "github.com/celestiaorg/celestia-app/v3/test/util" @@ -80,6 +81,20 @@ func TestProcessProposal(t *testing.T) { ns1 := share.MustNewV0Namespace(bytes.Repeat([]byte{1}, share.NamespaceVersionZeroIDSize)) data := bytes.Repeat([]byte{1}, 13) + tooManyShareBtx := blobfactory.ManyMultiBlobTx( + t, + enc, + kr, + testutil.ChainID, + accounts[3:4], + infos[3:4], + blobfactory.NestedBlobs( + t, + testfactory.RandomBlobNamespaces(tmrand.NewRand(), 4000), + [][]int{repeat(4000, 1)}, + ), + )[0] + type test struct { name string input *tmproto.Data @@ -299,6 +314,19 @@ func TestProcessProposal(t *testing.T) { appVersion: appconsts.LatestVersion, expectedResult: abci.ResponseProcessProposal_REJECT, }, + { + name: "blob tx that takes up too many shares", + input: &tmproto.Data{ + Txs: [][]byte{}, + }, + mutator: func(d *tmproto.Data) { + // this tx will get filtered out by prepare proposal before this + // so we add it here + d.Txs = append(d.Txs, tooManyShareBtx) + }, + appVersion: v3.Version, + expectedResult: abci.ResponseProcessProposal_REJECT, + }, } for _, tt := range tests { diff --git a/app/validate_txs.go b/app/validate_txs.go index 3538e221a0..c524fe25e6 100644 --- a/app/validate_txs.go +++ b/app/validate_txs.go @@ -45,6 +45,7 @@ func FilterTxs(logger log.Logger, ctx sdk.Context, handler sdk.AnteHandler, txCo func filterStdTxs(logger log.Logger, dec sdk.TxDecoder, ctx sdk.Context, handler sdk.AnteHandler, txs [][]byte) ([][]byte, sdk.Context) { n := 0 for _, tx := range txs { + ctx = ctx.WithTxBytes(tx) sdkTx, err := dec(tx) if err != nil { logger.Error("decoding already checked transaction", "tx", tmbytes.HexBytes(coretypes.Tx(tx).Hash()), "error", err) @@ -78,6 +79,7 @@ func filterStdTxs(logger log.Logger, dec sdk.TxDecoder, ctx sdk.Context, handler func filterBlobTxs(logger log.Logger, dec sdk.TxDecoder, ctx sdk.Context, handler sdk.AnteHandler, txs []*tx.BlobTx) ([]*tx.BlobTx, sdk.Context) { n := 0 for _, tx := range txs { + ctx = ctx.WithTxBytes(tx.Tx) sdkTx, err := dec(tx.Tx) if err != nil { logger.Error("decoding already checked blob transaction", "tx", tmbytes.HexBytes(coretypes.Tx(tx.Tx).Hash()), "error", err) diff --git a/x/blob/ante/blob_share_decorator.go b/x/blob/ante/blob_share_decorator.go index 3a23f45588..90eb7780c6 100644 --- a/x/blob/ante/blob_share_decorator.go +++ b/x/blob/ante/blob_share_decorator.go @@ -36,7 +36,7 @@ func (d BlobShareDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool maxBlobShares := d.getMaxBlobShares(ctx) for _, m := range tx.GetMsgs() { if pfb, ok := m.(*blobtypes.MsgPayForBlobs); ok { - if sharesNeeded := getSharesNeeded(pfb.BlobSizes); sharesNeeded > maxBlobShares { + if sharesNeeded := getSharesNeeded(uint32(len(ctx.TxBytes())), pfb.BlobSizes); sharesNeeded > maxBlobShares { return ctx, errors.Wrapf(blobtypes.ErrBlobsTooLarge, "the number of shares occupied by blobs in this MsgPayForBlobs %d exceeds the max number of shares available for blob data %d", sharesNeeded, maxBlobShares) } } @@ -49,10 +49,8 @@ func (d BlobShareDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool func (d BlobShareDecorator) getMaxBlobShares(ctx sdk.Context) int { squareSize := d.getMaxSquareSize(ctx) totalShares := squareSize * squareSize - // The PFB tx share must occupy at least one share so the number of blob shares - // is at most one less than totalShares. - blobShares := totalShares - 1 - return blobShares + // the shares used up by the tx are calculated in `getSharesNeeded` + return totalShares } // getMaxSquareSize returns the maximum square size based on the current values @@ -74,8 +72,9 @@ func (d BlobShareDecorator) getMaxSquareSize(ctx sdk.Context) int { } // getSharesNeeded returns the total number of shares needed to represent all of -// the blobs described by blobSizes. -func getSharesNeeded(blobSizes []uint32) (sum int) { +// the blobs described by blobSizes along with the shares used by the tx +func getSharesNeeded(txSize uint32, blobSizes []uint32) (sum int) { + sum = share.CompactSharesNeeded(txSize) for _, blobSize := range blobSizes { sum += share.SparseSharesNeeded(blobSize) } diff --git a/x/blob/ante/blob_share_decorator_test.go b/x/blob/ante/blob_share_decorator_test.go index 6c788e1935..e65dca6c3f 100644 --- a/x/blob/ante/blob_share_decorator_test.go +++ b/x/blob/ante/blob_share_decorator_test.go @@ -7,12 +7,18 @@ import ( "github.com/celestiaorg/celestia-app/v3/app/encoding" v1 "github.com/celestiaorg/celestia-app/v3/pkg/appconsts/v1" v2 "github.com/celestiaorg/celestia-app/v3/pkg/appconsts/v2" + "github.com/celestiaorg/celestia-app/v3/pkg/user" + "github.com/celestiaorg/celestia-app/v3/test/util/blobfactory" + "github.com/celestiaorg/celestia-app/v3/test/util/testfactory" + "github.com/celestiaorg/celestia-app/v3/test/util/testnode" ante "github.com/celestiaorg/celestia-app/v3/x/blob/ante" blob "github.com/celestiaorg/celestia-app/v3/x/blob/types" "github.com/celestiaorg/go-square/v2/share" + blobtx "github.com/celestiaorg/go-square/v2/tx" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + tmrand "github.com/tendermint/tendermint/libs/rand" tmproto "github.com/tendermint/tendermint/proto/tendermint/types" version "github.com/tendermint/tendermint/proto/tendermint/version" ) @@ -24,141 +30,137 @@ const ( func TestBlobShareDecorator(t *testing.T) { type testCase struct { - name string - pfb *blob.MsgPayForBlobs - appVersion uint64 - wantErr error + name string + blobsPerPFB, blobSize int + appVersion uint64 + wantErr error } + rand := tmrand.NewRand() + testCases := []testCase{ { - name: "want no error if appVersion v1 and 8 MiB blob", - pfb: &blob.MsgPayForBlobs{ - BlobSizes: []uint32{8 * mebibyte}, - }, - appVersion: v1.Version, + name: "want no error if appVersion v1 and 8 MiB blob", + blobsPerPFB: 1, + blobSize: 8 * mebibyte, + appVersion: v1.Version, }, { - name: "PFB with 1 blob that is 1 byte", - pfb: &blob.MsgPayForBlobs{ - BlobSizes: []uint32{1}, - }, - appVersion: v2.Version, + name: "PFB with 1 blob that is 1 byte", + blobsPerPFB: 1, + blobSize: 1, + appVersion: v2.Version, }, { - name: "PFB with 1 blob that is 1 MiB", - pfb: &blob.MsgPayForBlobs{ - BlobSizes: []uint32{mebibyte}, - }, - appVersion: v2.Version, + name: "PFB with 1 blob that is 1 MiB", + blobsPerPFB: 1, + blobSize: 1 * mebibyte, + appVersion: v2.Version, }, { - name: "PFB with 1 blob that is 2 MiB", - pfb: &blob.MsgPayForBlobs{ - BlobSizes: []uint32{2 * mebibyte}, - }, - appVersion: v2.Version, + name: "PFB with 1 blob that is 2 MiB", + blobsPerPFB: 1, + blobSize: 2 * mebibyte, + appVersion: v2.Version, // This test case should return an error because a square size of 64 // has exactly 2 MiB of total capacity so the total blob capacity // will be slightly smaller than 2 MiB. wantErr: blob.ErrBlobsTooLarge, }, { - name: "PFB with 2 blobs that are 1 byte each", - pfb: &blob.MsgPayForBlobs{ - BlobSizes: []uint32{1, 1}, - }, - appVersion: v2.Version, + name: "PFB with 2 blobs that are 1 byte each", + blobsPerPFB: 2, + blobSize: 1, + appVersion: v2.Version, }, { - name: "PFB with 2 blobs that are 1 MiB each", - pfb: &blob.MsgPayForBlobs{ - BlobSizes: []uint32{mebibyte, mebibyte}, - }, - appVersion: v2.Version, + name: "PFB with 2 blobs that are 1 MiB each", + blobsPerPFB: 2, + blobSize: 1 * mebibyte, + appVersion: v2.Version, // This test case should return an error for the same reason a // single blob that is 2 MiB returns an error. wantErr: blob.ErrBlobsTooLarge, }, { - name: "PFB with many single byte blobs should fit", - pfb: &blob.MsgPayForBlobs{ - // 4095 blobs each of size 1 byte should occupy 4095 shares. - // When square size is 64, there are 4095 shares available to - // blob shares so we don't expect an error for this test case. - BlobSizes: repeat(4095, 1), - }, - appVersion: v2.Version, + name: "PFB with many single byte blobs should fit", + blobsPerPFB: 3000, + blobSize: 1, + appVersion: v2.Version, }, { - name: "PFB with too many single byte blobs should not fit", - pfb: &blob.MsgPayForBlobs{ - // 4096 blobs each of size 1 byte should occupy 4096 shares. - // When square size is 64, there are 4095 shares available to - // blob shares so we expect an error for this test case. - BlobSizes: repeat(4096, 1), - }, - appVersion: v2.Version, - wantErr: blob.ErrBlobsTooLarge, + name: "PFB with too many single byte blobs should not fit", + blobsPerPFB: 4000, + blobSize: 1, + appVersion: v2.Version, + wantErr: blob.ErrBlobsTooLarge, }, { - name: "PFB with 1 blob that is 1 share", - pfb: &blob.MsgPayForBlobs{ - BlobSizes: []uint32{uint32(share.AvailableBytesFromSparseShares(1))}, - }, - appVersion: v2.Version, + name: "PFB with 1 blob that is 1 share", + blobsPerPFB: 1, + blobSize: 100, + appVersion: v2.Version, }, { - name: "PFB with 1 blob that occupies total square - 1", - pfb: &blob.MsgPayForBlobs{ - BlobSizes: []uint32{uint32(share.AvailableBytesFromSparseShares((squareSize * squareSize) - 1))}, - }, - appVersion: v2.Version, + name: "PFB with 1 blob that occupies total square - 1", + blobsPerPFB: 1, + blobSize: share.AvailableBytesFromSparseShares(squareSize*squareSize - 1), + appVersion: v2.Version, }, { - name: "PFB with 1 blob that occupies total square", - pfb: &blob.MsgPayForBlobs{ - BlobSizes: []uint32{uint32(share.AvailableBytesFromSparseShares(squareSize * squareSize))}, - }, - appVersion: v2.Version, + name: "PFB with 1 blob that occupies total square", + blobsPerPFB: 1, + blobSize: share.AvailableBytesFromSparseShares(squareSize * squareSize), + appVersion: v2.Version, // This test case should return an error because if the blob // occupies the total square, there is no space for the PFB tx // share. wantErr: blob.ErrBlobsTooLarge, }, { - name: "PFB with 2 blobs that are 1 share each", - pfb: &blob.MsgPayForBlobs{ - BlobSizes: []uint32{ - uint32(share.AvailableBytesFromSparseShares(1)), - uint32(share.AvailableBytesFromSparseShares(1)), - }, - }, - appVersion: v2.Version, + name: "PFB with 2 blobs that are 1 share each", + blobsPerPFB: 2, + blobSize: 100, + appVersion: v2.Version, }, { - name: "PFB with 2 blobs that occupy half the square each", - pfb: &blob.MsgPayForBlobs{ - BlobSizes: []uint32{ - uint32(share.AvailableBytesFromSparseShares(squareSize * squareSize / 2)), - uint32(share.AvailableBytesFromSparseShares(squareSize * squareSize / 2)), - }, - }, - appVersion: v2.Version, - wantErr: blob.ErrBlobsTooLarge, + name: "PFB with 2 blobs that occupy half the square each", + blobsPerPFB: 2, + blobSize: share.AvailableBytesFromSparseShares(squareSize * squareSize / 2), + appVersion: v2.Version, + wantErr: blob.ErrBlobsTooLarge, }, } - txConfig := encoding.MakeConfig(app.ModuleEncodingRegisters...).TxConfig + ecfg := encoding.MakeConfig(app.ModuleEncodingRegisters...) + for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - txBuilder := txConfig.NewTxBuilder() - require.NoError(t, txBuilder.SetMsgs(tc.pfb)) - tx := txBuilder.GetTx() + kr, _ := testnode.NewKeyring(testfactory.TestAccName) + signer, err := user.NewSigner( + kr, + ecfg.TxConfig, + testfactory.ChainID, + tc.appVersion, + user.NewAccount(testfactory.TestAccName, 1, 0), + ) + require.NoError(t, err) + + blobTx := blobfactory.RandBlobTxs(signer, rand, 1, tc.blobsPerPFB, tc.blobSize) + + btx, isBlob, err := blobtx.UnmarshalBlobTx([]byte(blobTx[0])) + require.NoError(t, err) + require.True(t, isBlob) + + sdkTx, err := ecfg.TxConfig.TxDecoder()(btx.Tx) + require.NoError(t, err) decorator := ante.NewBlobShareDecorator(mockBlobKeeper{}) - ctx := sdk.Context{}.WithIsCheckTx(true).WithBlockHeader(tmproto.Header{Version: version.Consensus{App: tc.appVersion}}) - _, err := decorator.AnteHandle(ctx, tx, false, mockNext) + ctx := sdk.Context{}. + WithIsCheckTx(true). + WithBlockHeader(tmproto.Header{Version: version.Consensus{App: tc.appVersion}}). + WithTxBytes(btx.Tx) + _, err = decorator.AnteHandle(ctx, sdkTx, false, mockNext) assert.ErrorIs(t, tc.wantErr, err) }) } @@ -167,12 +169,3 @@ func TestBlobShareDecorator(t *testing.T) { func mockNext(ctx sdk.Context, _ sdk.Tx, _ bool) (sdk.Context, error) { return ctx, nil } - -// repeat returns a slice of length n with each element set to val. -func repeat(n int, val uint32) []uint32 { - result := make([]uint32, n) - for i := range result { - result[i] = val - } - return result -}