change(rpc): Sort transaction hashes like zcashd in getrawmempool RPC response (#5994)

* Sorts transactions like zcashd in getrawmempool

* Simplifies sort logic and condenses duplicate code

* adds comment clarifying the intended byte order for transaction hashes

* Multiplies fee by MAX_BLOCK_BYTES/tx-size instead of tx-size

- Removes feature flag on get_raw_mempool

* Apply suggestions from code review

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

* Always uses `TransactionIds` request in tests

* reverts switch from #[cfg()] to cfg!()

* Adds feature flag to constant

* Updates tests and removes !cfg(not(test))

* Moves up comment

* adds missing transaction_ids

Co-authored-by: teor <teor@riseup.net>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

zebra-chain/src/transaction/hash.rs

@@ -60,7 +60,7 @@ use super::{txid::TxIdBuilder, AuthDigest, Transaction};
 ///
 /// [ZIP-244]: https://zips.z.cash/zip-0244
 /// [Spec: Transaction Identifiers]: https://zips.z.cash/protocol/protocol.pdf#txnidentifiers
-#[derive(Copy, Clone, Eq, PartialEq, Serialize, Deserialize, Hash)]
+#[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Serialize, Deserialize, Hash)]
 #[cfg_attr(any(test, feature = "proptest-impl"), derive(Arbitrary))]
 pub struct Hash(pub [u8; 32]);
 

zebra-rpc/src/methods.rs

@@ -647,9 +647,24 @@ where
 
     // TODO: use a generic error constructor (#5548)
     fn get_raw_mempool(&self) -> BoxFuture<Result<Vec<String>>> {
+        #[cfg(feature = "getblocktemplate-rpcs")]
+        use zebra_chain::block::MAX_BLOCK_BYTES;
+
+        #[cfg(feature = "getblocktemplate-rpcs")]
+        /// Determines whether the output of this RPC is sorted like zcashd
+        const SHOULD_USE_ZCASHD_ORDER: bool = true;
+
         let mut mempool = self.mempool.clone();
 
         async move {
+            #[cfg(feature = "getblocktemplate-rpcs")]
+            let request = if SHOULD_USE_ZCASHD_ORDER {
+                mempool::Request::FullTransactions
+            } else {
+                mempool::Request::TransactionIds
+            };
+
+            #[cfg(not(feature = "getblocktemplate-rpcs"))]
             let request = mempool::Request::TransactionIds;
 
             // `zcashd` doesn't check if it is synced to the tip here, so we don't either.
@@ -664,6 +679,28 @@ where
                 })?;
 
             match response {
+                #[cfg(feature = "getblocktemplate-rpcs")]
+                mempool::Response::FullTransactions(mut transactions) => {
+                    // Sort transactions in descending order by fee/size, using hash in serialized byte order as a tie-breaker
+                    transactions.sort_by_cached_key(|tx| {
+                        // zcashd uses modified fee here but Zebra doesn't currently
+                        // support prioritizing transactions
+                        std::cmp::Reverse((
+                            i64::from(tx.miner_fee) as u128 * MAX_BLOCK_BYTES as u128
+                                / tx.transaction.size as u128,
+                            // transaction hashes are compared in their serialized byte-order.
+                            tx.transaction.id.mined_id(),
+                        ))
+                    });
+
+                    let tx_ids: Vec<String> = transactions
+                        .iter()
+                        .map(|unmined_tx| unmined_tx.transaction.id.mined_id().encode_hex())
+                        .collect();
+
+                    Ok(tx_ids)
+                }
+
                 mempool::Response::TransactionIds(unmined_transaction_ids) => {
                     let mut tx_ids: Vec<String> = unmined_transaction_ids
                         .iter()
@@ -671,11 +708,11 @@ where
                         .collect();
 
                     // Sort returned transaction IDs in numeric/string order.
-                    // (zcashd's sort order appears arbitrary.)
                     tx_ids.sort();
 
                     Ok(tx_ids)
                 }
+
                 _ => unreachable!("unmatched response to a transactionids request"),
             }
         }

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

