fix!: implement full antehandler checks (#1984)

app/prepare_proposal.go

@@ -4,6 +4,7 @@ import (
 	"github.com/celestiaorg/celestia-app/pkg/da"
 	"github.com/celestiaorg/celestia-app/pkg/shares"
 	"github.com/celestiaorg/celestia-app/pkg/square"
+	"github.com/cosmos/cosmos-sdk/x/auth/ante"
 	abci "github.com/tendermint/tendermint/abci/types"
 	core "github.com/tendermint/tendermint/proto/tendermint/types"
 )
@@ -18,8 +19,20 @@ func (app *App) PrepareProposal(req abci.RequestPrepareProposal) abci.ResponsePr
 	// create a context using a branch of the state and loaded using the
 	// proposal height and chain-id
 	sdkCtx := app.NewProposalContext(core.Header{ChainID: app.GetChainID(), Height: app.LastBlockHeight() + 1})
-	// verify the signatures of the PFBs in the block data. Only the valid PFBs are returned
-	txs := filterForValidPFBSignature(sdkCtx, &app.AccountKeeper, app.txConfig, req.BlockData.Txs)
+	// filter out invalid transactions.
+	// TODO: we can remove all state independent checks from the ante handler here such as signature verification
+	// and only check the state dependent checks like fees and nonces as all these transactions have already
+	// passed CheckTx.
+	handler := NewAnteHandler(
+		app.AccountKeeper,
+		app.BankKeeper,
+		app.BlobKeeper,
+		app.FeeGrantKeeper,
+		app.GetTxConfig().SignModeHandler(),
+		ante.DefaultSigVerificationGasConsumer,
+		app.IBCKeeper,
+	)
+	txs := filterTxs(sdkCtx, handler, app.txConfig, req.BlockData.Txs)
 
 	// build the square from the set of valid and prioritised transactions.
 	// The txs returned are the ones used in the square and block

app/process_proposal.go

@@ -9,6 +9,7 @@ import (
 	"github.com/celestiaorg/celestia-app/pkg/square"
 	blobtypes "github.com/celestiaorg/celestia-app/x/blob/types"
 	sdk "github.com/cosmos/cosmos-sdk/types"
+	"github.com/cosmos/cosmos-sdk/x/auth/ante"
 	abci "github.com/tendermint/tendermint/abci/types"
 	"github.com/tendermint/tendermint/libs/log"
 	tmproto "github.com/tendermint/tendermint/proto/tendermint/types"
@@ -27,12 +28,19 @@ func (app *App) ProcessProposal(req abci.RequestProcessProposal) (resp abci.Resp
 		}
 	}()
 
-	// 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)
+	// Create the anteHander that are used to check the validity of
+	// transactions. All transactions need to be equally validated here
+	// so that the nonce number is always correctly incremented (which
+	// may affect the validity of future transactions).
+	handler := NewAnteHandler(
+		app.AccountKeeper,
+		app.BankKeeper,
+		app.BlobKeeper,
+		app.FeeGrantKeeper,
+		app.GetTxConfig().SignModeHandler(),
+		ante.DefaultSigVerificationGasConsumer,
+		app.IBCKeeper,
+	)
 	sdkCtx := app.NewProposalContext(req.Header).WithChainID(app.GetChainID())
 
 	// iterate over all txs and ensure that all blobTxs are valid, PFBs are correctly signed and non
@@ -64,7 +72,7 @@ func (app *App) ProcessProposal(req abci.RequestProcessProposal) (resp abci.Resp
 			// 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)
+			sdkCtx, err = handler(sdkCtx, sdkTx, false)
 			if err != nil {
 				logInvalidPropBlockError(app.Logger(), req.Header, "failure to incrememnt sequence", err)
 				return reject()
@@ -87,7 +95,7 @@ func (app *App) ProcessProposal(req abci.RequestProcessProposal) (resp abci.Resp
 		}
 
 		// validated the PFB signature
-		sdkCtx, err = svHander(sdkCtx, sdkTx, false)
+		sdkCtx, err = handler(sdkCtx, sdkTx, false)
 		if err != nil {
 			logInvalidPropBlockError(app.Logger(), req.Header, "invalid PFB signature", err)
 			return reject()

app/test/integration_test.go

@@ -47,7 +47,7 @@ type IntegrationTestSuite struct {
 func (s *IntegrationTestSuite) SetupSuite() {
 	t := s.T()
 
-	numAccounts := 120
+	numAccounts := 142
 	s.accounts = make([]string, numAccounts)
 	for i := 0; i < numAccounts; i++ {
 		s.accounts[i] = tmrand.Str(20)
@@ -256,7 +256,7 @@ func (s *IntegrationTestSuite) TestSubmitPayForBlob() {
 			// 20) so we wait a few blocks for the txs to clear
 			require.NoError(t, s.cctx.WaitForBlocks(3))
 
-			signer := blobtypes.NewKeyringSigner(s.cctx.Keyring, s.accounts[0], s.cctx.ChainID)
+			signer := blobtypes.NewKeyringSigner(s.cctx.Keyring, s.accounts[141], s.cctx.ChainID)
 			res, err := blob.SubmitPayForBlob(context.TODO(), signer, s.cctx.GRPCClient, []*blobtypes.Blob{tc.blob, tc.blob}, tc.opts...)
 			require.NoError(t, err)
 			require.NotNil(t, res)
@@ -276,7 +276,7 @@ func (s *IntegrationTestSuite) TestUnwrappedPFBRejection() {
 		1,
 		false,
 		s.cctx.ChainID,
-		s.accounts[:1],
+		s.accounts[140:],
 	)
 
 	btx, isBlob := coretypes.UnmarshalBlobTx(blobTx[0])
@@ -299,7 +299,7 @@ func (s *IntegrationTestSuite) TestShareInclusionProof() {
 		1,
 		true,
 		s.cctx.ChainID,
-		s.accounts[:20],
+		s.accounts[120:140],
 	)
 
 	hashes := make([]string, len(txs))
@@ -311,7 +311,7 @@ func (s *IntegrationTestSuite) TestShareInclusionProof() {
 		hashes[i] = res.TxHash
 	}
 
-	require.NoError(t, s.cctx.WaitForBlocks(10))
+	require.NoError(t, s.cctx.WaitForBlocks(5))
 
 	for _, hash := range hashes {
 		txResp, err := testnode.QueryTx(s.cctx.Context, hash, true)

app/test/prepare_proposal_test.go

@@ -5,7 +5,6 @@ import (
 
 	"github.com/cosmos/cosmos-sdk/crypto/hd"
 	"github.com/cosmos/cosmos-sdk/crypto/keyring"
-	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	abci "github.com/tendermint/tendermint/abci/types"
 	tmproto "github.com/tendermint/tendermint/proto/tendermint/types"
@@ -51,7 +50,7 @@ func TestPrepareProposalPutsPFBsAtEnd(t *testing.T) {
 		1000,
 		accnts[0],
 		accnts[numBlobTxs:],
-		"",
+		testutil.ChainID,
 	)
 	txs := append(blobTxs, coretypes.Txs(normalTxs).ToSliceOfBytes()...)
 
@@ -103,7 +102,7 @@ func TestPrepareProposalFiltering(t *testing.T) {
 		1000,
 		accounts[0],
 		accounts[len(accounts)-3:],
-		"",
+		testutil.ChainID,
 	)).ToSliceOfBytes()
 
 	validTxs := func() [][]byte {
@@ -123,7 +122,7 @@ func TestPrepareProposalFiltering(t *testing.T) {
 		1000,
 		accounts[0],
 		accounts[:3],
-		"",
+		testutil.ChainID,
 	)).ToSliceOfBytes()
 
 	// create a transaction with an account that doesn't exist. This will cause the increment nonce
@@ -187,7 +186,7 @@ func TestPrepareProposalFiltering(t *testing.T) {
 			require.Equal(t, len(tt.txs())-len(tt.prunedTxs), len(resp.BlockData.Txs))
 			// check the the expected txs were removed
 			for _, ptx := range tt.prunedTxs {
-				assert.NotContains(t, resp.BlockData.Txs, ptx)
+				require.NotContains(t, resp.BlockData.Txs, ptx)
 			}
 		})
 	}

app/test/process_proposal_test.go

@@ -49,7 +49,7 @@ func TestProcessProposal(t *testing.T) {
 	// create 3 MsgSend transactions that are signed with valid account numbers
 	// and sequences
 	sendTxs := testutil.SendTxsWithAccounts(
-		t, testApp, enc, kr, 1000, accounts[0], accounts[len(accounts)-3:], "",
+		t, testApp, enc, kr, 1000, accounts[0], accounts[len(accounts)-3:], testutil.ChainID,
 	)
 
 	// block with all blobs included
@@ -65,11 +65,11 @@ func TestProcessProposal(t *testing.T) {
 	// create an invalid block by adding an otherwise valid PFB, but an invalid
 	// signature since there's no account
 	badSigBlobTx := testutil.RandBlobTxsWithManualSequence(
-		t, enc, kr, 1000, 1, false, "", accounts[:1], 1, 1, true,
+		t, enc, kr, 1000, 1, false, testutil.ChainID, accounts[:1], 1, 1, true,
 	)[0]
 
 	blobTxWithInvalidNonce := testutil.RandBlobTxsWithManualSequence(
-		t, enc, kr, 1000, 1, false, "", accounts[:1], 1, 3, false,
+		t, enc, kr, 1000, 1, false, testutil.ChainID, accounts[:1], 1, 3, false,
 	)[0]
 
 	ns1 := appns.MustNewV0(bytes.Repeat([]byte{1}, appns.NamespaceVersionZeroIDSize))

app/validate_txs.go

@@ -3,9 +3,6 @@ package app
 import (
 	"github.com/cosmos/cosmos-sdk/client"
 	sdk "github.com/cosmos/cosmos-sdk/types"
-	"github.com/cosmos/cosmos-sdk/x/auth/ante"
-	"github.com/cosmos/cosmos-sdk/x/auth/keeper"
-	authkeeper "github.com/cosmos/cosmos-sdk/x/auth/keeper"
 	core "github.com/tendermint/tendermint/proto/tendermint/types"
 	tmproto "github.com/tendermint/tendermint/proto/tendermint/types"
 	coretypes "github.com/tendermint/tendermint/types"
@@ -26,23 +23,11 @@ func separateTxs(_ client.TxConfig, rawTxs [][]byte) ([][]byte, []core.BlobTx) {
 	return normalTxs, blobTxs
 }
 
-// filterForValidPFBSignature verifies the signatures of the provided PFB transactions. If it is invalid, it
-// drops the transaction.
-func filterForValidPFBSignature(ctx sdk.Context, accountKeeper *keeper.AccountKeeper, txConfig client.TxConfig, txs [][]byte) [][]byte {
+// filterTxs applies the antehandler to all proposed transactions and removes transactions that return an error.
+func filterTxs(ctx sdk.Context, handler sdk.AnteHandler, txConfig client.TxConfig, txs [][]byte) [][]byte {
 	normalTxs, blobTxs := separateTxs(txConfig, txs)
-
-	// increment the sequences of the standard cosmos-sdk transactions. Panics
-	// from the anteHandler are caught and logged.
-	seqHandler := incrementSequenceAnteHandler(accountKeeper)
-
-	normalTxs, ctx = filterStdTxs(txConfig.TxDecoder(), ctx, 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(accountKeeper, txConfig)
-	blobTxs, _ = filterBlobTxs(txConfig.TxDecoder(), ctx, svHandler, blobTxs)
-
+	normalTxs, ctx = filterStdTxs(txConfig.TxDecoder(), ctx, handler, normalTxs)
+	blobTxs, _ = filterBlobTxs(txConfig.TxDecoder(), ctx, handler, blobTxs)
 	return append(normalTxs, encodeBlobTxs(blobTxs)...)
 }
 
@@ -110,22 +95,3 @@ func encodeBlobTxs(blobTxs []tmproto.BlobTx) [][]byte {
 	}
 	return txs
 }
-
-// sigVerifyAnteHandler creates an AnteHandler with the SetupContext, SetPubKey,
-// SigVerification, and IncremementSequence ante decorators to check that
-// sequences have be incremented.
-func sigVerifyAnteHandler(accKeeper *authkeeper.AccountKeeper, txConfig client.TxConfig) sdk.AnteHandler {
-	setupd := ante.NewSetUpContextDecorator()
-	setPubKd := ante.NewSetPubKeyDecorator(accKeeper)
-	svd := ante.NewSigVerificationDecorator(accKeeper, txConfig.SignModeHandler())
-	isd := ante.NewIncrementSequenceDecorator(accKeeper)
-	return sdk.ChainAnteDecorators(setupd, setPubKd, svd, isd)
-}
-
-// incrementSequenceAnteHandler creates an AnteHandler that only incrememts the
-// sequence.
-func incrementSequenceAnteHandler(accKeeper *authkeeper.AccountKeeper) sdk.AnteHandler {
-	setupd := ante.NewSetUpContextDecorator()
-	isd := ante.NewIncrementSequenceDecorator(accKeeper)
-	return sdk.ChainAnteDecorators(setupd, isd)
-}

test/util/testnode/node_interaction_api.go

@@ -22,7 +22,7 @@ import (
 )
 
 const (
-	DefaultTimeout = 10 * time.Second
+	DefaultTimeout = 30 * time.Second
 )
 
 type Context struct {