3. change(rpc): Add fee and sigops fields to getblocktemplate transactions (#5508)

* Add a legacy_sigop_count field to VerifiedUnminedTx

* Add conversions from Vec<VerifiedUnminedTx> to block header roots

* Add fee and sigops field to block template transactions

* Fix up mempool request names

* Increase existing snapshot test coverage

* Document a new method parameter

Co-authored-by: Alfredo Garcia <oxarbitrage@gmail.com>

Co-authored-by: Alfredo Garcia <oxarbitrage@gmail.com>

zebra-chain/src/block/merkle.rs

@@ -6,7 +6,7 @@ use hex::{FromHex, ToHex};
 
 use crate::{
     serialization::sha256d,
-    transaction::{self, Transaction, UnminedTx, UnminedTxId},
+    transaction::{self, Transaction, UnminedTx, UnminedTxId, VerifiedUnminedTx},
 };
 
 #[cfg(any(any(test, feature = "proptest-impl"), feature = "proptest-impl"))]
@@ -204,6 +204,18 @@ impl std::iter::FromIterator<UnminedTxId> for Root {
     }
 }
 
+impl std::iter::FromIterator<VerifiedUnminedTx> for Root {
+    fn from_iter<I>(transactions: I) -> Self
+    where
+        I: IntoIterator<Item = VerifiedUnminedTx>,
+    {
+        transactions
+            .into_iter()
+            .map(|tx| tx.transaction.id.mined_id())
+            .collect()
+    }
+}
+
 impl std::iter::FromIterator<transaction::Hash> for Root {
     /// # Panics
     ///
@@ -351,6 +363,23 @@ impl std::iter::FromIterator<UnminedTx> for AuthDataRoot {
     }
 }
 
