mempool: add nop mempool (cometbft#1643)

* add `nop` mempool See [ADR-111](cometbft#1585) * implement NopMempool and NopMempoolReactor modify node.go logic I had to add NopMempoolReactor to pass it to RPC in order not to change it. * check config instead of asserting for nil * start writing docs * add changelog * move changelog * expand docs * remove unused func arguments * add simple test * make linter happy again * doc fixes Co-authored-by: Sergio Mena <sergio@informal.systems> * rename mempoolReactor to waitSyncP2PReactor * improve changelog message * allow empty string for backwards compatibility cometbft#1643 (comment) * make ErrNotAllowed private * mention `create_empty_blocks` in toml https://github.com/cometbft/cometbft/pull/1643/files#r1400434715 * return nil instead of closed channel https://github.com/cometbft/cometbft/pull/1643/files#r1400252575 The reader will block forever, which is exactly what we need. * grammar fixes Co-authored-by: lasaro <lasaro@informal.systems> * update changelog entry * adapt ADR to implementation * remove old ToC entry --------- Co-authored-by: Andy Nogueira <me@andynogueira.dev> Co-authored-by: Sergio Mena <sergio@informal.systems> Co-authored-by: lasaro <lasaro@informal.systems>
fabiotudone-da · Nov 23, 2023 · bc83503 · bc83503
1 parent 6302491
commit bc83503
Show file tree

Hide file tree

Showing 12 changed files with 326 additions and 39 deletions.
diff --git a/.changelog/unreleased/features/1643-nop-mempool.md b/.changelog/unreleased/features/1643-nop-mempool.md
@@ -0,0 +1,17 @@
+- `[mempool]` Add `nop` mempool ([\#1643](https://github.com/cometbft/cometbft/pull/1643))
+
+  If you want to use it, change mempool's `type` to `nop`:
+
+  ```toml
+  [mempool]
+
+  # The type of mempool for this node to use.
+  #
+  # Possible types:
+  # - "flood" : concurrent linked list mempool with flooding gossip protocol
+  # (default)
+  # - "nop"   : nop-mempool (short for no operation; the ABCI app is responsible
+  # for storing, disseminating and proposing txs). "create_empty_blocks=false"
+  # is not supported.
+  type = "nop"
+  ```
diff --git a/config/config.go b/config/config.go
@@ -48,6 +48,9 @@ const (
 	v0 = "v0"
 	v1 = "v1"
 	v2 = "v2"
+
+	MempoolTypeFlood = "flood"
+	MempoolTypeNop   = "nop"
 )
 
 // NOTE: Most of the structs & relevant comments + the
@@ -167,6 +170,9 @@ func (cfg *Config) ValidateBasic() error {
 	if err := cfg.Instrumentation.ValidateBasic(); err != nil {
 		return ErrInSection{Section: "instrumentation", Err: err}
 	}
+	if !cfg.Consensus.CreateEmptyBlocks && cfg.Mempool.Type == MempoolTypeNop {
+		return fmt.Errorf("`nop` mempool does not support create_empty_blocks = false")
+	}
 	return nil
 }
 
@@ -836,6 +842,15 @@ func DefaultFuzzConnConfig() *FuzzConnConfig {
 // implementation (previously called v0), and a prioritized mempool (v1), which
 // was removed (see https://github.com/cometbft/cometbft/issues/260).
 type MempoolConfig struct {
+	// The type of mempool for this node to use.
+	//
+	//  Possible types:
+	//  - "flood" : concurrent linked list mempool with flooding gossip protocol
+	//  (default)
+	//  - "nop"   : nop-mempool (short for no operation; the ABCI app is
+	//  responsible for storing, disseminating and proposing txs).
+	//  "create_empty_blocks=false" is not supported.
+	Type string `mapstructure:"type"`
 	// RootDir is the root directory for all data. This should be configured via
 	// the $CMTHOME env variable or --home cmd flag rather than overriding this
 	// struct field.
@@ -894,6 +909,7 @@ type MempoolConfig struct {
 // DefaultMempoolConfig returns a default configuration for the CometBFT mempool
 func DefaultMempoolConfig() *MempoolConfig {
 	return &MempoolConfig{
+		Type:      MempoolTypeFlood,
 		Recheck:   true,
 		Broadcast: true,
 		WalPath:   "",
@@ -928,6 +944,12 @@ func (cfg *MempoolConfig) WalEnabled() bool {
 // ValidateBasic performs basic validation (checking param bounds, etc.) and
 // returns an error if any check fails.
 func (cfg *MempoolConfig) ValidateBasic() error {
+	switch cfg.Type {
+	case MempoolTypeFlood, MempoolTypeNop:
+	case "": // allow empty string to be backwards compatible
+	default:
+		return fmt.Errorf("unknown mempool type: %q", cfg.Type)
+	}
 	if cfg.Size < 0 {
 		return cmterrors.ErrNegativeField{Field: "size"}
 	}

diff --git a/config/config_test.go b/config/config_test.go
@@ -38,6 +38,11 @@ func TestConfigValidateBasic(t *testing.T) {
 	// tamper with timeout_propose
 	cfg.Consensus.TimeoutPropose = -10 * time.Second
 	assert.Error(t, cfg.ValidateBasic())
+	cfg.Consensus.TimeoutPropose = 3 * time.Second
+
+	cfg.Consensus.CreateEmptyBlocks = false
+	cfg.Mempool.Type = config.MempoolTypeNop
+	assert.Error(t, cfg.ValidateBasic())
 }
 
 func TestTLSConfiguration(t *testing.T) {
@@ -121,6 +126,9 @@ func TestMempoolConfigValidateBasic(t *testing.T) {
 		assert.Error(t, cfg.ValidateBasic())
 		reflect.ValueOf(cfg).Elem().FieldByName(fieldName).SetInt(0)
 	}
+
+	reflect.ValueOf(cfg).Elem().FieldByName("Type").SetString("invalid")
+	assert.Error(t, cfg.ValidateBasic())
 }
 
 func TestStateSyncConfigValidateBasic(t *testing.T) {

diff --git a/config/toml.go b/config/toml.go
@@ -380,6 +380,16 @@ dial_timeout = "{{ .P2P.DialTimeout }}"
 #######################################################
 [mempool]
 
+# The type of mempool for this node to use.
+#
+#  Possible types:
+#  - "flood" : concurrent linked list mempool with flooding gossip protocol
+#  (default)
+#  - "nop"   : nop-mempool (short for no operation; the ABCI app is responsible
+#  for storing, disseminating and proposing txs). "create_empty_blocks=false" is
+#  not supported.
+type = "flood"
+
 # recheck (default: true) defines whether CometBFT should recheck the
 # validity for all remaining transaction in the mempool after a block.
 # Since a block affects the application state, some transactions in the

diff --git a/docs/architecture/README.md b/docs/architecture/README.md
@@ -45,7 +45,6 @@ numbering our ADRs from 100 onwards.
 - [ADR-103: Protobuf definition versioning](./adr-103-proto-versioning.md)
 - [ADR-105: Refactor list of senders in mempool](./adr-105-refactor-mempool-senders.md)
 - [ADR-109: Reduce CometBFT Go API Surface Area](./adr-109-reduce-go-api-surface.md)
-- [ADR-111: Nil Mempool](./adr-111-nil-mempool.md)
 
 ### Accepted
 

diff --git a/docs/architecture/adr-111-nop-mempool.md b/docs/architecture/adr-111-nop-mempool.md
@@ -6,6 +6,7 @@
 - 2023-11-15: Addressed PR comments (@sergio-mena)
 - 2023-11-17: Renamed `nil` to `nop` (@melekes)
 - 2023-11-20: Mentioned that the app could reuse p2p network in the future (@melekes)
+- 2023-11-22: Adapt ADR to implementation (@melekes)
 
 ## Status
 
@@ -157,15 +158,15 @@ within CometBFT.
 The `nop` Mempool implements the `Mempool` interface in a very simple manner:
 
 *	`CheckTx(tx types.Tx) (*abcicli.ReqRes, error)`: returns `nil, ErrNotAllowed`
-*	`RemoveTxByKey(txKey types.TxKey) error`: returns `ErrNotFound`
+*	`RemoveTxByKey(txKey types.TxKey) error`: returns `ErrNotAllowed`
 * `ReapMaxBytesMaxGas(maxBytes, maxGas int64) types.Txs`: returns `nil`
 * `ReapMaxTxs(max int) types.Txs`: returns `nil`
 *	`Lock()`: does nothing
 * `Unlock()`: does nothing
 *	`Update(...) error`: returns `nil`
 * `FlushAppConn() error`: returns `nil`
 *	`Flush()`: does nothing
-* `TxsAvailable() <-chan struct{}`: returns a closed channel
+* `TxsAvailable() <-chan struct{}`: returns `nil`
 * `EnableTxsAvailable()`: does nothing
 * `SetTxRemovedCallback(cb func(types.TxKey))`: does nothing
 * `Size() int` returns 0
@@ -221,13 +222,16 @@ will simply be calling the new implementation.
   These considerations are exclusively the application's concern in this approach.
 * *Time to propose a block*. The consensus reactor will call `ReapMaxBytesMaxGas` which will return a `nil` slice.
   `RequestPrepareProposal` will thus contain no transactions.
-* *Consensus waiting for transactions to become available*. `TxsAvailable()` returns a closed channel,
-  so consensus doesn't block (potentially producing empty blocks).
+* *Consensus waiting for transactions to become available*. `TxsAvailable()` returns `nil`. 
+  `cs.handleTxsAvailable()` won't ever be executed.
   At any rate, a configuration with the `nop` mempool and `create_empty_blocks` set to `false`
   will be rejected in the first place.
 * *A new block is decided*.
   * When `Update` is called, nothing is done (no decided transaction is removed).
   * Locking and unlocking the mempool has no effect.
+* *ABCI mempool's connection*
+  CometBFT will still open a "mempool" connection, even though it won't be used.
+  This is to avoid doing lots of breaking changes.
 
 ### Impact on Current Release Plans
 
@@ -255,8 +259,9 @@ However, it is not a clear-cut decision. These are the alternatives we see:
   in those versions where there were more than one mempool to choose from.
   As the configuration is in `config.toml`, it is up to the node operators to configure their
   nodes consistently, via social consensus. However this cannot be guaranteed.
-  A network with an inconsistent choice of mempool at different nodes might result in
-  hard-to-diagnose, undesirable side effects.
+  A network with an inconsistent choice of mempool at different nodes might
+  result in undesirable side effects, such as peers disconnecting from nodes
+  that sent them messages via the mempool channel.
 * *Mempool selected as a network-wide parameter*.
   A way to prevent any inconsistency when selecting the mempool is to move the configuration out of `config.toml`
   and have it as a network-wide application-enforced parameter, implemented in the same way as Consensus Params.

diff --git a/docs/core/configuration.md b/docs/core/configuration.md
@@ -305,6 +305,16 @@ dial_timeout = "3s"
 #######################################################
 [mempool]
 
+# The type of mempool for this node to use.
+#
+#  Possible types:
+#  - "flood" : concurrent linked list mempool with flooding gossip protocol
+#  (default)
+#  - "nop"   : nop-mempool (short for no operation; the ABCI app is responsible
+#  for storing, disseminating and proposing txs). "create_empty_blocks=false" is
+#  not supported.
+type = "flood"
+
 # recheck (default: true) defines whether CometBFT should recheck the
 # validity for all remaining transaction in the mempool after a block.
 # Since a block affects the application state, some transactions in the

diff --git a/docs/core/mempool.md b/docs/core/mempool.md
@@ -4,7 +4,41 @@ order: 12
 
 # Mempool
 
-## Transaction ordering
+A mempool (a contraction of memory and pool) is a node’s data structure for
+storing information on uncommitted transactions. It acts as a sort of waiting
+room for transactions that have not yet been committed.
+
+CometBFT currently supports two types of mempools: `flood` and `nop`.
+
+## 1. Flood
+
+The `flood` mempool stores transactions in a concurrent linked list. When a new
+transaction is received, it first checks if there's a space for it (`size` and
+`max_txs_bytes` config options) and that it's not too big (`max_tx_bytes` config
+option). Then, it checks if this transaction has already been seen before by using
+an LRU cache (`cache_size` regulates the cache's size). If all checks pass and
+the transaction is not in the cache (meaning it's new), the ABCI
+[`CheckTxAsync`][1] method is called. The ABCI application validates the
+transaction using its own rules.
+
+If the transaction is deemed valid by the ABCI application, it's added to the linked list.
+
+The mempool's name (`flood`) comes from the dissemination mechanism. When a new
+transaction is added to the linked list, the mempool sends it to all connected
+peers. Peers themselves gossip this transaction to their peers and so on. One
+can say that each transaction "floods" the network, hence the name `flood`.
+
+Note there are experimental config options
+`experimental_max_gossip_connections_to_persistent_peers` and
+`experimental_max_gossip_connections_to_non_persistent_peers` to limit the
+number of peers a transaction is broadcasted to. Also, you can turn off
+broadcasting with `broadcast` config option.
+
+After each committed block, CometBFT rechecks all uncommitted transactions (can
+be disabled with the `recheck` config option) by repeatedly calling the ABCI
+`CheckTxAsync`.
+
+### Transaction ordering
 
 Currently, there's no ordering of transactions other than the order they've
 arrived (via RPC or from other nodes).
@@ -46,3 +80,24 @@ order/nonce/sequence number, the application can reject transactions that are
 out of order. So if a node receives `tx3`, then `tx1`, it can reject `tx3` and then
 accept `tx1`. The sender can then retry sending `tx3`, which should probably be
 rejected until the node has seen `tx2`.
+
+## 2. Nop
+
+`nop` (short for no operation) mempool is used when the ABCI application developer wants to
+build their own mempool. When `type = "nop"`, transactions are not stored anywhere
+and are not gossiped to other peers using the P2P network.
+
+Submitting a transaction via the existing RPC methods (`BroadcastTxSync`,
+`BroadcastTxAsync`, and `BroadcastTxCommit`) will always result in an error.
+
+Because there's no way for the consensus to know if transactions are available
+to be committed, the node will always create blocks, which can be empty
+sometimes. Using `consensus.create_empty_blocks=false` is prohibited in such
+cases.
+
+The ABCI application becomes responsible for storing, disseminating, and
+proposing transactions using [`PrepareProposal`][2]. The concrete design is up
+to the ABCI application developers.
+
+[1]: ../../spec/abci/abci++_methods.md#checktx
+[2]: ../../spec/abci/abci++_methods.md#prepareproposal
diff --git a/mempool/nop_mempool.go b/mempool/nop_mempool.go
@@ -0,0 +1,111 @@
+package mempool
+
+import (
+	"errors"
+
+	abcicli "github.com/cometbft/cometbft/abci/client"
+	abci "github.com/cometbft/cometbft/abci/types"
+	"github.com/cometbft/cometbft/internal/service"
+	"github.com/cometbft/cometbft/p2p"
+	"github.com/cometbft/cometbft/types"
+)
+
+// NopMempool is a mempool that does nothing.
+//
+// The ABCI app is responsible for storing, disseminating, and proposing transactions.
+// See [ADR-111](../docs/architecture/adr-111-nop-mempool.md).
+type NopMempool struct{}
+
+// errNotAllowed indicates that the operation is not allowed with `nop` mempool.
+var errNotAllowed = errors.New("not allowed with `nop` mempool")
+
+var _ Mempool = &NopMempool{}
+
+// CheckTx always returns an error.
+func (*NopMempool) CheckTx(types.Tx) (*abcicli.ReqRes, error) {
+	return nil, errNotAllowed
+}
+
+// RemoveTxByKey always returns an error.
+func (*NopMempool) RemoveTxByKey(types.TxKey) error { return errNotAllowed }
+
+// ReapMaxBytesMaxGas always returns nil.
+func (*NopMempool) ReapMaxBytesMaxGas(int64, int64) types.Txs { return nil }
+
+// ReapMaxTxs always returns nil.
+func (*NopMempool) ReapMaxTxs(int) types.Txs { return nil }
+
+// Lock does nothing.
+func (*NopMempool) Lock() {}
+
+// Unlock does nothing.
+func (*NopMempool) Unlock() {}
+
+// Update does nothing.
+func (*NopMempool) Update(
+	int64,
+	types.Txs,
+	[]*abci.ExecTxResult,
+	PreCheckFunc,
+	PostCheckFunc,
+) error {
+	return nil
+}
+
+// FlushAppConn does nothing.
+func (*NopMempool) FlushAppConn() error { return nil }
+
+// Flush does nothing.
+func (*NopMempool) Flush() {}
+
+// TxsAvailable always returns nil.
+func (*NopMempool) TxsAvailable() <-chan struct{} {
+	return nil
+}
+
+// EnableTxsAvailable does nothing.
+func (*NopMempool) EnableTxsAvailable() {}
+
+// SetTxRemovedCallback does nothing.
+func (*NopMempool) SetTxRemovedCallback(func(txKey types.TxKey)) {}
+
+// Size always returns 0.
+func (*NopMempool) Size() int { return 0 }
+
+// SizeBytes always returns 0.
+func (*NopMempool) SizeBytes() int64 { return 0 }
+
+// NopMempoolReactor is a mempool reactor that does nothing.
+type NopMempoolReactor struct {
+	service.BaseService
+}
+
+// NewNopMempoolReactor returns a new `nop` reactor.
+//
+// To be used only in RPC.
+func NewNopMempoolReactor() *NopMempoolReactor {
+	return &NopMempoolReactor{*service.NewBaseService(nil, "NopMempoolReactor", nil)}
+}
+
+var _ p2p.Reactor = &NopMempoolReactor{}
+
+// WaitSync always returns false.
+func (*NopMempoolReactor) WaitSync() bool { return false }
+
+// GetChannels always returns nil.
+func (*NopMempoolReactor) GetChannels() []*p2p.ChannelDescriptor { return nil }
+
+// AddPeer does nothing.
+func (*NopMempoolReactor) AddPeer(p2p.Peer) {}
+
+// InitPeer always returns nil.
+func (*NopMempoolReactor) InitPeer(p2p.Peer) p2p.Peer { return nil }
+
+// RemovePeer does nothing.
+func (*NopMempoolReactor) RemovePeer(p2p.Peer, interface{}) {}
+
+// Receive does nothing.
+func (*NopMempoolReactor) Receive(p2p.Envelope) {}
+
+// SetSwitch does nothing.
+func (*NopMempoolReactor) SetSwitch(*p2p.Switch) {}