From 36ca8b9307c9c8454c67e48f9ed2d06249ff928e Mon Sep 17 00:00:00 2001 From: Marko Date: Thu, 2 May 2024 16:49:14 +0200 Subject: [PATCH 1/6] fix: remove txs from mempool when antehandler fails in recheck (#20144) Co-authored-by: Facundo (cherry picked from commit 599ae55c41e510b0e2605914826638323df1c8bb) # Conflicts: # baseapp/baseapp.go --- baseapp/abci_test.go | 101 +++++++++++++++++++++++++++++++++++++++++++ baseapp/baseapp.go | 10 +++++ 2 files changed, 111 insertions(+) diff --git a/baseapp/abci_test.go b/baseapp/abci_test.go index c3d606db018a..add3de9e453b 100644 --- a/baseapp/abci_test.go +++ b/baseapp/abci_test.go @@ -1693,3 +1693,104 @@ func TestABCI_Proposal_Reset_State_Between_Calls(t *testing.T) { Header: tmproto.Header{Height: suite.baseApp.LastBlockHeight() + 1}, }) } + +func TestABCI_Proposal_FailReCheckTx(t *testing.T) { + pool := mempool.NewPriorityMempool[int64](mempool.PriorityNonceMempoolConfig[int64]{ + TxPriority: mempool.NewDefaultTxPriority(), + MaxTx: 0, + SignerExtractor: mempool.NewDefaultSignerExtractionAdapter(), + }) + + anteOpt := func(bapp *baseapp.BaseApp) { + bapp.SetAnteHandler(func(ctx sdk.Context, tx sdk.Tx, simulate bool) (sdk.Context, error) { + // always fail on recheck, just to test the recheck logic + if ctx.IsReCheckTx() { + return ctx, errors.New("recheck failed in ante handler") + } + + return ctx, nil + }) + } + + suite := NewBaseAppSuite(t, anteOpt, baseapp.SetMempool(pool)) + baseapptestutil.RegisterKeyValueServer(suite.baseApp.MsgServiceRouter(), MsgKeyValueImpl{}) + baseapptestutil.RegisterCounterServer(suite.baseApp.MsgServiceRouter(), NoopCounterServerImpl{}) + + _, err := suite.baseApp.InitChain(&abci.RequestInitChain{ + ConsensusParams: &cmtproto.ConsensusParams{}, + }) + require.NoError(t, err) + + tx := newTxCounter(t, suite.txConfig, 0, 1) + txBytes, err := suite.txConfig.TxEncoder()(tx) + require.NoError(t, err) + + reqCheckTx := abci.RequestCheckTx{ + Tx: txBytes, + Type: abci.CheckTxType_New, + } + _, err = suite.baseApp.CheckTx(&reqCheckTx) + require.NoError(t, err) + + tx2 := newTxCounter(t, suite.txConfig, 1, 1) + + tx2Bytes, err := suite.txConfig.TxEncoder()(tx2) + require.NoError(t, err) + + err = pool.Insert(sdk.Context{}, tx2) + require.NoError(t, err) + + require.Equal(t, 2, pool.CountTx()) + + // call prepareProposal before calling recheck tx, just as a sanity check + reqPrepareProposal := abci.RequestPrepareProposal{ + MaxTxBytes: 1000, + Height: 1, + } + resPrepareProposal, err := suite.baseApp.PrepareProposal(&reqPrepareProposal) + require.NoError(t, err) + require.Equal(t, 2, len(resPrepareProposal.Txs)) + + // call recheck on the first tx, it MUST return an error + reqReCheckTx := abci.RequestCheckTx{ + Tx: txBytes, + Type: abci.CheckTxType_Recheck, + } + resp, err := suite.baseApp.CheckTx(&reqReCheckTx) + require.NoError(t, err) + require.True(t, resp.IsErr()) + require.Equal(t, "recheck failed in ante handler", resp.Log) + + // call prepareProposal again, should return only the second tx + resPrepareProposal, err = suite.baseApp.PrepareProposal(&reqPrepareProposal) + require.NoError(t, err) + require.Equal(t, 1, len(resPrepareProposal.Txs)) + require.Equal(t, tx2Bytes, resPrepareProposal.Txs[0]) + + // check the mempool, it should have only the second tx + require.Equal(t, 1, pool.CountTx()) + + reqProposalTxBytes := [][]byte{ + tx2Bytes, + } + reqProcessProposal := abci.RequestProcessProposal{ + Txs: reqProposalTxBytes, + Height: reqPrepareProposal.Height, + } + + resProcessProposal, err := suite.baseApp.ProcessProposal(&reqProcessProposal) + require.NoError(t, err) + require.Equal(t, abci.ResponseProcessProposal_ACCEPT, resProcessProposal.Status) + + // the same txs as in PrepareProposal + res, err := suite.baseApp.FinalizeBlock(&abci.RequestFinalizeBlock{ + Height: suite.baseApp.LastBlockHeight() + 1, + Txs: reqProposalTxBytes, + }) + require.NoError(t, err) + + require.Equal(t, 0, pool.CountTx()) + + require.NotEmpty(t, res.TxResults[0].Events) + require.True(t, res.TxResults[0].IsOK(), fmt.Sprintf("%v", res)) +} diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go index c2ce465ca9ca..465b8b27855e 100644 --- a/baseapp/baseapp.go +++ b/baseapp/baseapp.go @@ -717,7 +717,17 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte) (gInfo sdk.GasInfo, re gasWanted = ctx.GasMeter().Limit() if err != nil { +<<<<<<< HEAD return gInfo, nil, nil, 0, err +======= + if mode == execModeReCheck { + // if the ante handler fails on recheck, we want to remove the tx from the mempool + if mempoolErr := app.mempool.Remove(tx); mempoolErr != nil { + return gInfo, nil, anteEvents, errors.Join(err, mempoolErr) + } + } + return gInfo, nil, nil, err +>>>>>>> 599ae55c4 (fix: remove txs from mempool when antehandler fails in recheck (#20144)) } priority = ctx.Priority() From fcc9255d43564bb7d44f23bb547044912d068646 Mon Sep 17 00:00:00 2001 From: marbar3778 Date: Thu, 2 May 2024 17:28:41 +0200 Subject: [PATCH 2/6] fix conflicts --- baseapp/baseapp.go | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go index 465b8b27855e..52a25f319352 100644 --- a/baseapp/baseapp.go +++ b/baseapp/baseapp.go @@ -717,17 +717,13 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte) (gInfo sdk.GasInfo, re gasWanted = ctx.GasMeter().Limit() if err != nil { -<<<<<<< HEAD - return gInfo, nil, nil, 0, err -======= - if mode == execModeReCheck { + if mode == runTxModeReCheck { // if the ante handler fails on recheck, we want to remove the tx from the mempool if mempoolErr := app.mempool.Remove(tx); mempoolErr != nil { - return gInfo, nil, anteEvents, errors.Join(err, mempoolErr) + return gInfo, nil, anteEvents, 0, errors.Join(err, mempoolErr) } } - return gInfo, nil, nil, err ->>>>>>> 599ae55c4 (fix: remove txs from mempool when antehandler fails in recheck (#20144)) + return gInfo, nil, nil, 0, err } priority = ctx.Priority() From df7deb11a011beb9bd0e79d50a912c5d5a398e26 Mon Sep 17 00:00:00 2001 From: marbar3778 Date: Thu, 2 May 2024 17:36:21 +0200 Subject: [PATCH 3/6] fix tests --- baseapp/abci_test.go | 42 ++++++++++++++++-------------------------- 1 file changed, 16 insertions(+), 26 deletions(-) diff --git a/baseapp/abci_test.go b/baseapp/abci_test.go index add3de9e453b..0bc4ddfecdfc 100644 --- a/baseapp/abci_test.go +++ b/baseapp/abci_test.go @@ -10,6 +10,7 @@ import ( dbm "github.com/cometbft/cometbft-db" abci "github.com/cometbft/cometbft/abci/types" + cmtproto "github.com/cometbft/cometbft/proto/tendermint/types" tmproto "github.com/cometbft/cometbft/proto/tendermint/types" "github.com/cosmos/gogoproto/jsonpb" "github.com/stretchr/testify/require" @@ -1695,11 +1696,7 @@ func TestABCI_Proposal_Reset_State_Between_Calls(t *testing.T) { } func TestABCI_Proposal_FailReCheckTx(t *testing.T) { - pool := mempool.NewPriorityMempool[int64](mempool.PriorityNonceMempoolConfig[int64]{ - TxPriority: mempool.NewDefaultTxPriority(), - MaxTx: 0, - SignerExtractor: mempool.NewDefaultSignerExtractionAdapter(), - }) + pool := mempool.NewSenderNonceMempool() anteOpt := func(bapp *baseapp.BaseApp) { bapp.SetAnteHandler(func(ctx sdk.Context, tx sdk.Tx, simulate bool) (sdk.Context, error) { @@ -1716,10 +1713,9 @@ func TestABCI_Proposal_FailReCheckTx(t *testing.T) { baseapptestutil.RegisterKeyValueServer(suite.baseApp.MsgServiceRouter(), MsgKeyValueImpl{}) baseapptestutil.RegisterCounterServer(suite.baseApp.MsgServiceRouter(), NoopCounterServerImpl{}) - _, err := suite.baseApp.InitChain(&abci.RequestInitChain{ + suite.baseApp.InitChain(abci.RequestInitChain{ ConsensusParams: &cmtproto.ConsensusParams{}, }) - require.NoError(t, err) tx := newTxCounter(t, suite.txConfig, 0, 1) txBytes, err := suite.txConfig.TxEncoder()(tx) @@ -1729,8 +1725,7 @@ func TestABCI_Proposal_FailReCheckTx(t *testing.T) { Tx: txBytes, Type: abci.CheckTxType_New, } - _, err = suite.baseApp.CheckTx(&reqCheckTx) - require.NoError(t, err) + _ = suite.baseApp.CheckTx(reqCheckTx) tx2 := newTxCounter(t, suite.txConfig, 1, 1) @@ -1747,8 +1742,7 @@ func TestABCI_Proposal_FailReCheckTx(t *testing.T) { MaxTxBytes: 1000, Height: 1, } - resPrepareProposal, err := suite.baseApp.PrepareProposal(&reqPrepareProposal) - require.NoError(t, err) + resPrepareProposal := suite.baseApp.PrepareProposal(reqPrepareProposal) require.Equal(t, 2, len(resPrepareProposal.Txs)) // call recheck on the first tx, it MUST return an error @@ -1756,41 +1750,37 @@ func TestABCI_Proposal_FailReCheckTx(t *testing.T) { Tx: txBytes, Type: abci.CheckTxType_Recheck, } - resp, err := suite.baseApp.CheckTx(&reqReCheckTx) - require.NoError(t, err) + resp := suite.baseApp.CheckTx(reqReCheckTx) + require.True(t, resp.IsErr()) require.Equal(t, "recheck failed in ante handler", resp.Log) // call prepareProposal again, should return only the second tx - resPrepareProposal, err = suite.baseApp.PrepareProposal(&reqPrepareProposal) - require.NoError(t, err) + resPrepareProposal = suite.baseApp.PrepareProposal(reqPrepareProposal) require.Equal(t, 1, len(resPrepareProposal.Txs)) require.Equal(t, tx2Bytes, resPrepareProposal.Txs[0]) // check the mempool, it should have only the second tx require.Equal(t, 1, pool.CountTx()) - reqProposalTxBytes := [][]byte{ - tx2Bytes, - } + reqProposalTxBytes := tx2Bytes + reqProcessProposal := abci.RequestProcessProposal{ - Txs: reqProposalTxBytes, + Txs: [][]byte{reqProposalTxBytes}, Height: reqPrepareProposal.Height, } - resProcessProposal, err := suite.baseApp.ProcessProposal(&reqProcessProposal) - require.NoError(t, err) + resProcessProposal := suite.baseApp.ProcessProposal(reqProcessProposal) require.Equal(t, abci.ResponseProcessProposal_ACCEPT, resProcessProposal.Status) // the same txs as in PrepareProposal - res, err := suite.baseApp.FinalizeBlock(&abci.RequestFinalizeBlock{ - Height: suite.baseApp.LastBlockHeight() + 1, - Txs: reqProposalTxBytes, + res := suite.baseApp.DeliverTx(abci.RequestDeliverTx{ + Tx: reqProposalTxBytes, }) require.NoError(t, err) require.Equal(t, 0, pool.CountTx()) - require.NotEmpty(t, res.TxResults[0].Events) - require.True(t, res.TxResults[0].IsOK(), fmt.Sprintf("%v", res)) + require.NotEmpty(t, res.Events) + require.True(t, res.IsOK(), fmt.Sprintf("%v", res)) } From 19181dd0c36b88ba9535b399e02690ca7814c10c Mon Sep 17 00:00:00 2001 From: marbar3778 Date: Thu, 2 May 2024 17:53:58 +0200 Subject: [PATCH 4/6] remove errors.join --- baseapp/baseapp.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go index 52a25f319352..30a40db70721 100644 --- a/baseapp/baseapp.go +++ b/baseapp/baseapp.go @@ -720,7 +720,7 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte) (gInfo sdk.GasInfo, re if mode == runTxModeReCheck { // if the ante handler fails on recheck, we want to remove the tx from the mempool if mempoolErr := app.mempool.Remove(tx); mempoolErr != nil { - return gInfo, nil, anteEvents, 0, errors.Join(err, mempoolErr) + return gInfo, nil, anteEvents, 0, fmt.Errorf("error: %w, mempool error: %w", err, mempoolErr) } } return gInfo, nil, nil, 0, err From f69b0fce6a6d5a404bc5fc3027abee5ed9f260c8 Mon Sep 17 00:00:00 2001 From: marbar3778 Date: Thu, 2 May 2024 18:33:33 +0200 Subject: [PATCH 5/6] fix error --- baseapp/baseapp.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go index 30a40db70721..4c94b5e69b26 100644 --- a/baseapp/baseapp.go +++ b/baseapp/baseapp.go @@ -720,7 +720,7 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte) (gInfo sdk.GasInfo, re if mode == runTxModeReCheck { // if the ante handler fails on recheck, we want to remove the tx from the mempool if mempoolErr := app.mempool.Remove(tx); mempoolErr != nil { - return gInfo, nil, anteEvents, 0, fmt.Errorf("error: %w, mempool error: %w", err, mempoolErr) + return gInfo, nil, anteEvents, 0, fmt.Errorf("error: %v, mempool error: %w", err, mempoolErr) } } return gInfo, nil, nil, 0, err From 7db2fef209969185280c8fd5802bcf12c9d6152e Mon Sep 17 00:00:00 2001 From: marbar3778 Date: Thu, 2 May 2024 22:21:54 +0200 Subject: [PATCH 6/6] fix test --- baseapp/abci_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/baseapp/abci_test.go b/baseapp/abci_test.go index 0bc4ddfecdfc..401757dabd5f 100644 --- a/baseapp/abci_test.go +++ b/baseapp/abci_test.go @@ -1773,6 +1773,10 @@ func TestABCI_Proposal_FailReCheckTx(t *testing.T) { resProcessProposal := suite.baseApp.ProcessProposal(reqProcessProposal) require.Equal(t, abci.ResponseProcessProposal_ACCEPT, resProcessProposal.Status) + suite.baseApp.BeginBlock(abci.RequestBeginBlock{ + Header: tmproto.Header{Height: suite.baseApp.LastBlockHeight() + 1}, + }) + // the same txs as in PrepareProposal res := suite.baseApp.DeliverTx(abci.RequestDeliverTx{ Tx: reqProposalTxBytes,