+impl std::iter::FromIterator<VerifiedUnminedTx> for AuthDataRoot {
+    fn from_iter<I>(transactions: I) -> Self
+    where
+        I: IntoIterator<Item = VerifiedUnminedTx>,
+    {
+        transactions
+            .into_iter()
+            .map(|tx| {
+                tx.transaction
+                    .id
+                    .auth_digest()
+                    .unwrap_or(AUTH_DIGEST_PLACEHOLDER)
+            })
+            .collect()
+    }
+}
+
 impl std::iter::FromIterator<UnminedTxId> for AuthDataRoot {
     fn from_iter<I>(tx_ids: I) -> Self
     where

zebra-chain/src/transaction/unmined.rs

@@ -284,6 +284,10 @@ pub struct VerifiedUnminedTx {
 
     /// The transaction fee for this unmined transaction.
     pub miner_fee: Amount<NonNegative>,
+
+    /// The number of legacy signature operations in this transaction's
+    /// transparent inputs and outputs.
+    pub legacy_sigop_count: u64,
 }
 
 impl fmt::Display for VerifiedUnminedTx {
@@ -291,16 +295,22 @@ impl fmt::Display for VerifiedUnminedTx {
         f.debug_struct("VerifiedUnminedTx")
             .field("transaction", &self.transaction)
             .field("miner_fee", &self.miner_fee)
+            .field("legacy_sigop_count", &self.legacy_sigop_count)
             .finish()
     }
 }
 
 impl VerifiedUnminedTx {
-    /// Create a new verified unmined transaction from a transaction and its fee.
+    /// Create a new verified unmined transaction from a transaction, its fee and the legacy sigop count.
-    pub fn new(transaction: UnminedTx, miner_fee: Amount<NonNegative>) -> Self {
+    pub fn new(
+        transaction: UnminedTx,
+        miner_fee: Amount<NonNegative>,
+        legacy_sigop_count: u64,
+    ) -> Self {
         Self {
             transaction,
             miner_fee,
+            legacy_sigop_count,
         }
     }
 

zebra-consensus/src/block.rs

@@ -229,9 +229,7 @@ where
                     response
                 );
 
-                legacy_sigop_count += response
+                legacy_sigop_count += response.legacy_sigop_count();
-                    .legacy_sigop_count()
-                    .expect("block transaction responses must have a legacy sigop count");
 
                 // Coinbase transactions consume the miner fee,
                 // so they don't add any value to the block's total miner fee.

zebra-consensus/src/transaction.rs

@@ -249,7 +249,9 @@ impl Response {
 
     /// The miner fee for the transaction in this response.
     ///
-    /// Coinbase transactions do not have a miner fee.
+    /// Coinbase transactions do not have a miner fee,
+    /// and they don't need UTXOs to calculate their value balance,
+    /// because they don't spend any inputs.
     pub fn miner_fee(&self) -> Option<Amount<NonNegative>> {
         match self {
             Response::Block { miner_fee, .. } => *miner_fee,
@@ -259,15 +261,12 @@ impl Response {
 
     /// The number of legacy transparent signature operations in this transaction's
     /// inputs and outputs.
-    ///
+    pub fn legacy_sigop_count(&self) -> u64 {
-    /// Zebra does not check the legacy sigop count for mempool transactions,
-    /// because it is a standard rule (not a consensus rule).
-    pub fn legacy_sigop_count(&self) -> Option<u64> {
         match self {
             Response::Block {
                 legacy_sigop_count, ..
-            } => Some(*legacy_sigop_count),
+            } => *legacy_sigop_count,
-            Response::Mempool { .. } => None,
+            Response::Mempool { transaction, .. } => transaction.legacy_sigop_count,
         }
     }
 
@@ -420,11 +419,13 @@ where
                 };
             }
 
+            let legacy_sigop_count = cached_ffi_transaction.legacy_sigop_count()?;
+
             let rsp = match req {
                 Request::Block { .. } => Response::Block {
                     tx_id,
                     miner_fee,
-                    legacy_sigop_count: cached_ffi_transaction.legacy_sigop_count()?,
+                    legacy_sigop_count,
                 },
                 Request::Mempool { transaction, .. } => Response::Mempool {
                     transaction: VerifiedUnminedTx::new(
@@ -432,6 +433,7 @@ where
                         miner_fee.expect(
                             "unexpected mempool coinbase transaction: should have already rejected",
                         ),
+                        legacy_sigop_count,
                     ),
                 },
             };

zebra-node-services/src/mempool.rs

@@ -4,7 +4,10 @@
 
 use std::collections::HashSet;
 
-use zebra_chain::transaction::{Hash, UnminedTx, UnminedTxId};
+use zebra_chain::transaction::{self, UnminedTx, UnminedTxId};
+
+#[cfg(feature = "getblocktemplate-rpcs")]
+use zebra_chain::transaction::VerifiedUnminedTx;
 
 use crate::BoxError;
 
@@ -22,24 +25,28 @@ pub use self::gossip::Gossip;
 /// because all mempool transactions must be verified.
 #[derive(Debug, Eq, PartialEq)]
 pub enum Request {
-    /// Query all transaction IDs in the mempool.
+    /// Query all [`UnminedTxId`]s in the mempool.
     TransactionIds,
 
-    /// Query matching  transactions in the mempool,
+    /// Query matching [`UnminedTx`] in the mempool,
     /// using a unique set of [`UnminedTxId`]s.
     TransactionsById(HashSet<UnminedTxId>),
 
-    /// Query matching  transactions in the mempool,
+    /// Query matching [`UnminedTx`] in the mempool,
-    /// using a unique set of [`struct@Hash`]s. Pre-V5 transactions are matched
+    /// using a unique set of [`transaction::Hash`]es. Pre-V5 transactions are matched
     /// directly; V5 transaction are matched just by the Hash, disregarding
     /// the [`AuthDigest`](zebra_chain::transaction::AuthDigest).
-    TransactionsByMinedId(HashSet<Hash>),
+    TransactionsByMinedId(HashSet<transaction::Hash>),
 
-    /// Get all the transactions in the mempool.
+    /// Get all the [`VerifiedUnminedTx`] in the mempool.
     ///
-    /// Equivalent to `TransactionsById(TransactionIds)`.
+    /// Equivalent to `TransactionsById(TransactionIds)`,
+    /// but each transaction also includes the `miner_fee` and `legacy_sigop_count` fields.
+    //
+    // TODO: make the Transactions response return VerifiedUnminedTx,
+    //       and remove the FullTransactions variant
     #[cfg(feature = "getblocktemplate-rpcs")]
-    Transactions,
+    FullTransactions,
 
     /// Query matching cached rejected transaction IDs in the mempool,
     /// using a unique set of [`UnminedTxId`]s.
@@ -80,10 +87,10 @@ pub enum Request {
 /// confirm that the mempool has been checked for newly verified transactions.
 #[derive(Debug)]
 pub enum Response {
-    /// Returns all transaction IDs from the mempool.
+    /// Returns all [`UnminedTxId`]s from the mempool.
     TransactionIds(HashSet<UnminedTxId>),
 
-    /// Returns matching transactions from the mempool.
+    /// Returns matching [`UnminedTx`] from the mempool.
     ///
     /// Since the [`Request::TransactionsById`] request is unique,
     /// the response transactions are also unique. The same applies to
@@ -91,7 +98,14 @@ pub enum Response {
     /// different transactions with different mined IDs.
     Transactions(Vec<UnminedTx>),
 
-    /// Returns matching cached rejected transaction IDs from the mempool,
+    /// Returns all [`VerifiedUnminedTx`] in the mempool.
+    //
+    // TODO: make the Transactions response return VerifiedUnminedTx,
+    //       and remove the FullTransactions variant
+    #[cfg(feature = "getblocktemplate-rpcs")]
+    FullTransactions(Vec<VerifiedUnminedTx>),
+
+    /// Returns matching cached rejected [`UnminedTxId`]s from the mempool,
     RejectedTransactionIds(HashSet<UnminedTxId>),
 
     /// Returns a list of queue results.

zebra-rpc/src/methods/get_block_template_rpcs.rs

@@ -214,18 +214,18 @@ where
         // Since this is a very large RPC, we use separate functions for each group of fields.
         async move {
             // TODO: put this in a separate get_mempool_transactions() function
-            let request = mempool::Request::Transactions;
+            let request = mempool::Request::FullTransactions;
             let response = mempool.oneshot(request).await.map_err(|error| Error {
                 code: ErrorCode::ServerError(0),
                 message: error.to_string(),
                 data: None,
             })?;
 
-            let transactions = if let mempool::Response::Transactions(transactions) = response {
+            let transactions = if let mempool::Response::FullTransactions(transactions) = response {
                 // TODO: select transactions according to ZIP-317 (#5473)
                 transactions
             } else {
-                unreachable!("unmatched response to a mempool::Transactions request");
+                unreachable!("unmatched response to a mempool::FullTransactions request");
             };
 
             let merkle_root;

zebra-rpc/src/methods/get_block_template_rpcs/types/transaction.rs

@@ -3,7 +3,7 @@
 use zebra_chain::{
     amount::{self, Amount, NonNegative},
     block::merkle::AUTH_DIGEST_PLACEHOLDER,
-    transaction::{self, SerializedTransaction, UnminedTx},
+    transaction::{self, SerializedTransaction, VerifiedUnminedTx},
 };
 
 /// Transaction data and fields needed to generate blocks using the `getblocktemplate` RPC.
@@ -39,13 +39,9 @@ where
     /// Non-coinbase transactions must be `NonNegative`.
     /// The Coinbase transaction `fee` is the negative sum of the fees of the transactions in
     /// the block, so their fee must be `NegativeOrZero`.
-    //
-    // TODO: add a fee field to mempool transactions, based on the verifier response.
     pub(crate) fee: Amount<FeeConstraint>,
 
     /// The number of transparent signature operations in this transaction.
-    //
-    // TODO: add a sigops field to mempool transactions, based on the verifier response.
     pub(crate) sigops: u64,
 
     /// Is this transaction required in the block?
@@ -55,21 +51,23 @@ where
 }
 
 // Convert from a mempool transaction to a transaction template.
-impl From<&UnminedTx> for TransactionTemplate<NonNegative> {
+impl From<&VerifiedUnminedTx> for TransactionTemplate<NonNegative> {
-    fn from(tx: &UnminedTx) -> Self {
+    fn from(tx: &VerifiedUnminedTx) -> Self {
         Self {
-            data: tx.transaction.as_ref().into(),
+            data: tx.transaction.transaction.as_ref().into(),
-            hash: tx.id.mined_id(),
+            hash: tx.transaction.id.mined_id(),
-            auth_digest: tx.id.auth_digest().unwrap_or(AUTH_DIGEST_PLACEHOLDER),
+            auth_digest: tx
+                .transaction
+                .id
+                .auth_digest()
+                .unwrap_or(AUTH_DIGEST_PLACEHOLDER),
 
             // Always empty, not supported by Zebra's mempool.
             depends: Vec::new(),
 
-            // TODO: add a fee field to mempool transactions, based on the verifier response.
+            fee: tx.miner_fee,
-            fee: Amount::zero(),
 
-            // TODO: add a sigops field to mempool transactions, based on the verifier response.
+            sigops: tx.legacy_sigop_count,
-            sigops: 0,
 
             // Zebra does not require any transactions except the coinbase transaction.
             required: false,
@@ -77,8 +75,8 @@ impl From<&UnminedTx> for TransactionTemplate<NonNegative> {
     }
 }
 
-impl From<UnminedTx> for TransactionTemplate<NonNegative> {
+impl From<VerifiedUnminedTx> for TransactionTemplate<NonNegative> {
-    fn from(tx: UnminedTx) -> Self {
+    fn from(tx: VerifiedUnminedTx) -> Self {
         Self::from(&tx)
     }
 }

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

@@ -128,7 +128,7 @@ async fn test_rpc_response_data_for_network(network: Network) {
     // - as we have the mempool mocked we need to expect a request and wait for a response,
     // which will be an empty mempool in this case.
     let mempool_req = mempool
-        .expect_request_that(|_request| true)
+        .expect_request_that(|request| matches!(request, mempool::Request::TransactionIds))
         .map(|responder| {
             responder.respond(mempool::Response::TransactionIds(
                 std::collections::HashSet::new(),
@@ -152,9 +152,11 @@ async fn test_rpc_response_data_for_network(network: Network) {
     // `getrawtransaction`
     //
     // - similar to `getrawmempool` described above, a mempool request will be made to get the requested
-    // transaction from the mempoo, response will be empty as we have this transaction in state
+    // transaction from the mempool, response will be empty as we have this transaction in state
     let mempool_req = mempool
-        .expect_request_that(|_request| true)
+        .expect_request_that(|request| {
+            matches!(request, mempool::Request::TransactionsByMinedId(_))
+        })
         .map(|responder| {
             responder.respond(mempool::Response::Transactions(vec![]));
         });

zebra-rpc/src/methods/tests/snapshot/get_block_template_rpcs.rs

@@ -61,9 +61,9 @@ pub async fn test_responses<State>(
     let get_block_template = tokio::spawn(get_block_template_rpc.get_block_template());
 
     mempool
-        .expect_request(mempool::Request::Transactions)
+        .expect_request(mempool::Request::FullTransactions)
         .await
-        .respond(mempool::Response::Transactions(vec![]));
+        .respond(mempool::Response::FullTransactions(vec![]));
 
     let get_block_template = get_block_template
         .await

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

@@ -758,9 +758,9 @@ async fn rpc_getblocktemplate() {
     let get_block_template = tokio::spawn(get_block_template_rpc.get_block_template());
 
     mempool
-        .expect_request(mempool::Request::Transactions)
+        .expect_request(mempool::Request::FullTransactions)
         .await
-        .respond(mempool::Response::Transactions(vec![]));
+        .respond(mempool::Response::FullTransactions(vec![]));
 
     let get_block_template = get_block_template
         .await

zebrad/src/components/inbound/tests/fake_peer_set.rs

@@ -166,10 +166,11 @@ async fn mempool_push_transaction() -> Result<(), crate::BoxError> {
             .into_mempool_transaction()
             .expect("unexpected non-mempool request");
 
-        // Set a dummy fee.
+        // Set a dummy fee and sigops.
         responder.respond(transaction::Response::from(VerifiedUnminedTx::new(
             transaction,
             Amount::zero(),
+            0,
         )));
     });
 
@@ -269,10 +270,11 @@ async fn mempool_advertise_transaction_ids() -> Result<(), crate::BoxError> {
             .into_mempool_transaction()
             .expect("unexpected non-mempool request");
 
-        // Set a dummy fee.
+        // Set a dummy fee and sigops.
         responder.respond(transaction::Response::from(VerifiedUnminedTx::new(
             transaction,
             Amount::zero(),
+            0,
         )));
     });
 
@@ -369,10 +371,11 @@ async fn mempool_transaction_expiration() -> Result<(), crate::BoxError> {
             .into_mempool_transaction()
             .expect("unexpected non-mempool request");
 
-        // Set a dummy fee.
+        // Set a dummy fee and sigops.
         responder.respond(transaction::Response::from(VerifiedUnminedTx::new(
             transaction,
             Amount::zero(),
+            0,
         )));
     });
 
@@ -502,10 +505,11 @@ async fn mempool_transaction_expiration() -> Result<(), crate::BoxError> {
             .into_mempool_transaction()
             .expect("unexpected non-mempool request");
 
-        // Set a dummy fee.
+        // Set a dummy fee and sigops.
         responder.respond(transaction::Response::from(VerifiedUnminedTx::new(
             transaction,
             Amount::zero(),
+            0,
         )));
     });
 

zebrad/src/components/mempool.rs

@@ -446,15 +446,16 @@ impl Service<Request> for Mempool {
 
                     async move { Ok(Response::Transactions(res)) }.boxed()
                 }
+
                 #[cfg(feature = "getblocktemplate-rpcs")]
-                Request::Transactions => {
+                Request::FullTransactions => {
                     trace!(?req, "got mempool request");
 
-                    let res: Vec<_> = storage.transactions().cloned().collect();
+                    let res: Vec<_> = storage.full_transactions().cloned().collect();
 
                     trace!(?req, res_count = ?res.len(), "answered mempool request");
 
-                    async move { Ok(Response::Transactions(res)) }.boxed()
+                    async move { Ok(Response::FullTransactions(res)) }.boxed()
                 }
 
                 Request::RejectedTransactionIds(ref ids) => {
@@ -501,19 +502,19 @@ impl Service<Request> for Mempool {
                 // by the peer connection handler. Therefore, return successful
                 // empty responses.
                 let resp = match req {
-                    // Empty Queries
+                    // Return empty responses for queries.
                     Request::TransactionIds => Response::TransactionIds(Default::default()),
 
                     Request::TransactionsById(_) => Response::Transactions(Default::default()),
                     Request::TransactionsByMinedId(_) => Response::Transactions(Default::default()),
                     #[cfg(feature = "getblocktemplate-rpcs")]
-                    Request::Transactions => Response::Transactions(Default::default()),
+                    Request::FullTransactions => Response::FullTransactions(Default::default()),
 
                     Request::RejectedTransactionIds(_) => {
                         Response::RejectedTransactionIds(Default::default())
                     }
 
-                    // Don't queue mempool candidates
+                    // Don't queue mempool candidates, because there is no queue.
                     Request::Queue(gossiped_txs) => Response::Queued(
                         // Special case; we can signal the error inside the response,
                         // because the inbound service ignores inner errors.

zebrad/src/components/mempool/storage.rs

@@ -410,11 +410,23 @@ impl Storage {
         self.verified.transactions().map(|tx| tx.id)
     }
 
-    /// Returns the set of [`UnminedTx`]s in the mempool.
+    /// Returns an iterator over the [`UnminedTx`]s in the mempool.
+    //
+    // TODO: make the transactions() method return VerifiedUnminedTx,
+    //       and remove the full_transactions() method
     pub fn transactions(&self) -> impl Iterator<Item = &UnminedTx> {
         self.verified.transactions()
     }
 
+    /// Returns an iterator over the [`VerifiedUnminedTx`] in the set.
+    ///
+    /// Each [`VerifiedUnminedTx`] contains an [`UnminedTx`],
+    /// and adds extra fields from the transaction verifier result.
+    #[allow(dead_code)]
+    pub fn full_transactions(&self) -> impl Iterator<Item = &VerifiedUnminedTx> + '_ {
+        self.verified.full_transactions()
+    }
+
     /// Returns the number of transactions in the mempool.
     #[allow(dead_code)]
     pub fn transaction_count(&self) -> usize {

zebrad/src/components/mempool/storage/tests.rs

@@ -1,3 +1,5 @@
+//! Tests and test utility functions for mempool storage.
+
 use std::ops::RangeBounds;
 
 use zebra_chain::{
@@ -30,9 +32,10 @@ pub fn unmined_transactions_in_blocks(
         });
 
     // Extract the transactions from the blocks and wrap each one as an unmined transaction.
-    // Use a fake zero miner fee, because we don't have the UTXOs to calculate the correct fee.
+    // Use a fake zero miner fee and sigops, because we don't have the UTXOs to calculate
+    // the correct fee.
     selected_blocks
         .flat_map(|block| block.transactions)
         .map(UnminedTx::from)
-        .map(|transaction| VerifiedUnminedTx::new(transaction, Amount::zero()))
+        .map(|transaction| VerifiedUnminedTx::new(transaction, Amount::zero(), 0))
 }

zebrad/src/components/mempool/storage/tests/prop.rs

@@ -1,4 +1,6 @@
-use std::{collections::HashSet, convert::TryFrom, env, fmt::Debug, thread, time::Duration};
+//! Randomised property tests for mempool storage.
+
+use std::{collections::HashSet, env, fmt::Debug, thread, time::Duration};
 
 use proptest::{collection::vec, prelude::*};
 use proptest_derive::Arbitrary;
@@ -473,8 +475,8 @@ impl SpendConflictTestInput {
         };
 
         (
-            VerifiedUnminedTx::new(first.0.into(), Amount::zero()),
+            VerifiedUnminedTx::new(first.0.into(), Amount::zero(), 0),
-            VerifiedUnminedTx::new(second.0.into(), Amount::zero()),
+            VerifiedUnminedTx::new(second.0.into(), Amount::zero(), 0),
         )
     }
 
@@ -491,8 +493,8 @@ impl SpendConflictTestInput {
         Self::remove_orchard_conflicts(&mut first, &mut second);
 
         (
-            VerifiedUnminedTx::new(first.0.into(), Amount::zero()),
+            VerifiedUnminedTx::new(first.0.into(), Amount::zero(), 0),
-            VerifiedUnminedTx::new(second.0.into(), Amount::zero()),
+            VerifiedUnminedTx::new(second.0.into(), Amount::zero(), 0),
         )
     }
 

zebrad/src/components/mempool/storage/tests/vectors.rs

@@ -1,3 +1,5 @@
+//! Fixed test vectors for mempool storage.
+
 use std::iter;
 
 use color_eyre::eyre::Result;
@@ -269,8 +271,8 @@ fn mempool_expired_basic_for_network(network: Network) -> Result<()> {
 
     let tx_id = tx.unmined_id();
 
-    // Insert the transaction into the mempool, with a fake zero miner fee
+    // Insert the transaction into the mempool, with a fake zero miner fee and sigops
-    storage.insert(VerifiedUnminedTx::new(tx.into(), Amount::zero()))?;
+    storage.insert(VerifiedUnminedTx::new(tx.into(), Amount::zero(), 0))?;
 
     assert_eq!(storage.transaction_count(), 1);
 

zebrad/src/components/mempool/storage/verified_set.rs

@@ -54,11 +54,22 @@ impl Drop for VerifiedSet {
 }
 
 impl VerifiedSet {
-    /// Returns an iterator over the transactions in the set.
+    /// Returns an iterator over the [`UnminedTx`] in the set.
+    //
+    // TODO: make the transactions() method return VerifiedUnminedTx,
+    //       and remove the full_transactions() method
     pub fn transactions(&self) -> impl Iterator<Item = &UnminedTx> + '_ {
         self.transactions.iter().map(|tx| &tx.transaction)
     }
 
+    /// Returns an iterator over the [`VerifiedUnminedTx`] in the set.
+    ///
+    /// Each [`VerifiedUnminedTx`] contains an [`UnminedTx`],
+    /// and adds extra fields from the transaction verifier result.
+    pub fn full_transactions(&self) -> impl Iterator<Item = &VerifiedUnminedTx> + '_ {
+        self.transactions.iter()
+    }
+
     /// Returns the number of verified transactions in the set.
     pub fn transaction_count(&self) -> usize {
         self.transactions.len()