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

chore: optimize checkTx #3954

Merged
merged 6 commits into from
Oct 10, 2024
Merged
Show file tree
Hide file tree
Changes from 5 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/process_proposal.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ func (app *App) ProcessProposal(req abci.RequestProcessProposal) (resp abci.Resp
tx = blobTx.Tx
}

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 @@ -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
Comment on lines +140 to +141
Copy link
Collaborator

Choose a reason for hiding this comment

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

[optional]

Suggested change
// create a tx that can't be included in a 64 x 64 when accounting for the
// pfb along with the shares
// create a tx that can't be included in a 64 x 64 when accounting for the
// PFB shares along with the blob shares.

tooManyShareBtx := blobfactory.ManyMultiBlobTx(
Copy link
Collaborator

Choose a reason for hiding this comment

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

[optional] rename for clarity

Suggested change
tooManyShareBtx := blobfactory.ManyMultiBlobTx(
tooManyShareBlobTx := 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)},
),
evan-forbes marked this conversation as resolved.
Show resolved Hide resolved
)[0]

type test struct {
name string
txs func() [][]byte
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}
28 changes: 28 additions & 0 deletions app/test/process_proposal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Copy link
Member Author

Choose a reason for hiding this comment

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

this reject isn't new, but it should still be covered by tests

},
}

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 @@ -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)
Copy link
Member Author

Choose a reason for hiding this comment

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

this needs to be added for future things anyway, although it has no impact atm

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 @@ -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)
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)
}
Comment on lines +39 to 41
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

Potential integer overflow when casting transaction size to uint32

Casting len(ctx.TxBytes()) to uint32 may cause an integer overflow if the transaction size exceeds the maximum value of uint32. To prevent potential issues, consider using uint64 instead.

Apply this diff to use uint64:

-	if sharesNeeded := getSharesNeeded(uint32(len(ctx.TxBytes())), pfb.BlobSizes); sharesNeeded > maxBlobShares {
+	if sharesNeeded := getSharesNeeded(uint64(len(ctx.TxBytes())), pfb.BlobSizes); sharesNeeded > maxBlobShares {

Also, update the getSharesNeeded function signature:

-func getSharesNeeded(base uint32, blobSizes []uint32) (sum int) {
+func getSharesNeeded(base uint64, blobSizes []uint32) (sum int) {

Ensure that any functions called within getSharesNeeded, such as share.CompactSharesNeeded, accept uint64 parameters.

Committable suggestion was skipped due to low confidence.

}
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 += share.SparseSharesNeeded(blobSize)
}
Expand Down
Loading
Loading