Refactor mempool spend conflict checks to increase performance (#2826)

* Add `HashSet`s to help spend conflict detection

Keep track of the spent transparent outpoints and the revealed
nullifiers.

Clippy complained that the `ActiveState` had variants with large size
differences, but that was expected, so I disabled that lint on that
`enum`.

* Clear the `HashSet`s when clearing the mempool

Clear them so that they remain consistent with the set of verified
transactions.

* Use `HashSet`s to check for spend conflicts

Store new outputs into its respective `HashSet`, and abort if a
duplicate output is found.

* Remove inserted outputs when aborting

Restore the `HashSet` to its previous state.

* Remove tracked outputs when removing a transaction

Keep the mempool storage in a consistent state when a transaction is
removed.

* Remove tracked outputs when evicting from mempool

Ensure eviction also keeps the tracked outputs consistent with the
verified transactions.

* Refactor to create a `VerifiedSet` helper type

Move the code to handle the output caches into the new type. Also move
the eviction code to make things a little simpler.

* Refactor to have a single `remove` method

Centralize the code that handles the removal of a transaction to avoid
mistakes.

* Move mempool size limiting back to `Storage`

Because the evicted transactions must be added to the rejected list.

* Remove leftover `dbg!` statement

Leftover from some temporary testing code.

Co-authored-by: teor <teor@riseup.net>

* Remove unnecessary `TODO`

It is more speculation than planning, so it doesn't add much value.

Co-authored-by: teor <teor@riseup.net>

* Fix typo in documentation

The verb should match the subject "transactions" which is plural.

Co-authored-by: teor <teor@riseup.net>

* Add a comment to warn about correctness

There's a subtle but important detail in the implementation that should
be made more visible to avoid mistakes in the future.

Co-authored-by: teor <teor@riseup.net>

* Remove outdated comment

Left-over from the attempt to move the eviction into the `VerifiedSet`.

* Improve comment explaining lint removal

Rewrite the comment explaining why the Clippy lint was ignored.

* Check for spend conflicts in `VerifiedSet`

Refactor to avoid API misuse.

* Test rejected transaction rollback

Using two transactions, perform the same test adding a conflict to both
of them to check if the second inserted transaction is properly
rejected. Then remove any conflicts from the second transaction and add
it again. That should work, because if it doesn't it means that when the
second transaction was rejected it left things it shouldn't in the
cache.

* Test removal of multiple transactions

When removing multiple transactions from the mempool storage, all of the
ones requested should be removed and any other transaction should be
still be there afterwards.

* Increase mempool size to 4, so that spend conflict tests work

If the mempool size is smaller than 4,
these tests don't fail on a trivial removal bug.
Because we need a minimum number of transactions in the mempool
to trigger the bug.

Also commit a proptest seed that fails on a trivial removal bug.
(This seed fails if we remove indexes in order,
because every index past the first removes the wrong transaction.)

* Summarise transaction data in proptest error output

* Summarise spend conflict field data in proptest error output

* Summarise multiple removal field data in proptest error output

And replace the very large proptest debug output with the new summary.

Co-authored-by: teor <teor@riseup.net>

zebra-chain/src/block.rs

@@ -55,11 +55,14 @@ pub struct Block {
 impl fmt::Display for Block {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         let mut fmter = f.debug_struct("Block");
+
         if let Some(height) = self.coinbase_height() {
             fmter.field("height", &height);
         }
+        fmter.field("transactions", &self.transactions.len());
+        fmter.field("hash", &DisplayToDebug(self.hash()));
 
-        fmter.field("hash", &DisplayToDebug(self.hash())).finish()
+        fmter.finish()
     }
 }
 

zebra-chain/src/orchard/shielded_data.rs

@@ -2,7 +2,7 @@
 
 use std::{
     cmp::{Eq, PartialEq},
-    fmt::Debug,
+    fmt::{self, Debug},
     io,
 };
 
@@ -39,6 +39,22 @@ pub struct ShieldedData {
     pub binding_sig: Signature<Binding>,
 }
 
+impl fmt::Display for ShieldedData {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        let mut fmter = f.debug_struct("orchard::ShieldedData");
+
+        fmter.field("actions", &self.actions.len());
+        fmter.field("value_balance", &self.value_balance);
+        fmter.field("flags", &self.flags);
+
+        fmter.field("proof_len", &self.proof.zcash_serialized_size());
+
+        fmter.field("shared_anchor", &self.shared_anchor);
+
+        fmter.finish()
+    }
+}
+
 impl ShieldedData {
     /// Iterate over the [`Action`]s for the [`AuthorizedAction`]s in this
     /// transaction, in the order they appear in it.

zebra-chain/src/sapling/shielded_data.rs

@@ -23,7 +23,7 @@ use crate::{
 
 use std::{
     cmp::{max, Eq, PartialEq},
-    fmt::Debug,
+    fmt::{self, Debug},
 };
 
 /// Per-Spend Sapling anchors, used in Transaction V4 and the
@@ -179,6 +179,27 @@ where
     },
 }
 
