Skip to content

Commit

Permalink
chore: optimize checkTx (#3954)
Browse files Browse the repository at this point in the history
## Overview

this PR makes a simple optimization for checkTx

---------

Co-authored-by: Rootul P <[email protected]>
Co-authored-by: CHAMI Rachid <[email protected]>
(cherry picked from commit ca222a8)

# Conflicts:
#	app/test/process_proposal_test.go
#	x/blob/ante/blob_share_decorator_test.go
  • Loading branch information
evan-forbes authored and mergify[bot] committed Oct 10, 2024
1 parent b717c8d commit 73f3de0
Show file tree
Hide file tree
Showing 6 changed files with 242 additions and 73 deletions.
3 changes: 3 additions & 0 deletions app/process_proposal.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,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 {
Expand Down
32 changes: 32 additions & 0 deletions app/test/prepare_proposal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,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
Expand Down Expand Up @@ -182,6 +198,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 {
Expand Down Expand Up @@ -217,3 +240,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
}
86 changes: 86 additions & 0 deletions app/test/process_proposal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"github.com/tendermint/tendermint/proto/tendermint/version"
coretypes "github.com/tendermint/tendermint/types"

<<<<<<< HEAD

Check failure on line 17 in app/test/process_proposal_test.go

View workflow job for this annotation

GitHub Actions / lint / golangci-lint

missing import path (typecheck)

Check failure on line 17 in app/test/process_proposal_test.go

View workflow job for this annotation

GitHub Actions / test / test-short

missing import path

Check failure on line 17 in app/test/process_proposal_test.go

View workflow job for this annotation

GitHub Actions / test / test

missing import path

Check failure on line 17 in app/test/process_proposal_test.go

View workflow job for this annotation

GitHub Actions / test / test-coverage

missing import path

Check failure on line 17 in app/test/process_proposal_test.go

View workflow job for this annotation

GitHub Actions / test / test-race

missing import path
"github.com/celestiaorg/celestia-app/v2/app"
"github.com/celestiaorg/celestia-app/v2/app/encoding"
"github.com/celestiaorg/celestia-app/v2/pkg/appconsts"
Expand All @@ -28,6 +29,24 @@ import (
appns "github.com/celestiaorg/go-square/namespace"
"github.com/celestiaorg/go-square/shares"
"github.com/celestiaorg/go-square/square"
=======

Check failure on line 32 in app/test/process_proposal_test.go

View workflow job for this annotation

GitHub Actions / lint / golangci-lint

missing import path (typecheck)
"github.com/celestiaorg/celestia-app/v3/app"
"github.com/celestiaorg/celestia-app/v3/app/encoding"
"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"
"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"
blobtypes "github.com/celestiaorg/celestia-app/v3/x/blob/types"
"github.com/celestiaorg/go-square/v2"
"github.com/celestiaorg/go-square/v2/share"
"github.com/celestiaorg/go-square/v2/tx"
>>>>>>> ca222a86 (chore: optimize checkTx (#3954))
)

func TestProcessProposal(t *testing.T) {
Expand Down Expand Up @@ -81,6 +100,20 @@ func TestProcessProposal(t *testing.T) {
assert.Error(t, err)
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
Expand Down Expand Up @@ -330,6 +363,59 @@ func TestProcessProposal(t *testing.T) {
appVersion: appconsts.LatestVersion,
expectedResult: abci.ResponseProcessProposal_REJECT,
},
<<<<<<< HEAD
=======
{
name: "valid v1 authored blob",
input: validData(),
mutator: func(d *tmproto.Data) {
addr := signer.Account(accounts[0]).Address()
blob, err := share.NewV1Blob(ns1, data, addr)
require.NoError(t, err)
rawTx, _, err := signer.CreatePayForBlobs(accounts[0], []*share.Blob{blob}, user.SetGasLimit(100000), user.SetFee(100000))
require.NoError(t, err)
d.Txs[0] = rawTx
d.Hash = calculateNewDataHash(t, d.Txs)
},
appVersion: appconsts.LatestVersion,
expectedResult: abci.ResponseProcessProposal_ACCEPT,
},
{
name: "v1 authored blob with invalid signer",
input: validData(),
mutator: func(d *tmproto.Data) {
addr := signer.Account(accounts[0]).Address()
falseAddr := testnode.RandomAddress().(sdk.AccAddress)
blob, err := share.NewV1Blob(ns1, data, falseAddr)
require.NoError(t, err)
msg, err := blobtypes.NewMsgPayForBlobs(addr.String(), appconsts.LatestVersion, blob)
require.NoError(t, err)

rawTx, err := signer.CreateTx([]sdk.Msg{msg}, user.SetGasLimit(100000), user.SetFee(100000))
require.NoError(t, err)

blobTxBytes, err := tx.MarshalBlobTx(rawTx, blob)
require.NoError(t, err)
d.Txs[0] = blobTxBytes
d.Hash = calculateNewDataHash(t, d.Txs)
},
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,
},
>>>>>>> ca222a86 (chore: optimize checkTx (#3954))

Check failure on line 418 in app/test/process_proposal_test.go

View workflow job for this annotation

GitHub Actions / lint / golangci-lint

illegal character U+0023 '#' (typecheck)
}

for _, tt := range tests {
Expand Down
2 changes: 2 additions & 0 deletions app/validate_txs.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,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)
Expand Down Expand Up @@ -75,6 +76,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 []*blob.BlobTx) ([]*blob.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)
Expand Down
13 changes: 6 additions & 7 deletions x/blob/ante/blob_share_decorator.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
Expand All @@ -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
Expand All @@ -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 += shares.SparseSharesNeeded(blobSize)
}
Expand Down
Loading

0 comments on commit 73f3de0

Please sign in to comment.