From ae572b9038bf1086ad4257c9127afa48d49e113e Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Thu, 17 May 2018 11:52:07 +0400 Subject: [PATCH 1/5] remove extra call to Exists Refs #345 https://github.com/tendermint/tendermint/issues/1539#issuecomment-387240606 --- mempool/mempool.go | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/mempool/mempool.go b/mempool/mempool.go index aa2aa4f4..70ef7a38 100644 --- a/mempool/mempool.go +++ b/mempool/mempool.go @@ -203,10 +203,9 @@ func (mem *Mempool) CheckTx(tx types.Tx, cb func(*abci.Response)) (err error) { defer mem.proxyMtx.Unlock() // CACHE - if mem.cache.Exists(tx) { + if !mem.cache.Push(tx) { return ErrTxInCache } - mem.cache.Push(tx) // END CACHE // WAL @@ -463,14 +462,6 @@ func (cache *txCache) Reset() { cache.mtx.Unlock() } -// Exists returns true if the given tx is cached. -func (cache *txCache) Exists(tx types.Tx) bool { - cache.mtx.Lock() - _, exists := cache.map_[string(tx)] - cache.mtx.Unlock() - return exists -} - // Push adds the given tx to the txCache. It returns false if tx is already in the cache. func (cache *txCache) Push(tx types.Tx) bool { cache.mtx.Lock() From 90446261f3e38df6c85c86f0b4e00f4017b48820 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Thu, 17 May 2018 12:20:29 +0400 Subject: [PATCH 2/5] [docs] document transactional semantics Refs #1494, #345 --- docs/transactional-semantics.rst | 27 +++++++++++++++++++++++++++ docs/using-tendermint.rst | 2 +- 2 files changed, 28 insertions(+), 1 deletion(-) create mode 100644 docs/transactional-semantics.rst diff --git a/docs/transactional-semantics.rst b/docs/transactional-semantics.rst new file mode 100644 index 00000000..268247a2 --- /dev/null +++ b/docs/transactional-semantics.rst @@ -0,0 +1,27 @@ +Transactional Semantics +======================= + +In `"Using +Tendermint"<./specification/using-tendermint.html#broadcast-api>`__ we +discussed different API endpoints for sending transactions and +differences between them. + +What we have not yet covered is transactional semantics. + +When you send a transaction using one of the available methods, it +first goes to the mempool. Currently, it does not provide strong +guarantees like "if the transaction were accepted, it would be +eventually included in a block (given CheckTx passes)." + +For instance a tx could enter the mempool, but before it can be sent +to peers the node crashes. + +We are planning to provide such guarantees by using a WAL and +replaying transactions (See +`GH#248`__), but +it's non-trivial to do this all efficiently. + +The temporary solution is for clients to monitor the node and resubmit +transaction(s) or/and send them to more nodes at once, so the +probability of all of them crashing at the same time and losing the +msg decreases substantially. diff --git a/docs/using-tendermint.rst b/docs/using-tendermint.rst index 394a7f3e..f572277c 100644 --- a/docs/using-tendermint.rst +++ b/docs/using-tendermint.rst @@ -214,7 +214,7 @@ Broadcast API Earlier, we used the ``broadcast_tx_commit`` endpoint to send a transaction. When a transaction is sent to a Tendermint node, it will run via ``CheckTx`` against the application. If it passes ``CheckTx``, -it will be included in the mempool, broadcast to other peers, and +it will be included in the mempool, broadcasted to other peers, and eventually included in a block. Since there are multiple phases to processing a transaction, we offer From c9001d5a11dba8de01883128c81f0702a72361fd Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Thu, 17 May 2018 13:38:40 +0400 Subject: [PATCH 3/5] bound the mempool Refs #345 --- config/config.go | 2 ++ config/toml.go | 6 ++++++ docs/specification/configuration.rst | 6 ++++++ mempool/mempool.go | 13 +++++++++++-- 4 files changed, 25 insertions(+), 2 deletions(-) diff --git a/config/config.go b/config/config.go index b76f5ed1..47df4626 100644 --- a/config/config.go +++ b/config/config.go @@ -335,6 +335,7 @@ type MempoolConfig struct { RecheckEmpty bool `mapstructure:"recheck_empty"` Broadcast bool `mapstructure:"broadcast"` WalPath string `mapstructure:"wal_dir"` + Size int `mapstructure:"size"` CacheSize int `mapstructure:"cache_size"` } @@ -345,6 +346,7 @@ func DefaultMempoolConfig() *MempoolConfig { RecheckEmpty: true, Broadcast: true, WalPath: filepath.Join(defaultDataDir, "mempool.wal"), + Size: 100000, CacheSize: 100000, } } diff --git a/config/toml.go b/config/toml.go index a19fb315..3f4c7dda 100644 --- a/config/toml.go +++ b/config/toml.go @@ -179,6 +179,12 @@ recheck_empty = {{ .Mempool.RecheckEmpty }} broadcast = {{ .Mempool.Broadcast }} wal_dir = "{{ .Mempool.WalPath }}" +# size of the mempool +size = {{ .Mempool.Size }} + +# size of the cache (used to filter transactions we saw earlier) +cache_size = {{ .Mempool.CacheSize }} + ##### consensus configuration options ##### [consensus] diff --git a/docs/specification/configuration.rst b/docs/specification/configuration.rst index 6b52dbd1..2282095b 100644 --- a/docs/specification/configuration.rst +++ b/docs/specification/configuration.rst @@ -136,6 +136,12 @@ like the file below, however, double check by inspecting the broadcast = true wal_dir = "data/mempool.wal" + # size of the mempool + size = 100000 + + # size of the cache (used to filter transactions we saw earlier) + cache_size = 100000 + ##### consensus configuration options ##### [consensus] diff --git a/mempool/mempool.go b/mempool/mempool.go index 70ef7a38..f4ecf00f 100644 --- a/mempool/mempool.go +++ b/mempool/mempool.go @@ -49,7 +49,13 @@ TODO: Better handle abci client errors. (make it automatically handle connection */ -var ErrTxInCache = errors.New("Tx already exists in cache") +var ( + // ErrTxInCache is returned to the client if we saw tx earlier + ErrTxInCache = errors.New("Tx already exists in cache") + + // ErrMempoolIsFull means Tendermint & an application can't handle that much load + ErrMempoolIsFull = errors.New("Mempool is full") +) // Mempool is an ordered in-memory pool for transactions before they are proposed in a consensus // round. Transaction validity is checked using the CheckTx abci message before the transaction is @@ -80,7 +86,6 @@ type Mempool struct { } // NewMempool returns a new Mempool with the given configuration and connection to an application. -// TODO: Extract logger into arguments. func NewMempool(config *cfg.MempoolConfig, proxyAppConn proxy.AppConnMempool, height int64) *Mempool { mempool := &Mempool{ config: config, @@ -202,6 +207,10 @@ func (mem *Mempool) CheckTx(tx types.Tx, cb func(*abci.Response)) (err error) { mem.proxyMtx.Lock() defer mem.proxyMtx.Unlock() + if mem.Size() >= mem.config.Size { + return ErrMempoolIsFull + } + // CACHE if !mem.cache.Push(tx) { return ErrTxInCache From 2987158a653bfb0ce72c78621a2e42373cd2372d Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Fri, 18 May 2018 10:43:46 +0400 Subject: [PATCH 4/5] [docs] add a note about replay protection Refs #345, https://github.com/tendermint/tendermint/issues/458#issuecomment-297077148 --- docs/app-development.rst | 46 ++++++++++++++++++++++++++++------------ 1 file changed, 32 insertions(+), 14 deletions(-) diff --git a/docs/app-development.rst b/docs/app-development.rst index 42083f74..9aefa99c 100644 --- a/docs/app-development.rst +++ b/docs/app-development.rst @@ -178,21 +178,22 @@ connection, to query the local state of the app. Mempool Connection ~~~~~~~~~~~~~~~~~~ -The mempool connection is used *only* for CheckTx requests. Transactions -are run using CheckTx in the same order they were received by the -validator. If the CheckTx returns ``OK``, the transaction is kept in -memory and relayed to other peers in the same order it was received. -Otherwise, it is discarded. +The mempool connection is used *only* for CheckTx requests. +Transactions are run using CheckTx in the same order they were +received by the validator. If the CheckTx returns ``OK``, the +transaction is kept in memory and relayed to other peers in the same +order it was received. Otherwise, it is discarded. -CheckTx requests run concurrently with block processing; so they should -run against a copy of the main application state which is reset after -every block. This copy is necessary to track transitions made by a -sequence of CheckTx requests before they are included in a block. When a -block is committed, the application must ensure to reset the mempool -state to the latest committed state. Tendermint Core will then filter -through all transactions in the mempool, removing any that were included -in the block, and re-run the rest using CheckTx against the post-Commit -mempool state. +CheckTx requests run concurrently with block processing; so they +should run against a copy of the main application state which is reset +after every block. This copy is necessary to track transitions made by +a sequence of CheckTx requests before they are included in a block. +When a block is committed, the application must ensure to reset the +mempool state to the latest committed state. Tendermint Core will then +filter through all transactions in the mempool, removing any that were +included in the block, and re-run the rest using CheckTx against the +post-Commit mempool state (this behaviour can be turned off with +``[mempool] recheck = false``). .. container:: toggle @@ -226,6 +227,23 @@ mempool state. } } +Replay Protection +^^^^^^^^^^^^^^^^^ +To prevent old transactions from being replayed, CheckTx must +implement replay protection. + +Tendermint provides the first defence layer by keeping a lightweight +in-memory cache of 100k (``[mempool] cache_size``) last transactions in +the mempool. If Tendermint is just started or the clients sent more +than 100k transactions, old transactions may be sent to the +application. So it is important CheckTx implements some logic to +handle them. + +There are cases where a transaction will (or may) become valid in some +future state, in which case you probably want to disable Tendermint's +cache. You can do that by setting ``[mempool] cache_size = 0`` in the +config. + Consensus Connection ~~~~~~~~~~~~~~~~~~~~ From 202a43a5af4675019adbfd601bfa14c883ff5e07 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Fri, 18 May 2018 10:48:13 +0400 Subject: [PATCH 5/5] remove TODO no longer relevant, I guess. since ABCI only defines 0 (success) code. --- mempool/mempool.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/mempool/mempool.go b/mempool/mempool.go index f4ecf00f..938fb2a7 100644 --- a/mempool/mempool.go +++ b/mempool/mempool.go @@ -272,8 +272,6 @@ func (mem *Mempool) resCbNormal(req *abci.Request, res *abci.Response) { // remove from cache (it might be good later) mem.cache.Remove(tx) - - // TODO: handle other retcodes } default: // ignore other messages