feat!: Add a block validity rule that all PFB transactions must have valid signatures

app/estimate_square_size_test.go

@@ -97,6 +97,7 @@ func Test_estimateSquareSize_MultiBlob(t *testing.T) {
 				enc.TxConfig.TxEncoder(),
 				signer,
 				tt.getBlobSizes(),
+				0, 0,
 			)
 			normalTxs, blobTxs := separateTxs(enc.TxConfig, shares.TxsToBytes(txs))
 			resSquareSize, resStart := estimateSquareSize(normalTxs, blobTxs)

app/parse_txs.go

@@ -2,7 +2,10 @@ package app
 
 import (
 	"github.com/cosmos/cosmos-sdk/client"
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	"github.com/tendermint/tendermint/libs/log"
 	core "github.com/tendermint/tendermint/proto/tendermint/types"
+	tmproto "github.com/tendermint/tendermint/proto/tendermint/types"
 	coretypes "github.com/tendermint/tendermint/types"
 )
 
@@ -20,3 +23,66 @@ func separateTxs(txcfg client.TxConfig, rawTxs [][]byte) ([][]byte, []core.BlobT
 	}
 	return normalTxs, blobTxs
 }
+
+// filterStdTxs applies the provided antehandler to each transaction and removes
+// transactions that return an error. Panics are caught by the checkTxValidity
+// function used to apply the ante handler.
+func filterStdTxs(logger log.Logger, dec sdk.TxDecoder, ctx sdk.Context, handler sdk.AnteHandler, txs [][]byte) ([][]byte, sdk.Context) {
+	n := 0
+	var err error
+	for _, tx := range txs {
+		ctx, err = checkTxValidity(logger, dec, ctx, handler, tx)
+		// either the transaction is invalid (ie incorrect nonce) and we
+		// simply want to remove this tx, or we're catching a panic from one
+		// of the anteHanders which is logged.
+		if err != nil {
+			continue
+		}
+		txs[n] = tx
+		n++
+
+	}
+
+	return txs[:n], ctx
+}
+
+// filterBlobTxs applies the provided antehandler to each transaction
+// and removes transactions that return an error. Panics are caught by the checkTxValidity
+// function used to apply the ante handler.
+func filterBlobTxs(logger log.Logger, dec sdk.TxDecoder, ctx sdk.Context, handler sdk.AnteHandler, txs []tmproto.BlobTx) ([]tmproto.BlobTx, sdk.Context) {
+	n := 0
+	var err error
+	for _, tx := range txs {
+		ctx, err = checkTxValidity(logger, dec, ctx, handler, tx.Tx)
+		// either the transaction is invalid (ie incorrect nonce) and we
+		// simply want to remove this tx, or we're catching a panic from one
+		// of the anteHanders which is logged.
+		if err != nil {
+			continue
+		}
+		txs[n] = tx
+		n++
+
+	}
+
+	return txs[:n], ctx
+}
+
+func checkTxValidity(logger log.Logger, dec sdk.TxDecoder, ctx sdk.Context, handler sdk.AnteHandler, tx []byte) (sdk.Context, error) {
+	// catch panics from anteHandlers
+	defer func() {
+		if r := recover(); r != nil {
+			err := recoverHandler(r)
+			if err != nil {
+				logger.Error(err.Error())
+			}
+		}
+	}()
+
+	sdkTx, err := dec(tx)
+	if err != nil {
+		return ctx, err
+	}
+
+	return handler(ctx, sdkTx, false)
+}

app/prepare_proposal.go

@@ -25,6 +25,22 @@ func (app *App) PrepareProposal(req abci.RequestPrepareProposal) abci.ResponsePr
 	// the txs is maintained.
 	normalTxs, blobTxs := separateTxs(app.txConfig, req.BlockData.Txs)
 
