change(mempool): Evict transactions from the mempool using the ZIP-317 conventional fee (#5703)

* Add a ZIP-317 conventional fee module

* Add a conventional fee calculation stub, and use it for mempool size limiting

* Just return a usize from zcash_serialized_size(), removing the unused Result

* Add ZIP-317 constants

* Calculate the ZIP-317 conventional fee

* Update tests

* Add a CHANGELOG entry

* Fix a comment typo

Co-authored-by: Daira Hopwood <daira@jacaranda.org>

* Fix some missing words in a comment

Co-authored-by: Arya <aryasolhi@gmail.com>

Co-authored-by: Daira Hopwood <daira@jacaranda.org>
Co-authored-by: Arya <aryasolhi@gmail.com>

CHANGELOG.md

@@ -6,18 +6,26 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 
 ## [Zebra 1.0.0-rc.2](https://github.com/ZcashFoundation/zebra/releases/tag/v1.0.0-rc.2) - 2022-11-TODO
 
-Zebra's latest release continues work on mining pool RPCs, and fixes a rare RPC crash that could lead to memory corruption.
+Zebra's latest release continues work on mining pool RPCs, fixes a rare RPC crash that could lead to memory corruption, and uses the ZIP-317 conventional fee for mempool size limits.
 
 Zebra's consensus rules, node sync, and `lightwalletd` RPCs are ready for user testing and experimental use. Zebra has not been audited yet.
 
 ### Breaking Changes
 
 This release has the following breaking changes:
+- Evict transactions from the mempool using the ZIP-317 conventional fee ([#5703](https://github.com/ZcashFoundation/zebra/pull/5703))
+  - If there are a lot of unmined transactions on the Zcash network, and Zebra's mempool
+    becomes full, Zebra will penalise transactions that don't pay at least the ZIP-317
+    conventional fee. These transactions will be more likely to get evicted.
+  - The ZIP-317 convention fee increases based on the number of logical transparent or
+    shielded actions in a transaction.
+  - This change has no impact under normal network conditions.
 - TODO: search the changelog for breaking changes
 
 ### Security
 
 - Fix a rare crash and memory errors when Zebra's RPC server shuts down ([#5591](https://github.com/ZcashFoundation/zebra/pull/5591))
+- Evict transactions from the mempool using the ZIP-317 conventional fee ([#5703](https://github.com/ZcashFoundation/zebra/pull/5703))
 
 TODO: the rest of the changelog
 

zebra-chain/src/serialization/tests/prop.rs

@@ -1,6 +1,6 @@
 //! Property-based tests for basic serialization primitives.
 
-use std::{convert::TryFrom, io::Cursor};
+use std::io::Cursor;
 
 use proptest::prelude::*;
 
@@ -110,6 +110,6 @@ proptest! {
     fn transaction_serialized_size(transaction in any::<UnminedTx>()) {
         let _init_guard = zebra_test::init();
 
-        prop_assert_eq!(transaction.transaction.zcash_serialized_size().unwrap(), transaction.size);
+        prop_assert_eq!(transaction.transaction.zcash_serialized_size(), transaction.size);
     }
 }

zebra-chain/src/serialization/zcash_serialize.rs

@@ -37,11 +37,11 @@ pub trait ZcashSerialize: Sized {
     }
 
     /// Get the size of `self` by using a fake writer.
-    fn zcash_serialized_size(&self) -> Result<usize, io::Error> {
+    fn zcash_serialized_size(&self) -> usize {
         let mut writer = FakeWriter(0);
         self.zcash_serialize(&mut writer)
             .expect("writer should never fail");
-        Ok(writer.0)
+        writer.0
     }
 }
 

zebra-chain/src/transaction/unmined.rs

@@ -17,9 +17,6 @@
 
 use std::{fmt, sync::Arc};
 
-#[cfg(any(test, feature = "proptest-impl"))]
-use proptest_derive::Arbitrary;
-
 use crate::{
     amount::{Amount, NonNegative},
     serialization::ZcashSerialize,
@@ -32,6 +29,11 @@ use crate::{
 
 use UnminedTxId::*;
 
+#[cfg(any(test, feature = "proptest-impl"))]
+use proptest_derive::Arbitrary;
+
+mod zip317;
+
 /// The minimum cost value for a transaction in the mempool.
 ///
 /// Contributes to the randomized, weighted eviction of transactions from the
@@ -50,6 +52,12 @@ use UnminedTxId::*;
 /// [ZIP-401]: https://zips.z.cash/zip-0401
 const MEMPOOL_TRANSACTION_COST_THRESHOLD: u64 = 4000;
 
+/// When a transaction pays a fee less than the conventional fee,
+/// this low fee penalty is added to its cost for mempool eviction.
+///
+/// See [VerifiedUnminedTx::eviction_weight()] for details.
+const MEMPOOL_TRANSACTION_LOW_FEE_PENALTY: u64 = 16_000;
+
 /// A unique identifier for an unmined transaction, regardless of version.
 ///
 /// "The transaction ID of a version 4 or earlier transaction is the SHA-256d hash
@@ -201,6 +209,11 @@ pub struct UnminedTx {
 
     /// The size in bytes of the serialized transaction data
     pub size: usize,
+
+    /// The conventional fee for this transaction, as defined by [ZIP-317].
+    ///
+    /// [ZIP-317]: https://zips.z.cash/zip-0317#fee-calculation
+    pub conventional_fee: Amount<NonNegative>,
 }
 
 impl fmt::Display for UnminedTx {
@@ -208,6 +221,7 @@ impl fmt::Display for UnminedTx {
         f.debug_struct("UnminedTx")
             .field("transaction", &self.transaction)
             .field("serialized_size", &self.size)
+            .field("conventional_fee", &self.conventional_fee)
             .finish()
     }
 }
@@ -217,15 +231,15 @@ impl fmt::Display for UnminedTx {
 
 impl From<Transaction> for UnminedTx {
     fn from(transaction: Transaction) -> Self {
-        let size = transaction.zcash_serialized_size().expect(
+        let size = transaction.zcash_serialized_size();
-            "unexpected serialization failure: all structurally valid transactions have a size",
+        let conventional_fee = zip317::conventional_fee(&transaction);
-        );
 
         // The borrow is actually needed to avoid taking ownership
         #[allow(clippy::needless_borrow)]
         Self {
             id: (&transaction).into(),
             size,
+            conventional_fee,
             transaction: Arc::new(transaction),
         }
     }
@@ -233,42 +247,42 @@ impl From<Transaction> for UnminedTx {
 
 impl From<&Transaction> for UnminedTx {
     fn from(transaction: &Transaction) -> Self {
-        let size = transaction.zcash_serialized_size().expect(
+        let size = transaction.zcash_serialized_size();
-            "unexpected serialization failure: all structurally valid transactions have a size",
+        let conventional_fee = zip317::conventional_fee(transaction);
-        );
 
         Self {
             id: transaction.into(),
-            transaction: Arc::new(transaction.clone()),
             size,
+            conventional_fee,
+            transaction: Arc::new(transaction.clone()),
         }
     }
 }
 
 impl From<Arc<Transaction>> for UnminedTx {
     fn from(transaction: Arc<Transaction>) -> Self {
-        let size = transaction.zcash_serialized_size().expect(
+        let size = transaction.zcash_serialized_size();
-            "unexpected serialization failure: all structurally valid transactions have a size",
+        let conventional_fee = zip317::conventional_fee(&transaction);
-        );
 
         Self {
             id: transaction.as_ref().into(),
-            transaction,
             size,
+            conventional_fee,
+            transaction,
         }
     }
 }
 
 impl From<&Arc<Transaction>> for UnminedTx {
     fn from(transaction: &Arc<Transaction>) -> Self {
-        let size = transaction.zcash_serialized_size().expect(
+        let size = transaction.zcash_serialized_size();
-            "unexpected serialization failure: all structurally valid transactions have a size",
+        let conventional_fee = zip317::conventional_fee(transaction);
-        );
 
         Self {
             id: transaction.as_ref().into(),
-            transaction: transaction.clone(),
             size,
+            conventional_fee,
+            transaction: transaction.clone(),
         }
     }
 }
@@ -327,31 +341,34 @@ impl VerifiedUnminedTx {
     /// [ZIP-401]: https://zips.z.cash/zip-0401
     pub fn cost(&self) -> u64 {
         std::cmp::max(
-            self.transaction.size as u64,
+            u64::try_from(self.transaction.size).expect("fits in u64"),
             MEMPOOL_TRANSACTION_COST_THRESHOLD,
         )
     }
 
     /// The computed _eviction weight_ of a verified unmined transaction as part
-    /// of the mempool set.
+    /// of the mempool set, as defined in [ZIP-317] and [ZIP-401].
     ///
-    /// Consensus rule:
+    /// Standard rule:
     ///
     /// > Each transaction also has an eviction weight, which is cost +
     /// > low_fee_penalty, where low_fee_penalty is 16000 if the transaction pays
-    /// > a fee less than the conventional fee, otherwise 0. The conventional fee
+    /// > a fee less than the conventional fee, otherwise 0.
-    /// > is currently defined as 1000 zatoshis
     ///
+    /// > zcashd and zebrad limit the size of the mempool as described in [ZIP-401].
+    /// > This specifies a low fee penalty that is added to the "eviction weight" if the transaction
+    /// > pays a fee less than the conventional transaction fee. This threshold is
+    /// > modified to use the new conventional fee formula.
+    ///
+    /// [ZIP-317]: https://zips.z.cash/zip-0317#mempool-size-limiting
     /// [ZIP-401]: https://zips.z.cash/zip-0401
-    pub fn eviction_weight(self) -> u64 {
+    pub fn eviction_weight(&self) -> u64 {
-        let conventional_fee = 1000;
+        let mut cost = self.cost();
 
-        let low_fee_penalty = if u64::from(self.miner_fee) < conventional_fee {
+        if self.miner_fee < self.transaction.conventional_fee {
-            16_000
+            cost += MEMPOOL_TRANSACTION_LOW_FEE_PENALTY
-        } else {
+        }
-            0
-        };
 
-        self.cost() + low_fee_penalty
+        cost
     }
 }

zebra-chain/src/transaction/unmined/zip317.rs

@@ -0,0 +1,89 @@
+//! The [ZIP-317 conventional fee calculation](https://zips.z.cash/zip-0317#fee-calculation)
+//! for [UnminedTx]s.
+
+use std::cmp::max;
+
+use crate::{
+    amount::{Amount, NonNegative},
+    serialization::ZcashSerialize,
+    transaction::Transaction,
+};
+
+// For doc links
+#[allow(unused_imports)]
+use crate::transaction::UnminedTx;
+
+/// The marginal fee for the ZIP-317 fee calculation, in zatoshis per logical action.
+//
+// TODO: allow Amount<NonNegative> in constants
+const MARGINAL_FEE: i64 = 5_000;
+
+/// The number of grace logical actions allowed by the ZIP-317 fee calculation.
+const GRACE_ACTIONS: u64 = 2;
+
+/// The standard size of p2pkh inputs for the ZIP-317 fee calculation, in bytes.
+const P2PKH_STANDARD_INPUT_SIZE: usize = 150;
+
+/// The standard size of p2pkh outputs for the ZIP-317 fee calculation, in bytes.
+const P2PKH_STANDARD_OUTPUT_SIZE: usize = 34;
+
+/// Returns the conventional fee for `transaction`, as defined by [ZIP-317].
+///
+/// [ZIP-317]: https://zips.z.cash/zip-0317#fee-calculation
+pub fn conventional_fee(transaction: &Transaction) -> Amount<NonNegative> {
+    // zcash_primitives checks for non-p2pkh inputs, but Zebra doesn't.
+    // Conventional fees are only used in the standard rules for mempool eviction
+    // and block production, so these implementations are compatible.
+    //
+    // <https://github.com/zcash/librustzcash/blob/main/zcash_primitives/src/transaction/fees/zip317.rs#L135>
+
+    let marginal_fee: Amount<NonNegative> = MARGINAL_FEE.try_into().expect("fits in amount");
+
+    let tx_in_total_size: usize = transaction
+        .inputs()
+        .iter()
+        .map(|input| input.zcash_serialized_size())
+        .sum();
+
+    let tx_out_total_size: usize = transaction
+        .outputs()
+        .iter()
+        .map(|output| output.zcash_serialized_size())
+        .sum();
+
+    let n_join_split = transaction.joinsplit_count();
+    let n_spends_sapling = transaction.sapling_spends_per_anchor().count();
+    let n_outputs_sapling = transaction.sapling_outputs().count();
+    let n_actions_orchard = transaction.orchard_actions().count();
+
+    let tx_in_logical_actions = div_ceil(tx_in_total_size, P2PKH_STANDARD_INPUT_SIZE);
+    let tx_out_logical_actions = div_ceil(tx_out_total_size, P2PKH_STANDARD_OUTPUT_SIZE);
+
+    let logical_actions = max(tx_in_logical_actions, tx_out_logical_actions)
+        + 2 * n_join_split
+        + max(n_spends_sapling, n_outputs_sapling)
+        + n_actions_orchard;
+    let logical_actions: u64 = logical_actions
+        .try_into()
+        .expect("transaction items are limited by serialized size limit");
+
+    let conventional_fee = marginal_fee * max(GRACE_ACTIONS, logical_actions);
+
+    conventional_fee.expect("conventional fee is positive and limited by serialized size limit")
+}
+
+/// Divide `quotient` by `divisor`, rounding the result up to the nearest integer.
+///
+/// # Correctness
+///
+/// `quotient + divisor` must be less than `usize::MAX`.
+/// `divisor` must not be zero.
+//
+// TODO: replace with usize::div_ceil() when int_roundings stabilises:
+// https://github.com/rust-lang/rust/issues/88581
+fn div_ceil(quotient: usize, divisor: usize) -> usize {
+    // Rust uses truncated integer division, so this is equivalent to:
+    // `ceil(quotient/divisor)`
+    // as long as the addition doesn't overflow or underflow.
+    (quotient + divisor - 1) / divisor
+}

zebra-rpc/src/methods/tests/vectors.rs

@@ -6,6 +6,7 @@ use jsonrpc_core::ErrorCode;
 use tower::buffer::Buffer;
 
 use zebra_chain::{
+    amount::Amount,
     block::Block,
     chain_tip::NoChainTip,
     parameters::Network::*,
@@ -288,6 +289,7 @@ async fn rpc_getrawtransaction() {
                         id: UnminedTxId::Legacy(tx.hash()),
                         transaction: tx.clone(),
                         size: 0,
+                        conventional_fee: Amount::zero(),
                     }]));
                 });
             let get_tx_req = rpc.get_raw_transaction(tx.hash().encode_hex(), 0u8);