+impl<AnchorV> fmt::Display for ShieldedData<AnchorV>
+where
+    AnchorV: AnchorVariant + Clone,
+{
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        let mut fmter = f.debug_struct(
+            format!(
+                "sapling::ShieldedData<{}>",
+                std::any::type_name::<AnchorV>()
+            )
+            .as_str(),
+        );
+
+        fmter.field("spends", &self.transfers.spends().count());
+        fmter.field("outputs", &self.transfers.outputs().count());
+        fmter.field("value_balance", &self.value_balance);
+
+        fmter.finish()
+    }
+}
+
 impl<AnchorV> ShieldedData<AnchorV>
 where
     AnchorV: AnchorVariant + Clone,

zebra-chain/src/sapling/spend.rs

@@ -3,7 +3,7 @@
 //! Zebra uses a generic spend type for `V4` and `V5` transactions.
 //! The anchor change is handled using the `AnchorVariant` type trait.
 
-use std::io;
+use std::{fmt, io};
 
 use crate::{
     block::MAX_BLOCK_BYTES,
@@ -73,6 +73,21 @@ pub struct SpendPrefixInTransactionV5 {
     pub rk: redjubjub::VerificationKeyBytes<SpendAuth>,
 }
 
+impl<AnchorV> fmt::Display for Spend<AnchorV>
+where
+    AnchorV: AnchorVariant + Clone,
+{
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        let mut fmter = f
+            .debug_struct(format!("sapling::Spend<{}>", std::any::type_name::<AnchorV>()).as_str());
+
+        fmter.field("per_spend_anchor", &self.per_spend_anchor);
+        fmter.field("nullifier", &self.nullifier);
+
+        fmter.finish()
+    }
+}
+
 impl From<(Spend<SharedAnchor>, tree::Root)> for Spend<PerSpendAnchor> {
     /// Convert a `Spend<SharedAnchor>` and its shared anchor, into a
     /// `Spend<PerSpendAnchor>`.

zebra-chain/src/transaction.rs

@@ -40,7 +40,7 @@ use crate::{
     value_balance::{ValueBalance, ValueBalanceError},
 };
 
-use std::{collections::HashMap, iter};
+use std::{collections::HashMap, fmt, iter};
 
 /// A Zcash transaction.
 ///
@@ -131,6 +131,28 @@ pub enum Transaction {
     },
 }
 
