From ba3c8d8233dc5eed8f74921ec7dcd1c0998f2722 Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Thu, 23 Nov 2023 20:15:46 +0400 Subject: [PATCH] mempool: add `nop` mempool (backport #1643) (#1681) * mempool: add `nop` mempool (#1643) * add `nop` mempool See [ADR-111](https://github.com/cometbft/cometbft/pull/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 * rename mempoolReactor to waitSyncP2PReactor * improve changelog message * allow empty string for backwards compatibility https://github.com/cometbft/cometbft/pull/1643#discussion_r1400378375 * 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 * update changelog entry * adapt ADR to implementation * remove old ToC entry --------- Co-authored-by: Andy Nogueira Co-authored-by: Sergio Mena Co-authored-by: lasaro (cherry picked from commit bc835036aa0f6bd9d8b681ff44ed536df97d950e) # Conflicts: # config/config.go # config/toml.go # docs/architecture/README.md # docs/architecture/adr-111-nop-mempool.md # docs/core/configuration.md # node/node.go # node/setup.go * fix merge conflicts * add a missing ToC line --------- Co-authored-by: Anton Kaliaev --- .../unreleased/features/1643-nop-mempool.md | 17 + config/config.go | 24 ++ config/config_test.go | 8 + config/toml.go | 10 + docs/architecture/README.md | 1 + docs/architecture/adr-111-nop-mempool.md | 324 ++++++++++++++++++ docs/core/configuration.md | 13 + docs/core/mempool.md | 57 ++- mempool/nop_mempool.go | 110 ++++++ mempool/nop_mempool_test.go | 38 ++ node/node.go | 102 +++--- 11 files changed, 657 insertions(+), 47 deletions(-) create mode 100644 .changelog/unreleased/features/1643-nop-mempool.md create mode 100644 docs/architecture/adr-111-nop-mempool.md create mode 100644 mempool/nop_mempool.go create mode 100644 mempool/nop_mempool_test.go diff --git a/.changelog/unreleased/features/1643-nop-mempool.md b/.changelog/unreleased/features/1643-nop-mempool.md new file mode 100644 index 00000000000..e12ec43fc1a --- /dev/null +++ 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" + ``` \ No newline at end of file diff --git a/config/config.go b/config/config.go index 5f2c5559530..88edd1e3a53 100644 --- a/config/config.go +++ b/config/config.go @@ -28,6 +28,9 @@ const ( // Default is v0. MempoolV0 = "v0" MempoolV1 = "v1" + + MempoolTypeFlood = "flood" + MempoolTypeNop = "nop" ) // NOTE: Most of the structs & relevant comments + the @@ -151,6 +154,9 @@ func (cfg *Config) ValidateBasic() error { if err := cfg.Instrumentation.ValidateBasic(); err != nil { return fmt.Errorf("error in [instrumentation] section: %w", err) } + if !cfg.Consensus.CreateEmptyBlocks && cfg.Mempool.Type == MempoolTypeNop { + return fmt.Errorf("`nop` mempool does not support create_empty_blocks = false") + } return nil } @@ -721,6 +727,17 @@ type MempoolConfig struct { // 1) "v0" - (default) FIFO mempool. // 2) "v1" - prioritized mempool (deprecated; will be removed in the next release). Version string `mapstructure:"version"` + + // 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. @@ -802,6 +819,7 @@ type MempoolConfig struct { // DefaultMempoolConfig returns a default configuration for the CometBFT mempool func DefaultMempoolConfig() *MempoolConfig { return &MempoolConfig{ + Type: MempoolTypeFlood, Version: MempoolV0, Recheck: true, Broadcast: true, @@ -839,6 +857,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 errors.New("size can't be negative") } diff --git a/config/config_test.go b/config/config_test.go index 86b32c7687e..ac455a5c458 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -37,6 +37,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 = 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 index 5b22847831f..c6c0b39253c 100644 --- a/config/toml.go +++ b/config/toml.go @@ -345,6 +345,16 @@ dial_timeout = "{{ .P2P.DialTimeout }}" # 2) "v1" - prioritized mempool (deprecated; will be removed in the next release). version = "{{ .Mempool.Version }}" +# 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 = {{ .Mempool.Recheck }} broadcast = {{ .Mempool.Broadcast }} wal_dir = "{{ js .Mempool.WalPath }}" diff --git a/docs/architecture/README.md b/docs/architecture/README.md index 7a2ad6fed4e..5e10cc7b119 100644 --- a/docs/architecture/README.md +++ b/docs/architecture/README.md @@ -84,6 +84,7 @@ therefore they have links to it and refer to CometBFT as "Tendermint" or "Tender - [ADR-075: RPC Event Subscription Interface](./adr-075-rpc-subscription.md) - [ADR-079: Ed25519 Verification](./adr-079-ed25519-verification.md) - [ADR-081: Protocol Buffers Management](./adr-081-protobuf-mgmt.md) +- [ADR-111: `nop` Mempool](./adr-111-nop-mempool.md) ### Deprecated diff --git a/docs/architecture/adr-111-nop-mempool.md b/docs/architecture/adr-111-nop-mempool.md new file mode 100644 index 00000000000..234cd5b9c1d --- /dev/null +++ b/docs/architecture/adr-111-nop-mempool.md @@ -0,0 +1,324 @@ +# ADR 111: `nop` Mempool + +## Changelog + +- 2023-11-07: First version (@sergio-mena) +- 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 + +Accepted + +[Tracking issue](https://github.com/cometbft/cometbft/issues/1666) + +## Context + +### Summary + +The current mempool built into CometBFT implements a robust yet somewhat inefficient transaction gossip mechanism. +While the CometBFT team is currently working on more efficient general-purpose transaction gossiping mechanisms, +some users have expressed their desire to manage both the mempool and the transaction dissemination mechanism +outside CometBFT (typically at the application level). + +This ADR proposes a fairly simple way for CometBFT to fulfill this use case without moving away from our current architecture. + +### In the Beginning... + +It is well understood that a dissemination mechanism +(sometimes using _Reliable Broadcast_ [\[HT94\]][HT94] but not necessarily), +is needed in a distributed system implementing State-Machine Replication (SMR). +This is also the case in blockchains. +Early designs such as Bitcoin or Ethereum include an _internal_ component, +responsible for dissemination, called mempool. +Tendermint Core chose to follow the same design given the success +of those early blockchains and, since inception, Tendermint Core and later CometBFT have featured a mempool as an internal piece of its architecture. + + +However, the design of ABCI clearly dividing the application logic (i.e., the appchain) +and the consensus logic that provides SMR semantics to the app is a unique innovation in Cosmos +that sets it apart from Bitcoin, Ethereum, and many others. +This clear separation of concerns entailed many consequences, mostly positive: +it allows CometBFT to be used underneath (currently) tens of different appchains in production +in the Cosmos ecosystem and elsewhere. +But there are other implications for having an internal mempool +in CometBFT: the interaction between the mempool, the application, and the network +becomes more indirect, and thus more complex and hard to understand and operate. + +### ABCI++ Improvements and Remaining Shortcomings + +Before the release of ABCI++, `CheckTx` was the main mechanism the app had at its disposal to influence +what transactions made it to the mempool, and very indirectly what transactions got ultimately proposed in a block. +Since ABCI 1.0 (the first part of ABCI++, shipped in `v0.37.x`), the application has +a more direct say in what is proposed through `PrepareProposal` and `ProcessProposal`. + +This has greatly improved the ability for appchains to influence the contents of the proposed block. +Further, ABCI++ has enabled many new use cases for appchains. However some issues remain with +the current model: + +* We are using the same P2P network for disseminating transactions and consensus-related messages. +* Many mempool parameters are configured on a per-node basis by node operators, + allowing the possibility of inconsistent mempool configuration across the network + with potentially serious scalability effects + (even causing unacceptable performance degradation in some extreme cases). +* The current mempool implementation uses a basic (robust but sub-optimal) flood algorithm + * the CometBFT team is working on improving it as one of our current priorities, + but any improvement we come up with must address the needs of a vast spectrum of applications, + as well as be heavily scaled-tested in various scenarios + (in an attempt to cover the applications' wide spectrum) + * a mempool designed specifically for one particular application + would reduce the search space as its designers can devise it with just their application's + needs in mind. +* The interaction with the application is still somewhat convoluted: + * the application has to decide what logic to implement in `CheckTx`, + what to do with the transaction list coming in `RequestPrepareProposal`, + whether it wants to maintain an app-side mempool (more on this below), and whether or not + to combine the transactions in the app-side mempool with those coming in `RequestPrepareProposal` + * all those combinations are hard to fully understand, as the semantics and guarantees are + often not clear + * when using exclusively an app-mempool (the approach taken in the Cosmos SDK `v0.47.x`) + for populating proposed blocks, with the aim of simplifying the app developers' life, + we still have a suboptimal model where we need to continue using CometBFT's mempool + in order to disseminate the transactions. So, we end up using twice as much memory, + as in-transit transactions need to be kept in both mempools. + +The approach presented in this ADR builds on the app-mempool design released in `v0.47.x` +of the Cosmos SDK, +and briefly discussed in the last bullet point above (see [SDK app-mempool][sdk-app-mempool] for further details of this model). + +In the app-mempool design in Cosmos SDK `v0.47.x` +an unconfirmed transaction must be both in CometBFT's mempool for dissemination and +in the app's mempool so the application can decide how to manage the mempool. +There is no doubt that this approach has numerous advantages. However, it also has some implications that need to be considered: + +* Having every transaction both in CometBFT and in the application is suboptimal in terms of memory. + Additionally, the app developer has to be careful + that the contents of both mempools do not diverge over time + (hence the crucial role `re-CheckTx` plays post-ABCI++). +* The main reason for a transaction needing to be in CometBFT's mempool is + because the design in Cosmos SDK `v0.47.x` does not consider an application + that has its own means of disseminating transactions. + It reuses the peer to peer network underneath CometBFT reactors. +* There is no point in having transactions in CometBFT's mempool if an application implements an ad-hoc design for disseminating transactions. + +This proposal targets this kind of applications: +those that have an ad-hoc mechanism for transaction dissemination that better meets the application requirements. + +The ABCI application could reuse the P2P network once this is exposed via ABCI. +But this will take some time as it needs to be implemented, and has a dependency +on bi-directional ABCI, which is also quite substantial. See +[1](https://github.com/cometbft/cometbft/discussions/1112) and +[2](https://github.com/cometbft/cometbft/discussions/494) discussions. + +We propose to introduce a `nop` (short for no operation) mempool which will effectively act as a stubbed object +internally: + +* it will reject any transaction being locally submitted or gossipped by a peer +* when a _reap_ (as it is currently called) is executed in the mempool, an empty answer will always be returned +* the application running on the proposer validator will add transactions it received + using the appchains's own mechanism via `PrepareProposal`. + +## Alternative Approaches + +These are the alternatives known to date: + +1. Keep the current model. Useful for basic apps, but clearly suboptimal for applications + with their own mechanism to disseminate transactions and particular performance requirements. +2. Provide more efficient general-purpose mempool implementations. + This is an ongoing effort (e.g., [CAT mempool][cat-mempool]), but will take some time, and R&D effort, to come up with + advanced mechanisms -- likely highly configurable and thus complex -- which then will have to be thoroughly tested. +3. A similar approach to this one ([ADR110][adr-110]) whereby the application-specific + mechanism directly interacts with CometBFT via a newly defined gRPC interface. +4. Partially adopting this ADR. There are several possibilities: + * Use the current mempool, disable transaction broadcast in `config.toml`, and accept transactions from users via `BroadcastTX*` RPC methods. + Positive: avoids transaction gossiping; app can reuse the mempool existing in ComeBFT. + Negative: requires clients to know the validators' RPC endpoints (potential security issues). + * Transaction broadcast is disabled in `config.toml`, and have the application always reject transactions in `CheckTx`. + Positive: effectively disables the mempool; does not require modifications to Comet (may be used in `v0.37.x` and `v0.38.x`). + Negative: requires apps to disseminate txs themselves; the setup for this is less straightforward than this ADR's proposal. + +## Decision + +TBD + +## Detailed Design + +What this ADR proposes can already be achieved with an unmodified CometBFT since +`v0.37.x`, albeit with a complex, poor UX (see the last alternative in section +[Alternative Approaches](#alternative-approaches)). The core of this proposal +is to make some internal changes so it is clear an simple for app developers, +thus improving the UX. + +#### `nop` Mempool + +We propose a new mempool implementation, called `nop` Mempool, that effectively disables all mempool functionality +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 `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 `nil` +* `EnableTxsAvailable()`: does nothing +* `SetTxRemovedCallback(cb func(types.TxKey))`: does nothing +* `Size() int` returns 0 +* `SizeBytes() int64` returns 0 + +Upon startup, the `nop` mempool reactor will advertise no channels to the peer-to-peer layer. + +### Configuration + +We propose the following changes to the `config.toml` file: + +```toml +[mempool] +# The type of mempool for this CometBFT node to use. +# +# Valid types of mempools supported by CometBFT: +# - "flood" : clist mempool with flooding gossip protocol (default) +# - "nop" : nop-mempool (app has implemented an alternative tx dissemination mechanism) +type = "nop" +``` + +The config validation logic will be modified to add a new rule that rejects a configuration file +if all of these conditions are met: + +* the mempool is set to `nop` +* `create_empty_blocks`, in `consensus` section, is set to `false`. + +The reason for this extra validity rule is that the `nop`-mempool, as proposed here, +does not support the "do not create empty blocks" functionality. +Here are some considerations on this: + +* The "do not create empty blocks" functionality + * entangles the consensus and mempool reactors + * is hardly used in production by real appchains (to the best of CometBFT team's knowledge) + * its current implementation for the built-in mempool has undesired side-effects + * app hashes currently refer to the previous block, + * and thus it interferes with query provability. +* If needed in the future, this can be supported by extending ABCI, + but we will first need to see a real need for this before committing to changing ABCI + (which has other, higher-impact changes waiting to be prioritized). + +### RPC Calls + +There are no changes needed in the code dealing with RPC. Those RPC paths that call methods of the `Mempool` interface, +will simply be calling the new implementation. + +### Impacted Workflows + +* *Submitting a transaction*. Users are not to submit transactions via CometBFT's RPC. + `BroadcastTx*` RPC methods will fail with a reasonable error and the 501 status code. + The application running on a full node must offer an interface for users to submit new transactions. + It could also be a distinct node (or set of nodes) in the network. + 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 `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 + +The changes needed for this approach, are fairly simple, and the logic is clear. +This might allow us to even deliver it as part of CometBFT `v1` (our next release) +even without a noticeable impact on `v1`'s delivery schedule. + +The CometBFT team (learning from past dramatic events) usually takes a conservative approach +for backporting changes to release branches that have already undergone a full QA cycle +(and thus are in code-freeze mode). +For this reason, although the limited impact of these changes would limit the risks +of backporting to `v0.38.x` and `v0.37.x`, a careful risk/benefit evaluation will +have to be carried out. + +Backporting to `v0.34.x` does not make sense as this version predates the release of `ABCI 1.0`, +so using the `nop` mempool renders CometBFT's operation useless. + +### Config parameter _vs._ application-enforced parameter + +In the current proposal, the parameter selecting the mempool is in `config.toml`. +However, it is not a clear-cut decision. These are the alternatives we see: + +* *Mempool selected in `config.toml` (our current design)*. + This is the way the mempool has always been selected in Tendermint Core and CometBFT, + 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 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. + The Cosmos community may not be ready for such a rigid, radical change, + even if it eliminates the risk of operators shooting themselves in the foot. + Hence we went currently favor the previous alternative. +* *Mempool selected as a network-wide parameter, but allowing override*. + A third option, half way between the previous two, is to have the mempool selection + as a network-wide parameter, but with a special value called _local-config_ that still + allows an appchain to decide to leave it up to operators to configure it in `config.toml`. + +Ultimately, the "config parameter _vs._ application-enforced parameter" discussion +is a more general one that is applicable to other parameters not related to mempool selection. +In that sense, it is out of the scope of this ADR. + +## Consequences + +### Positive + +- Applications can now find mempool mechanisms that fit better their particular needs: + - Ad-hoc ways to add, remove, merge, reorder, modify, prioritize transactions according + to application needs. + - A way to disseminate transactions (gossip-based or other) to get the submitted transactions + to proposers. The application developers can devise simpler, efficient mechanisms tailored + to their application. + - Back-pressure mechanisms to prevent malicious users from abusing the transaction + dissemination mechanism. +- In this approach, CometBFT's peer-to-peer layer is relieved from managing transaction gossip, freeing up its resources for other reactors such as consensus, evidence, block-sync, or state-sync. +- There is no risk for the operators of a network to provide inconsistent configurations + for some mempool-related parameters. Some of those misconfigurations are known to have caused + serious performance issues in CometBFT's peer to peer network. + Unless, of course, the application-defined transaction dissemination mechanism ends up + allowing similar configuration inconsistencies. +- The interaction between the application and CometBFT at `PrepareProposal` time + is simplified. No transactions are ever provided by CometBFT, + and no transactions can ever be left in the mempool when CometBFT calls `PrepareProposal`: + the application trivially has all the information. +- UX is improved compared to how this can be done prior to this ADR. + +### Negative + +- With the `nop` mempool, it is up to the application to provide users with a way + to submit transactions and deliver those transactions to validators. + This is a considerable endeavor, and more basic appchains may consider it is not worth the hassle. +- There is a risk of wasting resources by those nodes that have a misconfigured + mempool (bandwidth, CPU, memory, etc). If there are TXs submitted (incorrectly) + via CometBFT's RPC, but those TXs are never submitted (correctly via an + app-specific interface) to the App. As those TXs risk being there until the node + is stopped. Moreover, those TXs will be replied & proposed every single block. + App developers will need to keep this in mind and panic on `CheckTx` or + `PrepareProposal` with non-empty list of transactions. +- Optimizing block proposals by only including transaction IDs (e.g. TX hashes) is more difficult. + The ABCI app could do it by submitting TX hashes (rather than TXs themselves) + in `PrepareProposal`, and then having a mechanism for pulling TXs from the + network upon `FinalizeBlock`. + +[sdk-app-mempool]: https://docs.cosmos.network/v0.47/build/building-apps/app-mempool +[adr-110]: https://github.com/cometbft/cometbft/pull/1565 +[HT94]: https://dl.acm.org/doi/book/10.5555/866693 +[cat-mempool]: https://github.com/cometbft/cometbft/pull/1472 \ No newline at end of file diff --git a/docs/core/configuration.md b/docs/core/configuration.md index cf88034d91c..2f9f011683e 100644 --- a/docs/core/configuration.md +++ b/docs/core/configuration.md @@ -294,6 +294,16 @@ dial_timeout = "3s" # 2) "v1" - prioritized mempool (deprecated; will be removed in the next release). version = "v0" +# 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 = true broadcast = true wal_dir = "" @@ -487,6 +497,7 @@ namespace = "cometbft" ``` ## Empty blocks VS no empty blocks + ### create_empty_blocks = true If `create_empty_blocks` is set to `true` in your config, blocks will be created ~ every second (with default consensus parameters). You can regulate the delay between blocks by changing the `timeout_commit`. E.g. `timeout_commit = "10s"` should result in ~ 10 second blocks. @@ -500,6 +511,7 @@ Note after the block H, CometBFT creates something we call a "proof block" (only Plus, if you set `create_empty_blocks_interval` to something other than the default (`0`), CometBFT will be creating empty blocks even in the absence of transactions every `create_empty_blocks_interval.` For instance, with `create_empty_blocks = false` and `create_empty_blocks_interval = "30s"`, CometBFT will only create blocks if there are transactions, or after waiting 30 seconds without receiving any transactions. ## Consensus timeouts explained + There's a variety of information about timeouts in [Running in production](./running-in-production.md#configuration-parameters). You can also find more detailed explanation in the paper describing @@ -518,6 +530,7 @@ timeout_precommit = "1s" timeout_precommit_delta = "500ms" timeout_commit = "1s" ``` + Note that in a successful round, the only timeout that we absolutely wait no matter what is `timeout_commit`. Here's a brief summary of the timeouts: diff --git a/docs/core/mempool.md b/docs/core/mempool.md index 8dd96878196..f86083ee04d 100644 --- 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 \ No newline at end of file diff --git a/mempool/nop_mempool.go b/mempool/nop_mempool.go new file mode 100644 index 00000000000..4a55a620d8d --- /dev/null +++ b/mempool/nop_mempool.go @@ -0,0 +1,110 @@ +package mempool + +import ( + "errors" + + abci "github.com/cometbft/cometbft/abci/types" + "github.com/cometbft/cometbft/libs/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, func(*abci.Response), TxInfo) error { + return 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.ResponseDeliverTx, + 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) ReceiveEnvelope(p2p.Envelope) {} + +// SetSwitch does nothing. +func (*NopMempoolReactor) SetSwitch(*p2p.Switch) {} diff --git a/mempool/nop_mempool_test.go b/mempool/nop_mempool_test.go new file mode 100644 index 00000000000..01b169e0695 --- /dev/null +++ b/mempool/nop_mempool_test.go @@ -0,0 +1,38 @@ +package mempool + +import ( + "testing" + + "github.com/cometbft/cometbft/types" + "github.com/stretchr/testify/assert" +) + +var tx = types.Tx([]byte{0x01}) + +func TestNopMempool_Basic(t *testing.T) { + mem := &NopMempool{} + + assert.Equal(t, 0, mem.Size()) + assert.Equal(t, int64(0), mem.SizeBytes()) + + err := mem.CheckTx(tx, nil, TxInfo{}) + assert.Equal(t, errNotAllowed, err) + + err = mem.RemoveTxByKey(tx.Key()) + assert.Equal(t, errNotAllowed, err) + + txs := mem.ReapMaxBytesMaxGas(0, 0) + assert.Nil(t, txs) + + txs = mem.ReapMaxTxs(0) + assert.Nil(t, txs) + + err = mem.FlushAppConn() + assert.NoError(t, err) + + err = mem.Update(0, nil, nil, nil, nil) + assert.NoError(t, err) + + txsAvailable := mem.TxsAvailable() + assert.Nil(t, txsAvailable) +} diff --git a/node/node.go b/node/node.go index 868bd8ea008..636a9d968f4 100644 --- a/node/node.go +++ b/node/node.go @@ -494,52 +494,62 @@ func createMempoolAndMempoolReactor( memplMetrics *mempl.Metrics, logger log.Logger, ) (mempl.Mempool, p2p.Reactor) { - switch config.Mempool.Version { - case cfg.MempoolV1: - mp := mempoolv1.NewTxMempool( - logger, - config.Mempool, - proxyApp.Mempool(), - state.LastBlockHeight, - mempoolv1.WithMetrics(memplMetrics), - mempoolv1.WithPreCheck(sm.TxPreCheck(state)), - mempoolv1.WithPostCheck(sm.TxPostCheck(state)), - ) - - reactor := mempoolv1.NewReactor( - config.Mempool, - mp, - ) - if config.Consensus.WaitForTxs() { - mp.EnableTxsAvailable() - } - - return mp, reactor + switch config.Mempool.Type { + // allow empty string for backward compatibility + case cfg.MempoolTypeFlood, "": + switch config.Mempool.Version { + case cfg.MempoolV1: + mp := mempoolv1.NewTxMempool( + logger, + config.Mempool, + proxyApp.Mempool(), + state.LastBlockHeight, + mempoolv1.WithMetrics(memplMetrics), + mempoolv1.WithPreCheck(sm.TxPreCheck(state)), + mempoolv1.WithPostCheck(sm.TxPostCheck(state)), + ) + + reactor := mempoolv1.NewReactor( + config.Mempool, + mp, + ) + if config.Consensus.WaitForTxs() { + mp.EnableTxsAvailable() + } - case cfg.MempoolV0: - mp := mempoolv0.NewCListMempool( - config.Mempool, - proxyApp.Mempool(), - state.LastBlockHeight, - mempoolv0.WithMetrics(memplMetrics), - mempoolv0.WithPreCheck(sm.TxPreCheck(state)), - mempoolv0.WithPostCheck(sm.TxPostCheck(state)), - ) + return mp, reactor + + case cfg.MempoolV0: + mp := mempoolv0.NewCListMempool( + config.Mempool, + proxyApp.Mempool(), + state.LastBlockHeight, + mempoolv0.WithMetrics(memplMetrics), + mempoolv0.WithPreCheck(sm.TxPreCheck(state)), + mempoolv0.WithPostCheck(sm.TxPostCheck(state)), + ) + + mp.SetLogger(logger) + + reactor := mempoolv0.NewReactor( + config.Mempool, + mp, + ) + if config.Consensus.WaitForTxs() { + mp.EnableTxsAvailable() + } - mp.SetLogger(logger) + return mp, reactor - reactor := mempoolv0.NewReactor( - config.Mempool, - mp, - ) - if config.Consensus.WaitForTxs() { - mp.EnableTxsAvailable() + default: + return nil, nil } - - return mp, reactor - + case cfg.MempoolTypeNop: + // Strictly speaking, there's no need to have a `mempl.NopMempoolReactor`, but + // adding it leads to a cleaner code. + return &mempl.NopMempool{}, mempl.NewNopMempoolReactor() default: - return nil, nil + panic(fmt.Sprintf("unknown mempool type: %q", config.Mempool.Type)) } } @@ -705,7 +715,9 @@ func createSwitch(config *cfg.Config, p2p.SwitchPeerFilters(peerFilters...), ) sw.SetLogger(p2pLogger) - sw.AddReactor("MEMPOOL", mempoolReactor) + if config.Mempool.Type != cfg.MempoolTypeNop { + sw.AddReactor("MEMPOOL", mempoolReactor) + } sw.AddReactor("BLOCKCHAIN", bcReactor) sw.AddReactor("CONSENSUS", consensusReactor) sw.AddReactor("EVIDENCE", evidenceReactor) @@ -933,11 +945,10 @@ func NewNodeWithContext(ctx context.Context, logNodeStartupInfo(state, pubKey, logger, consensusLogger) - // Make MempoolReactor mempool, mempoolReactor := createMempoolAndMempoolReactor(config, proxyApp, state, memplMetrics, logger) - // Make Evidence Reactor evidenceReactor, evidencePool, err := createEvidenceReactor(config, dbProvider, stateDB, blockStore, logger) + if err != nil { return nil, err } @@ -971,6 +982,7 @@ func NewNodeWithContext(ctx context.Context, } else if blockSync { csMetrics.BlockSyncing.Set(1) } + consensusReactor, consensusState := createConsensusReactor( config, state, blockExec, blockStore, mempool, evidencePool, privValidator, csMetrics, stateSync || blockSync, eventBus, consensusLogger, @@ -997,10 +1009,8 @@ func NewNodeWithContext(ctx context.Context, return nil, err } - // Setup Transport. transport, peerFilters := createTransport(config, nodeInfo, nodeKey, proxyApp) - // Setup Switch. p2pLogger := logger.With("module", "p2p") sw := createSwitch( config, transport, p2pMetrics, peerFilters, mempoolReactor, bcReactor,