+	sdkCtx, err := app.NewProcessProposalQueryContext()
+	if err != nil {
+		panic(err)
+	}
+
+	// increment the sequences of the standard cosmos-sdk transactions. Panics
+	// from the anteHandler are caught and logged.
+	seqHandler := incrementSequenceAnteHandler(&app.AccountKeeper)
+	normalTxs, sdkCtx = filterStdTxs(app.Logger(), app.txConfig.TxDecoder(), sdkCtx, seqHandler, normalTxs)
+
+	// check the signatures and increment the sequences of the blob txs,
+	// and filter out any that fail. Panics from the anteHandler are caught and
+	// logged.
+	svHandler := sigVerifyAnteHandler(&app.AccountKeeper, app.txConfig)
+	blobTxs, _ = filterBlobTxs(app.Logger(), app.txConfig.TxDecoder(), sdkCtx, svHandler, blobTxs)
+
 	// estimate the square size. This estimation errs on the side of larger
 	// squares but can only return values within the min and max square size.
 	squareSize, nonreservedStart := estimateSquareSize(normalTxs, blobTxs)

app/process_proposal.go

@@ -86,7 +86,22 @@ func (app *App) ProcessProposal(req abci.RequestProcessProposal) abci.ResponsePr
 		}
 	}
 
-	// iterate over all of the MsgPayForBlobs transactions and ensure that their
+	// create the anteHanders that are used to check the validity of
+	// transactions. We verify the signatures of PFB containing txs using the
+	// sigVerifyAnterHandler, and simply increase the nonce of all other
+	// transactions.
+	svHander := sigVerifyAnteHandler(&app.AccountKeeper, app.txConfig)
+	seqHandler := incrementSequenceAnteHandler(&app.AccountKeeper)
+
+	sdkCtx, err := app.NewProcessProposalQueryContext()
+	if err != nil {
+		logInvalidPropBlockError(app.Logger(), req.Header, "failure to load query context", err)
+		return abci.ResponseProcessProposal{
+			Result: abci.ResponseProcessProposal_REJECT,
+		}
+	}
+
+	// iterate over all of the MsgPayForBlob transactions and ensure that their
 	// commitments are subtree roots of the data root.
 	for _, rawTx := range req.BlockData.Txs {
 		tx := rawTx
@@ -105,6 +120,17 @@ func (app *App) ProcessProposal(req abci.RequestProcessProposal) abci.ResponsePr
 
 		pfb, has := hasPFB(sdkTx.GetMsgs())
 		if !has {
+			// we need to increment the sequence for every transaction so that
+			// the signature check below is accurate. this error only gets hit
+			// if the account in question doens't exist.
+			sdkCtx, err = seqHandler(sdkCtx, sdkTx, false)
+			if err != nil {
+				logInvalidPropBlockError(app.Logger(), req.Header, "failure to incrememnt sequence", err)
+				return abci.ResponseProcessProposal{
+					Result: abci.ResponseProcessProposal_REJECT,
+				}
+			}
+
 			// we do not need to perform further checks on this transaction,
 			// since it has no PFB
 			continue
@@ -149,6 +175,14 @@ func (app *App) ProcessProposal(req abci.RequestProcessProposal) abci.ResponsePr
 				}
 			}
 		}