@@ -18,7 +18,7 @@ use zebra_chain::{
         NetworkUpgrade,
     },
     serialization::{ZcashDeserialize, ZcashSerialize},
-    transaction::{self, Transaction, UnminedTx, UnminedTxId},
+    transaction::{self, Transaction, UnminedTx, VerifiedUnminedTx},
     transparent,
 };
 use zebra_node_services::mempool;
@@ -313,7 +313,7 @@ proptest! {
     /// Make the mock mempool service return a list of transaction IDs, and check that the RPC call
     /// returns those IDs as hexadecimal strings.
     #[test]
-    fn mempool_transactions_are_sent_to_caller(transaction_ids in any::<HashSet<UnminedTxId>>()) {
+    fn mempool_transactions_are_sent_to_caller(transactions in any::<Vec<VerifiedUnminedTx>>()) {
         let (runtime, _init_guard) = zebra_test::init_async();
         let _guard = runtime.enter();
 
@@ -334,16 +334,56 @@ proptest! {
             );
 
             let call_task = tokio::spawn(rpc.get_raw_mempool());
-            let mut expected_response: Vec<String> = transaction_ids
-                .iter()
-                .map(|id| id.mined_id().encode_hex())
-                .collect();
-            expected_response.sort();
 
-            mempool
-                .expect_request(mempool::Request::TransactionIds)
-                .await?
-                .respond(mempool::Response::TransactionIds(transaction_ids));
+
+            #[cfg(not(feature = "getblocktemplate-rpcs"))]
+            let expected_response = {
+                let transaction_ids: HashSet<_> = transactions
+                    .iter()
+                    .map(|tx| tx.transaction.id)
+                    .collect();
+
+                let mut expected_response: Vec<String> = transaction_ids
+                    .iter()
+                    .map(|id| id.mined_id().encode_hex())
+                    .collect();
+                expected_response.sort();
+
+                mempool
+                    .expect_request(mempool::Request::TransactionIds)
+                    .await?
+                    .respond(mempool::Response::TransactionIds(transaction_ids));
+
+                expected_response
+            };
+
+            // Note: this depends on `SHOULD_USE_ZCASHD_ORDER` being true.
+            #[cfg(feature = "getblocktemplate-rpcs")]
+            let expected_response = {
+                let mut expected_response = transactions.clone();
+                expected_response.sort_by_cached_key(|tx| {
+                    // zcashd uses modified fee here but Zebra doesn't currently
+                    // support prioritizing transactions
+                    std::cmp::Reverse((
+                        i64::from(tx.miner_fee) as u128 * zebra_chain::block::MAX_BLOCK_BYTES as u128
+                            / tx.transaction.size as u128,
+                        // transaction hashes are compared in their serialized byte-order.
+                        tx.transaction.id.mined_id(),
+                    ))
+                });
+
+                let expected_response = expected_response
+                    .iter()
+                    .map(|tx| tx.transaction.id.mined_id().encode_hex())
+                    .collect();
+
+                mempool
+                    .expect_request(mempool::Request::FullTransactions)
+                    .await?
+                    .respond(mempool::Response::FullTransactions(transactions));
+
+                expected_response
+            };
 
             mempool.expect_no_requests().await?;
             state.expect_no_requests().await?;

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

@@ -129,6 +129,15 @@ async fn test_rpc_response_data_for_network(network: Network) {
     // - a request to get all mempool transactions will be made by `getrawmempool` behind the scenes.
     // - 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.
+    // Note: this depends on `SHOULD_USE_ZCASHD_ORDER` being true.
+    #[cfg(feature = "getblocktemplate-rpcs")]
+    let mempool_req = mempool
+        .expect_request_that(|request| matches!(request, mempool::Request::FullTransactions))
+        .map(|responder| {
+            responder.respond(mempool::Response::FullTransactions(vec![]));
+        });
+
+    #[cfg(not(feature = "getblocktemplate-rpcs"))]
     let mempool_req = mempool
         .expect_request_that(|request| matches!(request, mempool::Request::TransactionIds))
         .map(|responder| {