Mempool cluster limits

CHANGELOG.md

@@ -22,6 +22,11 @@ The format is loosely based on [Keep a Changelog](https://keepachangelog.com/en/
     - `wallet-create`/`wallet-recover`/`wallet-open` support the `ledger` subcommand, in addition to the existing
       `software` and `trezor`, which specifies the type of the wallet to operate on.
 
+  - Node:
+    - New options to control the newly introduced mempool cluster limits: `--mempool-max-cluster-transaction-count`
+      and `--mempool-max-cluster-size-bytes`.
+    - New RPC method `mempool_get_config`, which returns the current mempool configuration.
+
 ### Changed
   - P2p:
     - Various improvements to transaction announcement:
@@ -33,6 +38,10 @@ The format is loosely based on [Keep a Changelog](https://keepachangelog.com/en/
 
   - Mempool:
     - Various optimizations were made.
+    - There are now limits on how big a single mempool transaction cluster can be (a cluster is a set of unconfirmed
+      transactions that depend on each other); the mempool will reject transactions that would result in the formation
+      of a cluster that exceeds the limits. The default cluster limits are: max 64 transactions, max 100'000 bytes
+      in total.
 
 ### Fixed
   - Wallet:

blockprod/src/lib.rs

@@ -327,7 +327,8 @@ mod tests {
             mempool_config,
             subsystem::Handle::clone(&chainstate),
             time_getter.clone(),
-        );
+        )
+        .unwrap();
         let mempool = manager.add_custom_subsystem("mempool", |hdl, _| mempool_init.init(hdl));
 
         let mut p2p_config = test_p2p_config();

chainstate/test-framework/src/helpers.rs

@@ -30,7 +30,10 @@ use orders_accounting::{OrdersAccountingDB, OrdersAccountingView as _};
 use randomness::{CryptoRng, Rng, RngExt as _, SliceRandom as _};
 use test_utils::{random_ascii_alphanumeric_string, token_utils::random_nft_issuance};
 
-use crate::{TestFramework, TransactionBuilder, empty_witness, get_output_value};
+use crate::{
+    TestFramework, TransactionBuilder, get_output_value,
+    utils::{EMPTY_WITNESS_DEFAULT_SIZE, empty_witness_with_size},
+};
 
 // Note: this function will create 2 blocks
 pub fn issue_and_mint_random_token_from_best_block(
@@ -396,15 +399,34 @@ pub fn output_value_with_amount(output_value: &OutputValue, new_amount: Amount)
 
 pub fn make_simple_coin_tx(
     rng: &mut impl CryptoRng,
-    ins: &[(OutPointSourceId, u32)],
-    outs: &[u128],
+    ins: impl IntoIterator<Item = (OutPointSourceId, u32)>,
+    outs: impl IntoIterator<Item = u128>,
 ) -> SignedTransaction {
-    let builder = ins.iter().fold(TransactionBuilder::new(), |b, (s, n)| {
-        b.add_input(TxInput::from_utxo(s.clone(), *n), empty_witness(rng))
-    });
-    let builder = outs.iter().fold(builder, |b, a| {
-        b.add_output(TxOutput::Transfer(
-            OutputValue::Coin(Amount::from_atoms(*a)),
+    make_simple_coin_tx_with_witness_sizes(
+        rng,
+        ins.into_iter()
+            .map(|(utxo_src, utxo_idx)| (utxo_src, utxo_idx, EMPTY_WITNESS_DEFAULT_SIZE)),
+        outs,
+    )
+}
+
+pub fn make_simple_coin_tx_with_witness_sizes(
+    rng: &mut impl CryptoRng,
+    ins: impl IntoIterator<Item = (OutPointSourceId, u32, usize)>,
+    outs: impl IntoIterator<Item = u128>,
+) -> SignedTransaction {
+    let builder = ins.into_iter().fold(
+        TransactionBuilder::new(),
+        |tx_builder, (utxo_src, utxo_idx, witness_size)| {
+            tx_builder.add_input(
+                TxInput::from_utxo(utxo_src.clone(), utxo_idx),
+                empty_witness_with_size(rng, witness_size),
+            )
+        },
+    );
+    let builder = outs.into_iter().fold(builder, |tx_builder, output_amount| {
+        tx_builder.add_output(TxOutput::Transfer(
+            OutputValue::Coin(Amount::from_atoms(output_amount)),
             Destination::AnyoneCanSpend,
         ))
     });

chainstate/test-framework/src/transaction_builder.rs

@@ -72,6 +72,13 @@ impl TransactionBuilder {
         self
     }
 
+    pub fn add_output_n_times(mut self, n: usize, output: &TxOutput) -> Self {
+        for _ in 0..n {
+            self = self.add_output(output.clone())
+        }
+        self
+    }
+
     pub fn outputs(&self) -> &[TxOutput] {
         &self.outputs
     }

chainstate/test-framework/src/utils.rs

@@ -57,12 +57,19 @@ use crypto::{
 use orders_accounting::OrdersAccountingView;
 use pos_accounting::{PoSAccountingDB, PoSAccountingView};
 use randomness::{CryptoRng, Rng, RngExt as _};
+use test_utils::random::gen_random_bytes;
 use utxo::UtxosView;
 
+// Note: 99 is because originally `empty_witness` returned a randomly shuffled vec of numbers (0..100)
+// and now some tests that check transaction sizes depend on this.
+pub const EMPTY_WITNESS_DEFAULT_SIZE: usize = 99;
+
 pub fn empty_witness(rng: &mut impl Rng) -> InputWitness {
-    use randomness::SliceRandom;
-    let mut msg: Vec<u8> = (1..100).collect();
-    msg.shuffle(rng);
+    empty_witness_with_size(rng, EMPTY_WITNESS_DEFAULT_SIZE)
+}
+
+pub fn empty_witness_with_size(rng: &mut impl Rng, size: usize) -> InputWitness {
+    let msg = gen_random_bytes(rng, size, size);
     InputWitness::NoSignature(Some(msg))
 }
 

common/src/chain/config/mod.rs

@@ -522,7 +522,11 @@ impl ChainConfig {
         self.max_block_size_with_smart_contracts
     }
 
-    /// The maximum size of any transaction submitted to the node for the mempool
+    /// The approximate maximum size of any transaction submitted to the node for the mempool.
+    ///
+    /// Note that this is just an upper limit - creating a tx bigger than this doesn't make much
+    /// sense, because it won't fit into a block. Normally, the cluster size limit (specified in
+    /// the mempool config) will be the actual limit.
     pub fn max_tx_size_for_mempool(&self) -> usize {
         // Reserve some space in the block for the data it needs to store beyond the transaction
         // data itself, namely the transaction count due to how sequences of elements are encoded.

mempool/Cargo.toml

@@ -48,5 +48,6 @@ crypto = { path = "../crypto" }
 test-utils = { path = "../test-utils" }
 
 ctor.workspace = true
+itertools.workspace = true
 mockall.workspace = true
 rstest.workspace = true

mempool/src/config.rs

@@ -17,7 +17,7 @@ use std::{str::FromStr, time::Duration};
 
 use common::primitives::{Amount, BlockDistance};
 use rpc::description::HasValueHint;
-use utils::make_config_setting;
+use utils::{ensure, make_config_setting};
 
 use crate::FeeRate;
 
@@ -141,13 +141,86 @@ make_config_setting!(
     FeeRate::from_amount_per_kb(Amount::from_atoms(100_000_000_000))
 );
 
+make_config_setting!(MaxClusterTxCount, usize, 64);
+make_config_setting!(MaxClusterSizeBytes, usize, 100_000);
+
 #[derive(Debug, Clone, Default)]
 pub struct MempoolConfig {
+    /// Minimum transaction relay fee rate (in atoms per 1000 bytes).
     pub min_tx_relay_fee_rate: MinTxRelayFeeRate,
+
+    /// Maximum number of transactions that a single cluster can contain.
+    pub max_cluster_tx_count: MaxClusterTxCount,
+
+    /// Maximum total size of transactions that is allowed in a single cluster.
+    pub max_cluster_size_bytes: MaxClusterSizeBytes,
 }
 
 impl MempoolConfig {
     pub fn new() -> Self {
         Self::default()
     }
+
+    pub fn to_rpc_type(&self) -> RpcMempoolConfig {
+        let MempoolConfig {
+            min_tx_relay_fee_rate,
+            max_cluster_tx_count,
+            max_cluster_size_bytes,
+        } = self;
+
+        RpcMempoolConfig {
+            min_tx_relay_fee_rate: **min_tx_relay_fee_rate,
+            max_cluster_tx_count: **max_cluster_tx_count,
+            max_cluster_size_bytes: **max_cluster_size_bytes,
+        }
+    }
+
+    pub fn validate(&self) -> Result<(), ConfigError> {
+        // Note: setting these values to 0 makes no sense (unless the goal is to prevent
+        // any tx from entering the mempool). Also, `max_cluster_tx_count=0` will behave
+        // the same way as `max_cluster_tx_count=1`, because parentless txs currently
+        // skip the cluster tx count check. So we just forbid passing zeros here.
+        ensure!(
+            *self.max_cluster_tx_count > 0,
+            ConfigError::MaxClusterTxCountCannotBeZero
+        );
+        ensure!(
+            *self.max_cluster_size_bytes > 0,
+            ConfigError::MaxClusterSizeBytesCannotBeZero
+        );
+        Ok(())
+    }
+}
+
+impl From<RpcMempoolConfig> for MempoolConfig {
+    fn from(value: RpcMempoolConfig) -> Self {
+        let RpcMempoolConfig {
+            min_tx_relay_fee_rate,
+            max_cluster_tx_count,
+            max_cluster_size_bytes,
+        } = value;
+
+        Self {
+            min_tx_relay_fee_rate: min_tx_relay_fee_rate.into(),
+            max_cluster_tx_count: max_cluster_tx_count.into(),
+            max_cluster_size_bytes: max_cluster_size_bytes.into(),
+        }
+    }
+}
+
+/// Same as MempoolConfig but for usage in RPC.
+#[derive(Debug, Clone, serde::Serialize, serde::Deserialize, rpc::description::HasValueHint)]
+pub struct RpcMempoolConfig {
+    pub min_tx_relay_fee_rate: FeeRate,
+    pub max_cluster_tx_count: usize,
+    pub max_cluster_size_bytes: usize,
+}
+
+#[derive(Debug, Clone, thiserror::Error, PartialEq, Eq)]
+pub enum ConfigError {
+    #[error("Maximum cluster transaction count cannot be zero")]
+    MaxClusterTxCountCannotBeZero,
+
+    #[error("Maximum cluster size in bytes cannot be zero")]
+    MaxClusterSizeBytesCannotBeZero,
 }

mempool/src/error/ban_score.rs

@@ -24,7 +24,10 @@ use chainstate::{
 };
 use common::chain::IdCreationError;
 
-use crate::error::{Error, MempoolPolicyError, ReorgError, TxCollectionError, TxValidationError};
+use crate::error::{
+    Error, MempoolPolicyError, MempoolStoreError, MempoolStoreInvariantError, ReorgError,
+    TxCollectionError, TxValidationError,
+};
 
 /// Ban score for transactions
 pub trait MempoolBanScore {
@@ -50,6 +53,9 @@ impl MempoolBanScore for Error {
 
             // Internal error
             Error::SubsystemCallError(_) => 0,
+
+            // A configuration error is not peer's fault.
+            Error::ConfigError(_) => 0,
         }
     }
 }
@@ -60,9 +66,13 @@ impl MempoolBanScore for MempoolPolicyError {
             // Basic transaction integrity checks failed, ban peer.
             MempoolPolicyError::NoInputs => 100,
             MempoolPolicyError::NoOutputs => 100,
-            MempoolPolicyError::ExceedsMaxBlockSize => 100,
+            MempoolPolicyError::TxSizeExceedsMaxBlockSize => 100,
             MempoolPolicyError::RelayFeeOverflow => 100,
 
+            // Don't punish the peer for this, because max cluster size is technically configurable
+            // and the peer doesn't know the node's mempool configuration.
+            MempoolPolicyError::TxSizeExceedsMaxClusterSize { .. } => 0,
+
             // Errors to do with transaction conflicts and replacements are not punished since the
             // peer may not be aware of all the transactions the node has in the mempool.
             // This could be refined later.
@@ -73,6 +83,11 @@ impl MempoolBanScore for MempoolPolicyError {
             MempoolPolicyError::ReplacementFeeLowerThanOriginal { .. } => 0,
             MempoolPolicyError::AdditionalFeesUnderflow => 0,
 
+            // Same as above, cluster size violations are not punished because the peer may not
+            // be aware of the node's mempool contents.
+            MempoolPolicyError::ClusterMaxTxCountLimitViolated { .. }
+            | MempoolPolicyError::ClusterTotalTxSizeLimitViolated { .. } => 0,
+
             // Sending transactions with a fee below the minimum should not be punished.
             MempoolPolicyError::InsufficientFeesToRelay { .. } => 0,
             MempoolPolicyError::InsufficientFeesToRelayRBF => 0,
@@ -85,8 +100,28 @@ impl MempoolBanScore for MempoolPolicyError {
             MempoolPolicyError::AncestorFeeOverflow => 0,
             MempoolPolicyError::AncestorFeeUpdateOverflow => 0,
             MempoolPolicyError::FeeOverflow => 0,
-            MempoolPolicyError::GetParentError => 0,
             MempoolPolicyError::DescendantOfExpiredTransaction => 0,
+
+            MempoolPolicyError::MempoolStoreInvariantError(err) => err.mempool_ban_score(),
+            MempoolPolicyError::MempoolStoreError(err) => err.mempool_ban_score(),
+        }
+    }
+}
+
+impl MempoolBanScore for MempoolStoreInvariantError {
+    fn mempool_ban_score(&self) -> u32 {
+        match self {
+            // This whole enum is supposed to contain only invariant violations, but just in case
+            // we explicitly list its variants here.
+            MempoolStoreInvariantError::SupposedlyExistingEntryNotFound(_) => 0,
+        }
+    }
+}
+
+impl MempoolBanScore for MempoolStoreError {
+    fn mempool_ban_score(&self) -> u32 {
+        match self {
+            MempoolStoreError::TxEntryNotFound(_) => 0,
         }
     }
 }
@@ -141,12 +176,8 @@ impl MempoolBanScore for ReorgError {
 impl MempoolBanScore for TxCollectionError {
     fn mempool_ban_score(&self) -> u32 {
         match self {
-            // This represents a function contract violation by the caller code.
-            TxCollectionError::SpecifiedTxNotFound(_) => 0,
-
-            // These 2 are mempool invariant errors.
-            TxCollectionError::TxParentNotFound { .. }
-            | TxCollectionError::TxChildNotFound { .. } => 0,
+            TxCollectionError::MempoolStoreInvariantError(err) => err.mempool_ban_score(),
+            TxCollectionError::MempoolStoreError(err) => err.mempool_ban_score(),
         }
     }
 }