+impl fmt::Display for Transaction {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        let mut fmter = f.debug_struct("Transaction");
+
+        fmter.field("version", &self.version());
+        if let Some(network_upgrade) = self.network_upgrade() {
+            fmter.field("network_upgrade", &network_upgrade);
+        }
+
+        fmter.field("transparent_inputs", &self.inputs().len());
+        fmter.field("transparent_outputs", &self.outputs().len());
+        fmter.field("sprout_joinsplits", &self.joinsplit_count());
+        fmter.field("sapling_spends", &self.sapling_spends_per_anchor().count());
+        fmter.field("sapling_outputs", &self.sapling_outputs().count());
+        fmter.field("orchard_actions", &self.orchard_actions().count());
+
+        fmter.field("unmined_id", &self.unmined_id());
+
+        fmter.finish()
+    }
+}
+
 impl Transaction {
     // identifiers and hashes
 

zebra-chain/src/transaction/joinsplit.rs

@@ -1,3 +1,5 @@
+use std::fmt;
+
 use serde::{Deserialize, Serialize};
 
 use crate::{
@@ -46,6 +48,18 @@ pub struct JoinSplitData<P: ZkSnarkProof> {
     pub sig: ed25519::Signature,
 }
 
+impl<P: ZkSnarkProof> fmt::Display for JoinSplitData<P> {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        let mut fmter =
+            f.debug_struct(format!("JoinSplitData<{}>", std::any::type_name::<P>()).as_str());
+
+        fmter.field("joinsplits", &self.joinsplits().count());
+        fmter.field("value_balance", &self.value_balance());
+
+        fmter.finish()
+    }
+}
+
 impl<P: ZkSnarkProof> JoinSplitData<P> {
     /// Iterate over the [`JoinSplit`]s in `self`, in the order they appear
     /// in the transaction.

zebra-chain/src/transparent.rs

@@ -33,7 +33,7 @@ use crate::{
     block, transaction,
 };
 
-use std::{collections::HashMap, iter};
+use std::{collections::HashMap, fmt, iter};
 
 /// The maturity threshold for transparent coinbase outputs.
 ///
@@ -123,6 +123,33 @@ pub enum Input {
     },
 }
 
+impl fmt::Display for Input {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        match self {
+            Input::PrevOut {
+                outpoint,
+                unlock_script,
+                ..
+            } => {
+                let mut fmter = f.debug_struct("transparent::Input::PrevOut");
+
+                fmter.field("unlock_script_len", &unlock_script.as_raw_bytes().len());
+                fmter.field("outpoint", outpoint);
+
+                fmter.finish()
+            }
+            Input::Coinbase { height, data, .. } => {
+                let mut fmter = f.debug_struct("transparent::Input::Coinbase");
+
+                fmter.field("height", height);
+                fmter.field("data_len", &data.0.len());
+
+                fmter.finish()
+            }
+        }
+    }
+}
+
 impl Input {
     /// If this is a `PrevOut` input, returns this input's outpoint.
     /// Otherwise, returns `None`.

zebrad/proptest-regressions/components/mempool/storage/tests/prop.txt

@@ -0,0 +1,7 @@
+# Seeds for failure cases proptest has generated in the past. It is
+# automatically read and these particular cases re-run before any
+# novel cases are generated.
+#
+# It is recommended to check this file in to source control so that
+# everyone who runs the test benefits from these saved cases.
+cc b258d507c0b2aef6c2793bdb3fc29e6367e62fba9df378ea6797e9bc97fd2780 # shrinks to input = RemoveSameEffects { transactions: alloc::vec::Vec<zebra_chain::transaction::unmined::UnminedTx><zebra_chain::transaction::unmined::UnminedTx>, len=2, mined_ids_to_remove: std::collections::hash::set::HashSet<zebra_chain::transaction::hash::Hash><zebra_chain::transaction::hash::Hash>, len=2 }

zebrad/src/components/mempool.rs

@@ -81,6 +81,7 @@ pub enum Response {
 ///
 /// Indicates wether it is enabled or disabled and, if enabled, contains
 /// the necessary data to run it.
+#[allow(clippy::large_enum_variant)]
 enum ActiveState {
     /// The Mempool is disabled.
     Disabled,