From 4c6bfbfb98b686786333c7b2e26a5903799c3390 Mon Sep 17 00:00:00 2001
From: Jack Grigg <jack@electriccoin.co>
Date: Thu, 15 Jun 2023 13:45:53 +0000
Subject: [PATCH 1/7] metrics: Track mempool actions and size bucketed by
 weight

---
 zebra-chain/src/transaction/unmined.rs        |   9 ++
 zebra-chain/src/transaction/unmined/zip317.rs |   2 +-
 .../mempool/storage/verified_set.rs           | 100 ++++++++++++++++++
 3 files changed, 110 insertions(+), 1 deletion(-)

diff --git a/zebra-chain/src/transaction/unmined.rs b/zebra-chain/src/transaction/unmined.rs
index 6b953966627..ea689346d0c 100644
--- a/zebra-chain/src/transaction/unmined.rs
+++ b/zebra-chain/src/transaction/unmined.rs
@@ -337,6 +337,13 @@ pub struct VerifiedUnminedTx {
     /// transparent inputs and outputs.
     pub legacy_sigop_count: u64,
 
+    /// The number of conventional actions for `transaction`, as defined by [ZIP-317].
+    ///
+    /// The number of actions is limited by [`MAX_BLOCK_BYTES`], so it fits in a u32.
+    ///
+    /// [ZIP-317]: https://zips.z.cash/zip-0317#block-production
+    pub conventional_actions: u32,
+
     /// The number of unpaid actions for `transaction`,
     /// as defined by [ZIP-317] for block production.
     ///
@@ -381,6 +388,7 @@ impl VerifiedUnminedTx {
         legacy_sigop_count: u64,
     ) -> Result<Self, zip317::Error> {
         let fee_weight_ratio = zip317::conventional_fee_weight_ratio(&transaction, miner_fee);
+        let conventional_actions = zip317::conventional_actions(&transaction.transaction);
         let unpaid_actions = zip317::unpaid_actions(&transaction, miner_fee);
 
         zip317::mempool_checks(unpaid_actions, miner_fee, transaction.size)?;
@@ -390,6 +398,7 @@ impl VerifiedUnminedTx {
             miner_fee,
             legacy_sigop_count,
             fee_weight_ratio,
+            conventional_actions,
             unpaid_actions,
         })
     }