+
+		sdkCtx, err = svHander(sdkCtx, sdkTx, true)
+		if err != nil {
+			logInvalidPropBlockError(app.Logger(), req.Header, "invalid PFB signature", err)
+			return abci.ResponseProcessProposal{
+				Result: abci.ResponseProcessProposal_REJECT,
+			}
+		}
 	}
 
 	return abci.ResponseProcessProposal{

app/test/fuzz_abci_test.go

@@ -7,9 +7,9 @@ import (
 	"github.com/celestiaorg/celestia-app/app"
 	"github.com/celestiaorg/celestia-app/app/encoding"
 	"github.com/celestiaorg/celestia-app/testutil"
-	"github.com/celestiaorg/celestia-app/testutil/blobfactory"
 	"github.com/stretchr/testify/require"
 	abci "github.com/tendermint/tendermint/abci/types"
+	tmrand "github.com/tendermint/tendermint/libs/rand"
 	core "github.com/tendermint/tendermint/proto/tendermint/types"
 	coretypes "github.com/tendermint/tendermint/types"
 )
@@ -25,54 +25,68 @@ func TestPrepareProposalConsistency(t *testing.T) {
 		t.Skip("skipping TestPrepareProposalConsistency in short mode.")
 	}
 	encConf := encoding.MakeConfig(app.ModuleEncodingRegisters...)
-	testApp, _ := testutil.SetupTestAppWithGenesisValSet()
+	accounts := make([]string, 1100) // 1000 for creating blob txs, 100 for creating send txs
+	for i := range accounts {
+		accounts[i] = tmrand.Str(20)
+	}
+
+	testApp, kr := testutil.SetupTestAppWithGenesisValSet(accounts...)
 
 	type test struct {
 		name                   string
 		count, blobCount, size int
 	}
 	tests := []test{
-		{"many small single share single blob transactions", 10000, 1, 400},
+		{"many small single share single blob transactions", 1000, 1, 400},
 		{"one hundred normal sized single blob transactions", 100, 1, 400000},
 		{"many single share multi-blob transactions", 1000, 100, 400},
 		{"one hundred normal sized multi-blob transactions", 100, 4, 400000},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			timer := time.After(time.Second * 30)
+			timer := time.After(time.Second * 20)
 			for {
 				select {
 				case <-timer:
 					return
 				default:
-					ProcessRandomProposal(t, tt.count, tt.size, tt.blobCount, encConf, testApp)
+					txs := testutil.RandBlobTxsWithAccounts(
+						t,
+						testApp,
+						encConf.TxConfig.TxEncoder(),
+						kr,
+						tt.size,
+						tt.count,
+						true,
+						"",
+						accounts[:tt.count],
+					)
+					// create 100 send transactions
+					sendTxs := testutil.SendTxsWithAccounts(
+						t,
+						testApp,
+						encConf.TxConfig.TxEncoder(),
+						kr,
+						1000,
+						accounts[0],
+						accounts[len(accounts)-100:],
+						"",
+					)
+					txs = append(txs, sendTxs...)
+					resp := testApp.PrepareProposal(abci.RequestPrepareProposal{
+						BlockData: &core.Data{
+							Txs: coretypes.Txs(txs).ToSliceOfBytes(),
+						},
+					})
+					res := testApp.ProcessProposal(abci.RequestProcessProposal{
+						BlockData: resp.BlockData,
+						Header: core.Header{
+							DataHash: resp.BlockData.Hash,
+						},
+					})
+					require.Equal(t, abci.ResponseProcessProposal_ACCEPT, res.Result)
 				}
 			}
 		})
 	}
 }
-
-func ProcessRandomProposal(
-	t *testing.T,
-	count,
-	maxSize int,
-	maxBlobCount int,
-	cfg encoding.Config,
-	capp *app.App,
-) {
-	txs := blobfactory.RandBlobTxsRandomlySized(cfg.TxConfig.TxEncoder(), count, maxSize, maxBlobCount)
-	sendTxs := blobfactory.GenerateManyRawSendTxs(cfg.TxConfig, count)
-	txs = append(txs, sendTxs...)
-	resp := capp.PrepareProposal(abci.RequestPrepareProposal{
-		BlockData: &core.Data{
-			Txs: coretypes.Txs(txs).ToSliceOfBytes(),
-		},
-	})
-	res := capp.ProcessProposal(abci.RequestProcessProposal{
-		BlockData: resp.BlockData,
-		Header: core.Header{
-			DataHash: resp.BlockData.Hash,
-		},
-	})
-	require.Equal(t, abci.ResponseProcessProposal_ACCEPT, res.Result)
-}

app/test/integration_test.go

@@ -60,7 +60,7 @@ func (s *IntegrationTestSuite) SetupSuite() {
 	}
 	s.T().Log("setting up integration test suite")
 
-	numAccounts := 100
+	numAccounts := 120
 	s.accounts = make([]string, numAccounts)
 	for i := 0; i < numAccounts; i++ {
 		s.accounts[i] = tmrand.Str(20)
@@ -115,7 +115,7 @@ func (s *IntegrationTestSuite) TestMaxBlockSize() {
 			3,
 			false,
 			s.cfg.ChainID,
-			s.accounts[:20],
+			s.accounts[20:40],
 		)
 	}
 
@@ -132,7 +132,7 @@ func (s *IntegrationTestSuite) TestMaxBlockSize() {
 			8,
 			true,
 			s.cfg.ChainID,
-			s.accounts[:80],
+			s.accounts[40:120],
 		)
 	}