From dc90cf60d5e751cb9f1ef4dc24903d235c942694 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Mon, 21 Dec 2020 15:25:14 +0400 Subject: [PATCH] mempool: introduce KeepInvalidTxsInCache config option (#5813) When set to true, an invalid transaction will be kept in the cache (this may help some applications to protect against spam). NOTE: this is a temporary config option. The more correct solution would be to add a TTL to each transaction (i.e. CheckTx may return a TTL in ResponseCheckTx). Closes: #5751 --- CHANGELOG_PENDING.md | 4 +- config/config.go | 4 ++ config/toml.go | 5 +++ docs/tendermint-core/configuration.md | 5 +++ mempool/clist_mempool.go | 10 +++-- mempool/clist_mempool_test.go | 57 +++++++++++++++++++++++++++ 6 files changed, 79 insertions(+), 6 deletions(-) diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index 260963771..395b13f47 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -22,10 +22,10 @@ Friendly reminder, we have a [bug bounty program](https://hackerone.com/tendermi ### FEATURES - - ### IMPROVEMENTS +- [mempool] \#5813 Add `keep-invalid-txs-in-cache` config option. When set to true, mempool will keep invalid transactions in the cache (@p4u) + ### BUG FIXES - [crypto] \#5707 Fix infinite recursion in string formatting of Secp256k1 keys (@erikgrinaker) diff --git a/config/config.go b/config/config.go index 5e4eb9c77..882e59fb4 100644 --- a/config/config.go +++ b/config/config.go @@ -656,6 +656,10 @@ type MempoolConfig struct { MaxTxsBytes int64 `mapstructure:"max_txs_bytes"` // Size of the cache (used to filter transactions we saw earlier) in transactions CacheSize int `mapstructure:"cache_size"` + // Do not remove invalid transactions from the cache (default: false) + // Set to true if it's not possible for any invalid transaction to become + // valid again in the future. + KeepInvalidTxsInCache bool `mapstructure:"keep-invalid-txs-in-cache"` // Maximum size of a single transaction // NOTE: the max size of a tx transmitted over the network is {max_tx_bytes}. MaxTxBytes int `mapstructure:"max_tx_bytes"` diff --git a/config/toml.go b/config/toml.go index 0dce3e6dc..62a17db9a 100644 --- a/config/toml.go +++ b/config/toml.go @@ -329,6 +329,11 @@ max_txs_bytes = {{ .Mempool.MaxTxsBytes }} # Size of the cache (used to filter transactions we saw earlier) in transactions cache_size = {{ .Mempool.CacheSize }} +# Do not remove invalid transactions from the cache (default: false) +# Set to true if it's not possible for any invalid transaction to become valid +# again in the future. +keep-invalid-txs-in-cache = {{ .Mempool.KeepInvalidTxsInCache }} + # Maximum size of a single transaction. # NOTE: the max size of a tx transmitted over the network is {max_tx_bytes}. max_tx_bytes = {{ .Mempool.MaxTxBytes }} diff --git a/docs/tendermint-core/configuration.md b/docs/tendermint-core/configuration.md index 1d79e9068..16a600cc0 100644 --- a/docs/tendermint-core/configuration.md +++ b/docs/tendermint-core/configuration.md @@ -278,6 +278,11 @@ max_txs_bytes = 1073741824 # Size of the cache (used to filter transactions we saw earlier) in transactions cache_size = 10000 +# Do not remove invalid transactions from the cache (default: false) +# Set to true if it's not possible for any invalid transaction to become valid +# again in the future. +keep-invalid-txs-in-cache = false + # Maximum size of a single transaction. # NOTE: the max size of a tx transmitted over the network is {max_tx_bytes}. max_tx_bytes = 1048576 diff --git a/mempool/clist_mempool.go b/mempool/clist_mempool.go index 7b0c97522..e58a84df3 100644 --- a/mempool/clist_mempool.go +++ b/mempool/clist_mempool.go @@ -439,8 +439,10 @@ func (mem *CListMempool) resCbFirstTime( mem.logger.Info("Rejected bad transaction", "tx", txID(tx), "peerID", peerP2PID, "res", r, "err", postCheckErr) mem.metrics.FailedTxs.Add(1) - // remove from cache (it might be good later) - mem.cache.Remove(tx) + if !mem.config.KeepInvalidTxsInCache { + // remove from cache (it might be good later) + mem.cache.Remove(tx) + } } default: // ignore other messages @@ -472,7 +474,7 @@ func (mem *CListMempool) resCbRecheck(req *abci.Request, res *abci.Response) { // Tx became invalidated due to newly committed block. mem.logger.Info("Tx is no longer valid", "tx", txID(tx), "res", r, "err", postCheckErr) // NOTE: we remove tx from the cache because it might be good later - mem.removeTx(tx, mem.recheckCursor, true) + mem.removeTx(tx, mem.recheckCursor, !mem.config.KeepInvalidTxsInCache) } if mem.recheckCursor == mem.recheckEnd { mem.recheckCursor = nil @@ -586,7 +588,7 @@ func (mem *CListMempool) Update( if deliverTxResponses[i].Code == abci.CodeTypeOK { // Add valid committed tx to the cache (if missing). _ = mem.cache.Push(tx) - } else { + } else if !mem.config.KeepInvalidTxsInCache { // Allow invalid transactions to be resubmitted. mem.cache.Remove(tx) } diff --git a/mempool/clist_mempool_test.go b/mempool/clist_mempool_test.go index 46c161b65..a40ba69af 100644 --- a/mempool/clist_mempool_test.go +++ b/mempool/clist_mempool_test.go @@ -216,6 +216,63 @@ func TestMempoolUpdate(t *testing.T) { } } +func TestMempool_KeepInvalidTxsInCache(t *testing.T) { + app := counter.NewApplication(true) + cc := proxy.NewLocalClientCreator(app) + wcfg := cfg.DefaultConfig() + wcfg.Mempool.KeepInvalidTxsInCache = true + mempool, cleanup := newMempoolWithAppAndConfig(cc, wcfg) + defer cleanup() + + // 1. An invalid transaction must remain in the cache after Update + { + a := make([]byte, 8) + binary.BigEndian.PutUint64(a, 0) + + b := make([]byte, 8) + binary.BigEndian.PutUint64(b, 1) + + err := mempool.CheckTx(b, nil, TxInfo{}) + require.NoError(t, err) + + // simulate new block + _ = app.DeliverTx(abci.RequestDeliverTx{Tx: a}) + _ = app.DeliverTx(abci.RequestDeliverTx{Tx: b}) + err = mempool.Update(1, []types.Tx{a, b}, + []*abci.ResponseDeliverTx{{Code: abci.CodeTypeOK}, {Code: 2}}, nil, nil) + require.NoError(t, err) + + // a must be added to the cache + err = mempool.CheckTx(a, nil, TxInfo{}) + if assert.Error(t, err) { + assert.Equal(t, ErrTxInCache, err) + } + + // b must remain in the cache + err = mempool.CheckTx(b, nil, TxInfo{}) + if assert.Error(t, err) { + assert.Equal(t, ErrTxInCache, err) + } + } + + // 2. An invalid transaction must remain in the cache + { + a := make([]byte, 8) + binary.BigEndian.PutUint64(a, 0) + + // remove a from the cache to test (2) + mempool.cache.Remove(a) + + err := mempool.CheckTx(a, nil, TxInfo{}) + require.NoError(t, err) + + err = mempool.CheckTx(a, nil, TxInfo{}) + if assert.Error(t, err) { + assert.Equal(t, ErrTxInCache, err) + } + } +} + func TestTxsAvailable(t *testing.T) { app := kvstore.NewApplication() cc := proxy.NewLocalClientCreator(app)