diff --git a/zebra-chain/src/transaction/unmined/zip317.rs b/zebra-chain/src/transaction/unmined/zip317.rs
index 44ef709aacd..e9f4a757e53 100644
--- a/zebra-chain/src/transaction/unmined/zip317.rs
+++ b/zebra-chain/src/transaction/unmined/zip317.rs
@@ -133,7 +133,7 @@ pub fn conventional_fee_weight_ratio(
 /// as defined by [ZIP-317].
 ///
 /// [ZIP-317]: https://zips.z.cash/zip-0317#fee-calculation
-fn conventional_actions(transaction: &Transaction) -> u32 {
+pub fn conventional_actions(transaction: &Transaction) -> u32 {
     let tx_in_total_size: usize = transaction
         .inputs()
         .iter()
diff --git a/zebrad/src/components/mempool/storage/verified_set.rs b/zebrad/src/components/mempool/storage/verified_set.rs
index e6f0dcbd3f1..1d1f835fb36 100644
--- a/zebrad/src/components/mempool/storage/verified_set.rs
+++ b/zebrad/src/components/mempool/storage/verified_set.rs
@@ -286,10 +286,110 @@ impl VerifiedSet {
     }
 
     fn update_metrics(&mut self) {
+        // Track the sum of unpaid actions within each transaction (as they are subject to the
+        // unpaid action limit). Transactions that have weight >= 1 have no unpaid actions by
+        // definition.
+        let mut unpaid_actions_with_weight_lt20pct = 0;
+        let mut unpaid_actions_with_weight_lt40pct = 0;
+        let mut unpaid_actions_with_weight_lt60pct = 0;
+        let mut unpaid_actions_with_weight_lt80pct = 0;
+        let mut unpaid_actions_with_weight_lt1 = 0;
+
+        // Track the total number of paid actions across all transactions in the mempool. This
+        // added to the bucketed unpaid actions above is equal to the total number of conventional
+        // actions in the mempool.
+        let mut paid_actions = 0;
+
+        // Track the sum of transaction sizes (the metric by which they are mainly limited) across
+        // several buckets.
+        let mut size_with_weight_lt1 = 0;
+        let mut size_with_weight_eq1 = 0;
+        let mut size_with_weight_gt1 = 0;
+        let mut size_with_weight_gt2 = 0;
+        let mut size_with_weight_gt3 = 0;
+
+        for entry in self.full_transactions() {
+            paid_actions += entry.conventional_actions - entry.unpaid_actions;
+
+            if entry.fee_weight_ratio > 3.0 {
+                size_with_weight_gt3 += entry.transaction.size;
+            } else if entry.fee_weight_ratio > 2.0 {
+                size_with_weight_gt2 += entry.transaction.size;
+            } else if entry.fee_weight_ratio > 1.0 {
+                size_with_weight_gt1 += entry.transaction.size;
+            } else if entry.fee_weight_ratio == 1.0 {
+                size_with_weight_eq1 += entry.transaction.size;
+            } else {
+                size_with_weight_lt1 += entry.transaction.size;
+                if entry.fee_weight_ratio < 0.2 {
+                    unpaid_actions_with_weight_lt20pct += entry.unpaid_actions;
+                } else if entry.fee_weight_ratio < 0.4 {
+                    unpaid_actions_with_weight_lt40pct += entry.unpaid_actions;
+                } else if entry.fee_weight_ratio < 0.6 {
+                    unpaid_actions_with_weight_lt60pct += entry.unpaid_actions;
+                } else if entry.fee_weight_ratio < 0.8 {
+                    unpaid_actions_with_weight_lt80pct += entry.unpaid_actions;
+                } else {
+                    unpaid_actions_with_weight_lt1 += entry.unpaid_actions;
+                }
+            }
+        }
+
+        metrics::gauge!(
+            "zcash.mempool.actions.unpaid",
+            unpaid_actions_with_weight_lt20pct as f64,
+            "bk" => "< 0.2",
+        );
+        metrics::gauge!(
+            "zcash.mempool.actions.unpaid",
+            unpaid_actions_with_weight_lt40pct as f64,
+            "bk" => "< 0.4",
+        );
+        metrics::gauge!(
+            "zcash.mempool.actions.unpaid",
+            unpaid_actions_with_weight_lt60pct as f64,
+            "bk" => "< 0.6",
+        );
+        metrics::gauge!(
+            "zcash.mempool.actions.unpaid",
+            unpaid_actions_with_weight_lt80pct as f64,
+            "bk" => "< 0.8",
+        );
+        metrics::gauge!(
+            "zcash.mempool.actions.unpaid",
+            unpaid_actions_with_weight_lt1 as f64,
+            "bk" => "< 1",
+        );
+        metrics::gauge!("zcash.mempool.actions.paid", paid_actions as f64);
         metrics::gauge!(
             "zcash.mempool.size.transactions",
             self.transaction_count() as f64,
         );
+        metrics::gauge!(
+            "zcash.mempool.size.weighted",
+            size_with_weight_lt1 as f64,
+            "bk" => "< 1",
+        );
+        metrics::gauge!(
+            "zcash.mempool.size.weighted",
+            size_with_weight_eq1 as f64,
+            "bk" => "1",
+        );
+        metrics::gauge!(
+            "zcash.mempool.size.weighted",
+            size_with_weight_gt1 as f64,
+            "bk" => "> 1",
+        );
+        metrics::gauge!(
+            "zcash.mempool.size.weighted",
+            size_with_weight_gt2 as f64,
+            "bk" => "> 2",
+        );
+        metrics::gauge!(
+            "zcash.mempool.size.weighted",
+            size_with_weight_gt3 as f64,
+            "bk" => "> 3",
+        );
         metrics::gauge!(
             "zcash.mempool.size.bytes",
             self.transactions_serialized_size as f64,

From c697558b79d81a3236b3c56261292a48dd3370f6 Mon Sep 17 00:00:00 2001
From: Marek <mail@marek.onl>
Date: Thu, 15 Jun 2023 18:23:16 +0200
Subject: [PATCH 2/7] Fix tests

---
 zebra-rpc/src/methods/tests/vectors.rs | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/zebra-rpc/src/methods/tests/vectors.rs b/zebra-rpc/src/methods/tests/vectors.rs
index f3b22fce482..aec71ed28a6 100644
--- a/zebra-rpc/src/methods/tests/vectors.rs
+++ b/zebra-rpc/src/methods/tests/vectors.rs
@@ -1170,7 +1170,7 @@ async fn rpc_getblocktemplate_mining_address(use_p2pkh: bool) {
         block::{Hash, MAX_BLOCK_BYTES, ZCASH_BLOCK_VERSION},
         chain_sync_status::MockSyncStatus,
         serialization::DateTime32,
-        transaction::VerifiedUnminedTx,
+        transaction::{zip317, VerifiedUnminedTx},
         work::difficulty::{CompactDifficulty, ExpandedDifficulty, U256},
     };
     use zebra_consensus::MAX_BLOCK_SIGOPS;
@@ -1425,10 +1425,13 @@ async fn rpc_getblocktemplate_mining_address(use_p2pkh: bool) {
         conventional_fee: 0.try_into().unwrap(),
     };
 
+    let conventional_actions = zip317::conventional_actions(&unmined_tx.transaction);
+
     let verified_unmined_tx = VerifiedUnminedTx {
         transaction: unmined_tx,
         miner_fee: 0.try_into().unwrap(),
         legacy_sigop_count: 0,
+        conventional_actions,
         unpaid_actions: 0,
         fee_weight_ratio: 1.0,
     };

From 61a621c61318ba60416379bae794dfec93db7920 Mon Sep 17 00:00:00 2001
From: Alfredo Garcia <oxarbitrage@gmail.com>
Date: Mon, 17 Jul 2023 11:21:06 -0300
Subject: [PATCH 3/7] draft fix tests

---
 zebra-chain/src/transaction/unmined.rs        | 16 ++++
 zebrad/src/components/mempool/storage.rs      |  1 +
 .../components/mempool/storage/tests/prop.rs  | 78 ++++++++++---------
 .../mempool/storage/verified_set.rs           |  2 +
 zebrad/src/components/mempool/tests/prop.rs   | 10 +--
 5 files changed, 65 insertions(+), 42 deletions(-)

diff --git a/zebra-chain/src/transaction/unmined.rs b/zebra-chain/src/transaction/unmined.rs
index ea689346d0c..86df3a07bba 100644
--- a/zebra-chain/src/transaction/unmined.rs
+++ b/zebra-chain/src/transaction/unmined.rs
@@ -452,4 +452,20 @@ impl VerifiedUnminedTx {
 
         cost
     }
+
+    ///
+    #[cfg(any(test, feature = "proptest-impl"))]
+    pub fn fix_arbitrary_generated_action_overflows(&mut self) -> Self {
+        use rand::Rng;
+
+        let mut rng = rand::thread_rng();
+
+        let conventional_actions = rng.gen_range(10..10000);
+        let unpaid_actions = rng.gen_range(0..10);
+
+        self.conventional_actions = conventional_actions;
+        self.unpaid_actions = unpaid_actions;
+
+        self.clone()
+    }
 }
diff --git a/zebrad/src/components/mempool/storage.rs b/zebrad/src/components/mempool/storage.rs
index bed0f9aabd2..5c0b635c085 100644
--- a/zebrad/src/components/mempool/storage.rs
+++ b/zebrad/src/components/mempool/storage.rs
@@ -215,6 +215,7 @@ impl Storage {
         // Then, we try to insert into the pool. If this fails the transaction is rejected.
         let mut result = Ok(tx_id);
         if let Err(rejection_error) = self.verified.insert(tx) {
+            dbg!("error here?");
             tracing::debug!(
                 ?tx_id,
                 ?rejection_error,
diff --git a/zebrad/src/components/mempool/storage/tests/prop.rs b/zebrad/src/components/mempool/storage/tests/prop.rs
index eca65935acb..443c0565753 100644
--- a/zebrad/src/components/mempool/storage/tests/prop.rs
+++ b/zebrad/src/components/mempool/storage/tests/prop.rs
@@ -111,7 +111,7 @@ proptest! {
     /// Test that the reject list length limits are applied when evicting transactions.
     #[test]
     fn reject_lists_are_limited_insert_eviction(
-        transactions in vec(any::<VerifiedUnminedTx>(), MEMPOOL_TX_COUNT + 1).prop_map(SummaryDebug),
+        transactions in vec(any::<VerifiedUnminedTx>().prop_map(|mut tx| tx.fix_arbitrary_generated_action_overflows()), MEMPOOL_TX_COUNT + 1).prop_map(SummaryDebug),
         mut rejection_template in any::<UnminedTxId>()
     ) {
         // Use as cost limit the costs of all transactions except one
@@ -960,43 +960,47 @@ impl Arbitrary for MultipleTransactionRemovalTestInput {
     type Parameters = ();
 
     fn arbitrary_with(_: Self::Parameters) -> Self::Strategy {
-        vec(any::<VerifiedUnminedTx>(), 1..MEMPOOL_TX_COUNT)
-            .prop_flat_map(|transactions| {
-                let indices_to_remove =
-                    vec(any::<bool>(), 1..=transactions.len()).prop_map(|removal_markers| {
-                        removal_markers
-                            .into_iter()
-                            .enumerate()
-                            .filter(|(_, should_remove)| *should_remove)
-                            .map(|(index, _)| index)
-                            .collect::<HashSet<usize>>()
-                    });
-
-                (Just(transactions), indices_to_remove)
-            })
-            .prop_flat_map(|(transactions, indices_to_remove)| {
-                let wtx_ids_to_remove: HashSet<_> = indices_to_remove
-                    .iter()
-                    .map(|&index| transactions[index].transaction.id)
-                    .collect();
+        vec(
+            any::<VerifiedUnminedTx>()
+                .prop_map(|mut tx| tx.fix_arbitrary_generated_action_overflows()),
+            1..MEMPOOL_TX_COUNT,
+        )
+        .prop_flat_map(|transactions| {
+            let indices_to_remove =
+                vec(any::<bool>(), 1..=transactions.len()).prop_map(|removal_markers| {
+                    removal_markers
+                        .into_iter()
+                        .enumerate()
+                        .filter(|(_, should_remove)| *should_remove)
+                        .map(|(index, _)| index)
+                        .collect::<HashSet<usize>>()
+                });
 
-                let mined_ids_to_remove: HashSet<transaction::Hash> = wtx_ids_to_remove
-                    .iter()
-                    .map(|unmined_id| unmined_id.mined_id())
-                    .collect();
-
-                prop_oneof![
-                    Just(RejectAndRemoveSameEffects {
-                        transactions: transactions.clone().into(),
-                        mined_ids_to_remove: mined_ids_to_remove.into(),
-                    }),
-                    Just(RemoveExact {
-                        transactions: transactions.into(),
-                        wtx_ids_to_remove: wtx_ids_to_remove.into(),
-                    }),
-                ]
-            })
-            .boxed()
+            (Just(transactions), indices_to_remove)
+        })
+        .prop_flat_map(|(transactions, indices_to_remove)| {
+            let wtx_ids_to_remove: HashSet<_> = indices_to_remove
+                .iter()
+                .map(|&index| transactions[index].transaction.id)
+                .collect();
+
+            let mined_ids_to_remove: HashSet<transaction::Hash> = wtx_ids_to_remove
+                .iter()
+                .map(|unmined_id| unmined_id.mined_id())
+                .collect();
+
+            prop_oneof![
+                Just(RejectAndRemoveSameEffects {
+                    transactions: transactions.clone().into(),
+                    mined_ids_to_remove: mined_ids_to_remove.into(),
+                }),
+                Just(RemoveExact {
+                    transactions: transactions.into(),
+                    wtx_ids_to_remove: wtx_ids_to_remove.into(),
+                }),
+            ]
+        })
+        .boxed()
     }
 
     type Strategy = BoxedStrategy<Self>;
diff --git a/zebrad/src/components/mempool/storage/verified_set.rs b/zebrad/src/components/mempool/storage/verified_set.rs
index 1d1f835fb36..7437e470634 100644
--- a/zebrad/src/components/mempool/storage/verified_set.rs
+++ b/zebrad/src/components/mempool/storage/verified_set.rs
@@ -309,6 +309,8 @@ impl VerifiedSet {
         let mut size_with_weight_gt3 = 0;
 
         for entry in self.full_transactions() {
+            dbg!(&entry);
+            //dbg!(&entry.unpaid_actions);
             paid_actions += entry.conventional_actions - entry.unpaid_actions;
 
             if entry.fee_weight_ratio > 3.0 {
diff --git a/zebrad/src/components/mempool/tests/prop.rs b/zebrad/src/components/mempool/tests/prop.rs
index 46e85b35e14..c6dcc34e9ee 100644
--- a/zebrad/src/components/mempool/tests/prop.rs
+++ b/zebrad/src/components/mempool/tests/prop.rs
@@ -52,7 +52,7 @@ proptest! {
     #[test]
     fn storage_is_cleared_on_single_chain_reset(
         network in any::<Network>(),
-        transaction in any::<DisplayToDebug<VerifiedUnminedTx>>(),
+        transaction in any::<VerifiedUnminedTx>().prop_map(|mut tx| tx.fix_arbitrary_generated_action_overflows()),
         chain_tip in any::<DisplayToDebug<ChainTipBlock>>(),
     ) {
         let (runtime, _init_guard) = zebra_test::init_async();
@@ -74,7 +74,7 @@ proptest! {
             // Insert a dummy transaction.
             mempool
                 .storage()
-                .insert(transaction.0)
+                .insert(transaction)
                 .expect("Inserting a transaction should succeed");
 
             // The first call to `poll_ready` shouldn't clear the storage yet.
@@ -102,7 +102,7 @@ proptest! {
     fn storage_is_cleared_on_multiple_chain_resets(
         network in any::<Network>(),
         mut previous_chain_tip in any::<DisplayToDebug<ChainTipBlock>>(),
-        mut transactions in vec(any::<DisplayToDebug<VerifiedUnminedTx>>(), 0..CHAIN_LENGTH),
+        mut transactions in vec(any::<VerifiedUnminedTx>().prop_map(|mut tx| tx.fix_arbitrary_generated_action_overflows()), 0..CHAIN_LENGTH),
         fake_chain_tips in vec(any::<DisplayToDebug<FakeChainTip>>(), 0..CHAIN_LENGTH),
     ) {
         let (runtime, _init_guard) = zebra_test::init_async();
@@ -148,7 +148,7 @@ proptest! {
                 // Insert the dummy transaction into the mempool.
                 mempool
                     .storage()
-                    .insert(transaction.0.clone())
+                    .insert(transaction.clone())
                     .expect("Inserting a transaction should succeed");
 
                 // Set the new chain tip.
@@ -184,7 +184,7 @@ proptest! {
     #[test]
     fn storage_is_cleared_if_syncer_falls_behind(
         network in any::<Network>(),
-        transaction in any::<VerifiedUnminedTx>(),
+        transaction in any::<VerifiedUnminedTx>().prop_map(|mut tx| tx.fix_arbitrary_generated_action_overflows()),
     ) {
         let (runtime, _init_guard) = zebra_test::init_async();
 

From c873b6be73a303f18260d08baea0a2e9fa6c453f Mon Sep 17 00:00:00 2001
From: Alfredo Garcia <oxarbitrage@gmail.com>
Date: Mon, 17 Jul 2023 16:04:34 -0300
Subject: [PATCH 4/7] fix `fix_arbitrary_generated_action_overflows`

---
 zebra-chain/src/transaction/unmined.rs               | 12 ++++++++++--
 zebrad/src/components/mempool/storage.rs             |  1 -
 .../src/components/mempool/storage/verified_set.rs   |  2 --
 3 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/zebra-chain/src/transaction/unmined.rs b/zebra-chain/src/transaction/unmined.rs
index 86df3a07bba..10f12d9db86 100644
--- a/zebra-chain/src/transaction/unmined.rs
+++ b/zebra-chain/src/transaction/unmined.rs
@@ -460,8 +460,16 @@ impl VerifiedUnminedTx {
 
         let mut rng = rand::thread_rng();
 
-        let conventional_actions = rng.gen_range(10..10000);
-        let unpaid_actions = rng.gen_range(0..10);
+        //
+        let conventional_actions = rng.gen_range(0..2 ^ 16);
+
+        //
+        let mut unpaid_actions = rng.gen_range(0..zip317::BLOCK_PRODUCTION_UNPAID_ACTION_LIMIT);
+
+        //
+        if unpaid_actions > conventional_actions {
+            unpaid_actions = conventional_actions;
+        }
 
         self.conventional_actions = conventional_actions;
         self.unpaid_actions = unpaid_actions;
diff --git a/zebrad/src/components/mempool/storage.rs b/zebrad/src/components/mempool/storage.rs
index 5c0b635c085..bed0f9aabd2 100644
--- a/zebrad/src/components/mempool/storage.rs
+++ b/zebrad/src/components/mempool/storage.rs
@@ -215,7 +215,6 @@ impl Storage {
         // Then, we try to insert into the pool. If this fails the transaction is rejected.
         let mut result = Ok(tx_id);
         if let Err(rejection_error) = self.verified.insert(tx) {
-            dbg!("error here?");
             tracing::debug!(
                 ?tx_id,
                 ?rejection_error,
diff --git a/zebrad/src/components/mempool/storage/verified_set.rs b/zebrad/src/components/mempool/storage/verified_set.rs
index 7437e470634..1d1f835fb36 100644
--- a/zebrad/src/components/mempool/storage/verified_set.rs
+++ b/zebrad/src/components/mempool/storage/verified_set.rs
@@ -309,8 +309,6 @@ impl VerifiedSet {
         let mut size_with_weight_gt3 = 0;
 
         for entry in self.full_transactions() {
-            dbg!(&entry);
-            //dbg!(&entry.unpaid_actions);
             paid_actions += entry.conventional_actions - entry.unpaid_actions;
 
             if entry.fee_weight_ratio > 3.0 {

From a0aa7037e681d874152ff051fac19c57c7ee8d97 Mon Sep 17 00:00:00 2001
From: Alfredo Garcia <oxarbitrage@gmail.com>
Date: Mon, 17 Jul 2023 16:39:47 -0300
Subject: [PATCH 5/7] add some docs

---
 zebra-chain/src/transaction/unmined.rs | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/zebra-chain/src/transaction/unmined.rs b/zebra-chain/src/transaction/unmined.rs
index 10f12d9db86..0f4f36f61d3 100644
--- a/zebra-chain/src/transaction/unmined.rs
+++ b/zebra-chain/src/transaction/unmined.rs
@@ -32,6 +32,9 @@ use UnminedTxId::*;
 #[cfg(any(test, feature = "proptest-impl"))]
 use proptest_derive::Arbitrary;
 
+#[cfg(any(test, feature = "proptest-impl"))]
+use rand::Rng;
+
 // Documentation-only
 #[allow(unused_imports)]
 use crate::block::MAX_BLOCK_BYTES;
@@ -453,20 +456,20 @@ impl VerifiedUnminedTx {
         cost
     }
 
-    ///
+    /// When deriving `Arbitrary` for `VerifiedUnminedTx` action fields can be generated
+    /// that are against consensus rules. This function prevent those issues by generating
+    /// new conventional and unpaid actions where rules are considered.
     #[cfg(any(test, feature = "proptest-impl"))]
     pub fn fix_arbitrary_generated_action_overflows(&mut self) -> Self {
-        use rand::Rng;
-
         let mut rng = rand::thread_rng();
 
-        //
+        // The number of actions can't be greater than 2^16.
         let conventional_actions = rng.gen_range(0..2 ^ 16);
 
-        //
+        // The number of unpaid actions can't be greater than `BLOCK_PRODUCTION_UNPAID_ACTION_LIMIT`.
         let mut unpaid_actions = rng.gen_range(0..zip317::BLOCK_PRODUCTION_UNPAID_ACTION_LIMIT);
 
-        //
+        // The number of unpaid actions can't be greater than the actions of the transaction.
         if unpaid_actions > conventional_actions {
             unpaid_actions = conventional_actions;
         }

From 44c9a80ad00eedc2e39aa06a2060177e908f95f5 Mon Sep 17 00:00:00 2001
From: Alfredo Garcia <oxarbitrage@gmail.com>
Date: Tue, 18 Jul 2023 14:15:21 -0300
Subject: [PATCH 6/7] manually derive arbitrary

---
 zebra-chain/src/transaction/arbitrary.rs      | 50 +++++++++++-
 zebra-chain/src/transaction/unmined.rs        | 25 ------
 .../components/mempool/storage/tests/prop.rs  | 78 +++++++++----------
 zebrad/src/components/mempool/tests/prop.rs   | 10 +--
 4 files changed, 91 insertions(+), 72 deletions(-)

diff --git a/zebra-chain/src/transaction/arbitrary.rs b/zebra-chain/src/transaction/arbitrary.rs
index c218ccb6238..43581e07a0e 100644
--- a/zebra-chain/src/transaction/arbitrary.rs
+++ b/zebra-chain/src/transaction/arbitrary.rs
@@ -30,7 +30,9 @@ use crate::{
 
 use itertools::Itertools;
 
-use super::{FieldNotPresent, JoinSplitData, LockTime, Memo, Transaction, UnminedTx};
+use super::{
+    FieldNotPresent, JoinSplitData, LockTime, Memo, Transaction, UnminedTx, VerifiedUnminedTx,
+};
 
 /// The maximum number of arbitrary transactions, inputs, or outputs.
 ///
@@ -783,6 +785,52 @@ impl Arbitrary for UnminedTx {
     type Strategy = BoxedStrategy<Self>;
 }
 
+impl Arbitrary for VerifiedUnminedTx {
+    type Parameters = ();
+
+    fn arbitrary_with(_args: Self::Parameters) -> Self::Strategy {
+        (
+            any::<UnminedTx>(),
+            any::<Amount<NonNegative>>(),
+            any::<u64>(),
+            any::<(u16, u16)>().prop_map(|(unpaid_actions, conventional_actions)| {
+                (
+                    unpaid_actions % conventional_actions.saturating_add(1),
+                    conventional_actions,
+                )
+            }),
+            any::<f32>(),
+        )
+            .prop_map(
+                |(
+                    transaction,
+                    miner_fee,
+                    legacy_sigop_count,
+                    (conventional_actions, mut unpaid_actions),
+                    fee_weight_ratio,
+                )| {
+                    if unpaid_actions > conventional_actions {
+                        unpaid_actions = conventional_actions;
+                    }
+
+                    let conventional_actions = conventional_actions as u32;
+                    let unpaid_actions = unpaid_actions as u32;
+
+                    Self {
+                        transaction,
+                        miner_fee,
+                        legacy_sigop_count,
+                        conventional_actions,
+                        unpaid_actions,
+                        fee_weight_ratio,
+                    }
+                },
+            )
+            .boxed()
+    }
+    type Strategy = BoxedStrategy<Self>;
+}
+
 // Utility functions
 
 /// Convert `trans` into a fake v5 transaction,
diff --git a/zebra-chain/src/transaction/unmined.rs b/zebra-chain/src/transaction/unmined.rs
index 0f4f36f61d3..96ad19db4d9 100644
--- a/zebra-chain/src/transaction/unmined.rs
+++ b/zebra-chain/src/transaction/unmined.rs
@@ -328,7 +328,6 @@ impl From<&Arc<Transaction>> for UnminedTx {
 //
 // This struct can't be `Eq`, because it contains a `f32`.
 #[derive(Clone, PartialEq)]
-#[cfg_attr(any(test, feature = "proptest-impl"), derive(Arbitrary))]
 pub struct VerifiedUnminedTx {
     /// The unmined transaction.
     pub transaction: UnminedTx,
@@ -455,28 +454,4 @@ impl VerifiedUnminedTx {
 
         cost
     }
-
-    /// When deriving `Arbitrary` for `VerifiedUnminedTx` action fields can be generated
-    /// that are against consensus rules. This function prevent those issues by generating
-    /// new conventional and unpaid actions where rules are considered.
-    #[cfg(any(test, feature = "proptest-impl"))]
-    pub fn fix_arbitrary_generated_action_overflows(&mut self) -> Self {
-        let mut rng = rand::thread_rng();
-
-        // The number of actions can't be greater than 2^16.
-        let conventional_actions = rng.gen_range(0..2 ^ 16);
-
-        // The number of unpaid actions can't be greater than `BLOCK_PRODUCTION_UNPAID_ACTION_LIMIT`.
-        let mut unpaid_actions = rng.gen_range(0..zip317::BLOCK_PRODUCTION_UNPAID_ACTION_LIMIT);
-
-        // The number of unpaid actions can't be greater than the actions of the transaction.
-        if unpaid_actions > conventional_actions {
-            unpaid_actions = conventional_actions;
-        }
-
-        self.conventional_actions = conventional_actions;
-        self.unpaid_actions = unpaid_actions;
-
-        self.clone()
-    }
 }
diff --git a/zebrad/src/components/mempool/storage/tests/prop.rs b/zebrad/src/components/mempool/storage/tests/prop.rs
index 443c0565753..eca65935acb 100644
--- a/zebrad/src/components/mempool/storage/tests/prop.rs
+++ b/zebrad/src/components/mempool/storage/tests/prop.rs
@@ -111,7 +111,7 @@ proptest! {
     /// Test that the reject list length limits are applied when evicting transactions.
     #[test]
     fn reject_lists_are_limited_insert_eviction(
-        transactions in vec(any::<VerifiedUnminedTx>().prop_map(|mut tx| tx.fix_arbitrary_generated_action_overflows()), MEMPOOL_TX_COUNT + 1).prop_map(SummaryDebug),
+        transactions in vec(any::<VerifiedUnminedTx>(), MEMPOOL_TX_COUNT + 1).prop_map(SummaryDebug),
         mut rejection_template in any::<UnminedTxId>()
     ) {
         // Use as cost limit the costs of all transactions except one
@@ -960,47 +960,43 @@ impl Arbitrary for MultipleTransactionRemovalTestInput {
     type Parameters = ();
 
     fn arbitrary_with(_: Self::Parameters) -> Self::Strategy {
-        vec(
-            any::<VerifiedUnminedTx>()
-                .prop_map(|mut tx| tx.fix_arbitrary_generated_action_overflows()),
-            1..MEMPOOL_TX_COUNT,
-        )
-        .prop_flat_map(|transactions| {
-            let indices_to_remove =
-                vec(any::<bool>(), 1..=transactions.len()).prop_map(|removal_markers| {
-                    removal_markers
-                        .into_iter()
-                        .enumerate()
-                        .filter(|(_, should_remove)| *should_remove)
-                        .map(|(index, _)| index)
-                        .collect::<HashSet<usize>>()
-                });
-
-            (Just(transactions), indices_to_remove)
-        })
-        .prop_flat_map(|(transactions, indices_to_remove)| {
-            let wtx_ids_to_remove: HashSet<_> = indices_to_remove
-                .iter()
-                .map(|&index| transactions[index].transaction.id)
-                .collect();
-
-            let mined_ids_to_remove: HashSet<transaction::Hash> = wtx_ids_to_remove
-                .iter()
-                .map(|unmined_id| unmined_id.mined_id())
-                .collect();
+        vec(any::<VerifiedUnminedTx>(), 1..MEMPOOL_TX_COUNT)
+            .prop_flat_map(|transactions| {
+                let indices_to_remove =
+                    vec(any::<bool>(), 1..=transactions.len()).prop_map(|removal_markers| {
+                        removal_markers
+                            .into_iter()
+                            .enumerate()
+                            .filter(|(_, should_remove)| *should_remove)
+                            .map(|(index, _)| index)
+                            .collect::<HashSet<usize>>()
+                    });
+
+                (Just(transactions), indices_to_remove)
+            })
+            .prop_flat_map(|(transactions, indices_to_remove)| {
+                let wtx_ids_to_remove: HashSet<_> = indices_to_remove
+                    .iter()
+                    .map(|&index| transactions[index].transaction.id)
+                    .collect();
 
-            prop_oneof![
-                Just(RejectAndRemoveSameEffects {
-                    transactions: transactions.clone().into(),
-                    mined_ids_to_remove: mined_ids_to_remove.into(),
-                }),
-                Just(RemoveExact {
-                    transactions: transactions.into(),
-                    wtx_ids_to_remove: wtx_ids_to_remove.into(),
-                }),
-            ]
-        })
-        .boxed()
+                let mined_ids_to_remove: HashSet<transaction::Hash> = wtx_ids_to_remove
+                    .iter()
+                    .map(|unmined_id| unmined_id.mined_id())
+                    .collect();
+
+                prop_oneof![
+                    Just(RejectAndRemoveSameEffects {
+                        transactions: transactions.clone().into(),
+                        mined_ids_to_remove: mined_ids_to_remove.into(),
+                    }),
+                    Just(RemoveExact {
+                        transactions: transactions.into(),
+                        wtx_ids_to_remove: wtx_ids_to_remove.into(),
+                    }),
+                ]
+            })
+            .boxed()
     }
 
     type Strategy = BoxedStrategy<Self>;
diff --git a/zebrad/src/components/mempool/tests/prop.rs b/zebrad/src/components/mempool/tests/prop.rs
index 79a0a952f65..0b0a35cd19e 100644
--- a/zebrad/src/components/mempool/tests/prop.rs
+++ b/zebrad/src/components/mempool/tests/prop.rs
@@ -52,7 +52,7 @@ proptest! {
     #[test]
     fn storage_is_cleared_on_single_chain_reset(
         network in any::<Network>(),
-        transaction in any::<VerifiedUnminedTx>().prop_map(|mut tx| tx.fix_arbitrary_generated_action_overflows()),
+        transaction in any::<DisplayToDebug<VerifiedUnminedTx>>(),
         chain_tip in any::<DisplayToDebug<ChainTipBlock>>(),
     ) {
         let (runtime, _init_guard) = zebra_test::init_async();
@@ -74,7 +74,7 @@ proptest! {
             // Insert a dummy transaction.
             mempool
                 .storage()
-                .insert(transaction)
+                .insert(transaction.0)
                 .expect("Inserting a transaction should succeed");
 
             // The first call to `poll_ready` shouldn't clear the storage yet.
@@ -102,7 +102,7 @@ proptest! {
     fn storage_is_cleared_on_multiple_chain_resets(
         network in any::<Network>(),
         mut previous_chain_tip in any::<DisplayToDebug<ChainTipBlock>>(),
-        mut transactions in vec(any::<DisplayToDebug<VerifiedUnminedTx>>().prop_map(|mut tx| tx.fix_arbitrary_generated_action_overflows()), 0..CHAIN_LENGTH),
+        mut transactions in vec(any::<DisplayToDebug<VerifiedUnminedTx>>(), 0..CHAIN_LENGTH),
         fake_chain_tips in vec(any::<TypeNameToDebug<FakeChainTip>>(), 0..CHAIN_LENGTH),
     ) {
         let (runtime, _init_guard) = zebra_test::init_async();
@@ -148,7 +148,7 @@ proptest! {
                 // Insert the dummy transaction into the mempool.
                 mempool
                     .storage()
-                    .insert(transaction.clone())
+                    .insert(transaction.0.clone())
                     .expect("Inserting a transaction should succeed");
 
                 // Set the new chain tip.
@@ -184,7 +184,7 @@ proptest! {
     #[test]
     fn storage_is_cleared_if_syncer_falls_behind(
         network in any::<Network>(),
-        transaction in any::<VerifiedUnminedTx>().prop_map(|mut tx| tx.fix_arbitrary_generated_action_overflows()),
+        transaction in any::<VerifiedUnminedTx>(),
     ) {
         let (runtime, _init_guard) = zebra_test::init_async();
 

From 5f4185292b9ce4519c4b9b0a41ef6e544de53763 Mon Sep 17 00:00:00 2001
From: Alfredo Garcia <oxarbitrage@gmail.com>
Date: Tue, 18 Jul 2023 14:18:08 -0300
Subject: [PATCH 7/7] remove unused import

---
 zebra-chain/src/transaction/unmined.rs | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/zebra-chain/src/transaction/unmined.rs b/zebra-chain/src/transaction/unmined.rs
index 96ad19db4d9..da716573e8b 100644
--- a/zebra-chain/src/transaction/unmined.rs
+++ b/zebra-chain/src/transaction/unmined.rs
@@ -32,9 +32,6 @@ use UnminedTxId::*;
 #[cfg(any(test, feature = "proptest-impl"))]
 use proptest_derive::Arbitrary;
 
-#[cfg(any(test, feature = "proptest-impl"))]
-use rand::Rng;
-
 // Documentation-only
 #[allow(unused_imports)]
 use crate::block::MAX_BLOCK_BYTES;