From 605b996a970942f62a0dc2bd37c85b0774e46436 Mon Sep 17 00:00:00 2001 From: marbar3778 Date: Mon, 22 Apr 2024 19:24:30 +0200 Subject: [PATCH 01/11] remove txs from mempool when antehandler fails in recheck --- baseapp/baseapp.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go index 82f47d81b1c5..eb72c4709a78 100644 --- a/baseapp/baseapp.go +++ b/baseapp/baseapp.go @@ -911,6 +911,12 @@ func (app *BaseApp) runTx(mode execMode, txBytes []byte) (gInfo sdk.GasInfo, res gasWanted = ctx.GasMeter().Limit() if err != nil { + if mode == execModeReCheck { + // if the ante handler fails on recheck, we want to remove the tx from the mempool + if err := app.mempool.Remove(tx); err != nil { + return gInfo, nil, anteEvents, errors.Join(err, err) + } + } return gInfo, nil, nil, err } From bd66084ce930680004f568098fa475a5fe3ce194 Mon Sep 17 00:00:00 2001 From: marbar3778 Date: Tue, 23 Apr 2024 08:29:33 +0200 Subject: [PATCH 02/11] random --- baseapp/baseapp_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/baseapp/baseapp_test.go b/baseapp/baseapp_test.go index b6bcc4eed2ba..fb8a74dbfd71 100644 --- a/baseapp/baseapp_test.go +++ b/baseapp/baseapp_test.go @@ -32,6 +32,7 @@ import ( "github.com/cosmos/cosmos-sdk/testutil" "github.com/cosmos/cosmos-sdk/testutil/testdata" sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/cosmos/cosmos-sdk/types/mempool" ) var ( @@ -80,6 +81,12 @@ func NewBaseAppSuite(t *testing.T, opts ...func(*baseapp.BaseApp)) *BaseAppSuite app.SetParamStore(paramStore{db: dbm.NewMemDB()}) app.SetTxDecoder(txConfig.TxDecoder()) app.SetTxEncoder(txConfig.TxEncoder()) + mem := mempool.NewPriorityMempool[int64](mempool.PriorityNonceMempoolConfig[int64]{ + TxPriority: mempool.NewDefaultTxPriority(), + MaxTx: 0, + SignerExtractor: mempool.NewDefaultSignerExtractionAdapter(), + }) + app.SetMempool(mem) // mount stores and seal require.Nil(t, app.LoadLatestVersion()) From 70bffb88b038946f5d53f79e940a86a1516ac29b Mon Sep 17 00:00:00 2001 From: marbar3778 Date: Tue, 23 Apr 2024 15:36:51 +0200 Subject: [PATCH 03/11] avoid joining error when mempooltx is not found --- baseapp/baseapp.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go index eb72c4709a78..e0d8b2cdafb3 100644 --- a/baseapp/baseapp.go +++ b/baseapp/baseapp.go @@ -913,7 +913,8 @@ func (app *BaseApp) runTx(mode execMode, txBytes []byte) (gInfo sdk.GasInfo, res if err != nil { if mode == execModeReCheck { // if the ante handler fails on recheck, we want to remove the tx from the mempool - if err := app.mempool.Remove(tx); err != nil { + err := app.mempool.Remove(tx) + if err != nil && errors.Is(err, mempool.ErrTxNotFound) { return gInfo, nil, anteEvents, errors.Join(err, err) } } From f5da24771e63e713dffabca7051b8cc0115b0a35 Mon Sep 17 00:00:00 2001 From: marbar3778 Date: Tue, 23 Apr 2024 15:55:21 +0200 Subject: [PATCH 04/11] remove error is not found --- baseapp/baseapp.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go index e0d8b2cdafb3..0b9180a04441 100644 --- a/baseapp/baseapp.go +++ b/baseapp/baseapp.go @@ -914,7 +914,7 @@ func (app *BaseApp) runTx(mode execMode, txBytes []byte) (gInfo sdk.GasInfo, res if mode == execModeReCheck { // if the ante handler fails on recheck, we want to remove the tx from the mempool err := app.mempool.Remove(tx) - if err != nil && errors.Is(err, mempool.ErrTxNotFound) { + if err != nil { return gInfo, nil, anteEvents, errors.Join(err, err) } } From 0b802b19cd1cd4b7ab265903f57cc1413afea8b0 Mon Sep 17 00:00:00 2001 From: marbar3778 Date: Wed, 24 Apr 2024 11:22:22 +0200 Subject: [PATCH 05/11] address comment --- baseapp/baseapp.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go index 0b9180a04441..eb72c4709a78 100644 --- a/baseapp/baseapp.go +++ b/baseapp/baseapp.go @@ -913,8 +913,7 @@ func (app *BaseApp) runTx(mode execMode, txBytes []byte) (gInfo sdk.GasInfo, res if err != nil { if mode == execModeReCheck { // if the ante handler fails on recheck, we want to remove the tx from the mempool - err := app.mempool.Remove(tx) - if err != nil { + if err := app.mempool.Remove(tx); err != nil { return gInfo, nil, anteEvents, errors.Join(err, err) } } From 3d56c95cc78b82f3558e1dc581ebd134af21f56e Mon Sep 17 00:00:00 2001 From: marbar3778 Date: Wed, 24 Apr 2024 16:57:42 +0200 Subject: [PATCH 06/11] fix --- baseapp/abci_test.go | 4 ++-- baseapp/baseapp.go | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/baseapp/abci_test.go b/baseapp/abci_test.go index 4a3693094353..fc0c7fb544b7 100644 --- a/baseapp/abci_test.go +++ b/baseapp/abci_test.go @@ -1749,7 +1749,7 @@ func TestABCI_PrepareProposal_Failures(t *testing.T) { err = pool.Insert(sdk.Context{}, failTx) require.NoError(t, err) - require.Equal(t, 2, pool.CountTx()) + require.Equal(t, 1, pool.CountTx()) req := abci.RequestPrepareProposal{ MaxTxBytes: 1000, @@ -1757,7 +1757,7 @@ func TestABCI_PrepareProposal_Failures(t *testing.T) { } res, err := suite.baseApp.PrepareProposal(&req) require.NoError(t, err) - require.Equal(t, 1, len(res.Txs)) + require.Equal(t, 0, len(res.Txs)) } func TestABCI_PrepareProposal_PanicRecovery(t *testing.T) { diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go index eb72c4709a78..269ebe2f5221 100644 --- a/baseapp/baseapp.go +++ b/baseapp/baseapp.go @@ -912,6 +912,7 @@ func (app *BaseApp) runTx(mode execMode, txBytes []byte) (gInfo sdk.GasInfo, res if err != nil { if mode == execModeReCheck { + fmt.Println("here") // if the ante handler fails on recheck, we want to remove the tx from the mempool if err := app.mempool.Remove(tx); err != nil { return gInfo, nil, anteEvents, errors.Join(err, err) From 54ef00993bedf8284475e3c01b896899512c6ca2 Mon Sep 17 00:00:00 2001 From: Facundo Date: Thu, 2 May 2024 16:27:29 +0200 Subject: [PATCH 07/11] add rechecktx test --- baseapp/abci_test.go | 106 +++++++++++++++++++++++++++++++++++++++- baseapp/baseapp_test.go | 7 --- 2 files changed, 104 insertions(+), 9 deletions(-) diff --git a/baseapp/abci_test.go b/baseapp/abci_test.go index fc0c7fb544b7..5d1c79e97831 100644 --- a/baseapp/abci_test.go +++ b/baseapp/abci_test.go @@ -1749,7 +1749,7 @@ func TestABCI_PrepareProposal_Failures(t *testing.T) { err = pool.Insert(sdk.Context{}, failTx) require.NoError(t, err) - require.Equal(t, 1, pool.CountTx()) + require.Equal(t, 2, pool.CountTx()) req := abci.RequestPrepareProposal{ MaxTxBytes: 1000, @@ -1757,7 +1757,7 @@ func TestABCI_PrepareProposal_Failures(t *testing.T) { } res, err := suite.baseApp.PrepareProposal(&req) require.NoError(t, err) - require.Equal(t, 0, len(res.Txs)) + require.Equal(t, 1, len(res.Txs)) } func TestABCI_PrepareProposal_PanicRecovery(t *testing.T) { @@ -2398,3 +2398,105 @@ func TestOptimisticExecution(t *testing.T) { require.Equal(t, int64(50), suite.baseApp.LastBlockHeight()) } + +func TestABCI_Proposal_FailReCheckTx(t *testing.T) { + // mem := mempool.NewSenderNonceMempool() + 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_test.go b/baseapp/baseapp_test.go index fb8a74dbfd71..b6bcc4eed2ba 100644 --- a/baseapp/baseapp_test.go +++ b/baseapp/baseapp_test.go @@ -32,7 +32,6 @@ import ( "github.com/cosmos/cosmos-sdk/testutil" "github.com/cosmos/cosmos-sdk/testutil/testdata" sdk "github.com/cosmos/cosmos-sdk/types" - "github.com/cosmos/cosmos-sdk/types/mempool" ) var ( @@ -81,12 +80,6 @@ func NewBaseAppSuite(t *testing.T, opts ...func(*baseapp.BaseApp)) *BaseAppSuite app.SetParamStore(paramStore{db: dbm.NewMemDB()}) app.SetTxDecoder(txConfig.TxDecoder()) app.SetTxEncoder(txConfig.TxEncoder()) - mem := mempool.NewPriorityMempool[int64](mempool.PriorityNonceMempoolConfig[int64]{ - TxPriority: mempool.NewDefaultTxPriority(), - MaxTx: 0, - SignerExtractor: mempool.NewDefaultSignerExtractionAdapter(), - }) - app.SetMempool(mem) // mount stores and seal require.Nil(t, app.LoadLatestVersion()) From 03732a346dd19e930e5d5694417268b161a3d7f2 Mon Sep 17 00:00:00 2001 From: Facundo Date: Thu, 2 May 2024 16:28:07 +0200 Subject: [PATCH 08/11] remove debug print --- baseapp/baseapp.go | 1 - 1 file changed, 1 deletion(-) diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go index 269ebe2f5221..eb72c4709a78 100644 --- a/baseapp/baseapp.go +++ b/baseapp/baseapp.go @@ -912,7 +912,6 @@ func (app *BaseApp) runTx(mode execMode, txBytes []byte) (gInfo sdk.GasInfo, res if err != nil { if mode == execModeReCheck { - fmt.Println("here") // if the ante handler fails on recheck, we want to remove the tx from the mempool if err := app.mempool.Remove(tx); err != nil { return gInfo, nil, anteEvents, errors.Join(err, err) From f30f863f744721b751b78ce611d4d6c87cae64c7 Mon Sep 17 00:00:00 2001 From: Facundo Date: Thu, 2 May 2024 16:28:56 +0200 Subject: [PATCH 09/11] fix: shadowing of err --- baseapp/baseapp.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go index eb72c4709a78..74ff44d7c547 100644 --- a/baseapp/baseapp.go +++ b/baseapp/baseapp.go @@ -913,8 +913,8 @@ func (app *BaseApp) runTx(mode execMode, txBytes []byte) (gInfo sdk.GasInfo, res if err != nil { if mode == execModeReCheck { // if the ante handler fails on recheck, we want to remove the tx from the mempool - if err := app.mempool.Remove(tx); err != nil { - return gInfo, nil, anteEvents, errors.Join(err, err) + if mempoolErr := app.mempool.Remove(tx); mempoolErr != nil { + return gInfo, nil, anteEvents, errors.Join(err, mempoolErr) } } return gInfo, nil, nil, err From cb3d39cdaaa35f0c6017972e20506b8947353170 Mon Sep 17 00:00:00 2001 From: Facundo Date: Thu, 2 May 2024 16:29:44 +0200 Subject: [PATCH 10/11] remove comment --- baseapp/abci_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/baseapp/abci_test.go b/baseapp/abci_test.go index 5d1c79e97831..036ecf223912 100644 --- a/baseapp/abci_test.go +++ b/baseapp/abci_test.go @@ -2400,7 +2400,6 @@ func TestOptimisticExecution(t *testing.T) { } func TestABCI_Proposal_FailReCheckTx(t *testing.T) { - // mem := mempool.NewSenderNonceMempool() pool := mempool.NewPriorityMempool[int64](mempool.PriorityNonceMempoolConfig[int64]{ TxPriority: mempool.NewDefaultTxPriority(), MaxTx: 0, From 7c5b6710a061ca83bb11920af7782c23c5bbb823 Mon Sep 17 00:00:00 2001 From: Facundo Date: Thu, 2 May 2024 16:41:01 +0200 Subject: [PATCH 11/11] lint --- baseapp/abci_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/baseapp/abci_test.go b/baseapp/abci_test.go index 036ecf223912..c8d9433c39b5 100644 --- a/baseapp/abci_test.go +++ b/baseapp/abci_test.go @@ -2490,7 +2490,7 @@ func TestABCI_Proposal_FailReCheckTx(t *testing.T) { // the same txs as in PrepareProposal res, err := suite.baseApp.FinalizeBlock(&abci.RequestFinalizeBlock{ Height: suite.baseApp.LastBlockHeight() + 1, - Txs: reqProposalTxBytes[:], + Txs: reqProposalTxBytes, }) require.NoError(t, err)