From 89ebc8be5ea52fc3d55fb44fed5bf6cc78b34866 Mon Sep 17 00:00:00 2001 From: arya2 Date: Wed, 18 Jan 2023 00:27:04 -0500 Subject: [PATCH 01/11] Sorts transactions like zcashd in getrawmempool --- zebra-chain/src/transaction/hash.rs | 2 +- zebra-rpc/src/methods.rs | 58 +++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 1 deletion(-) diff --git a/zebra-chain/src/transaction/hash.rs b/zebra-chain/src/transaction/hash.rs index 2525c1d955c..5c644d0e7e6 100644 --- a/zebra-chain/src/transaction/hash.rs +++ b/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]); diff --git a/zebra-rpc/src/methods.rs b/zebra-rpc/src/methods.rs index 43b53a8e240..ad42ef48733 100644 --- a/zebra-rpc/src/methods.rs +++ b/zebra-rpc/src/methods.rs @@ -645,6 +645,64 @@ where }) } + #[cfg(feature = "getblocktemplate-rpcs")] + // TODO: use a generic error constructor (#5548) + fn get_raw_mempool(&self) -> BoxFuture>> { + use std::cmp::Ordering; + use zebra_chain::transaction::VerifiedUnminedTx; + + /// Returns relative score for sorting [`VerifiedUnminedTx`]s (fee/size) + fn tx_sort_score(tx: &VerifiedUnminedTx, tx2: &VerifiedUnminedTx) -> Option { + u64::try_from(tx2.transaction.size) + .expect("transaction size should fit in u64") + .checked_mul(u64::from(tx.miner_fee)) + } + + let mut mempool = self.mempool.clone(); + + async move { + let request = mempool::Request::FullTransactions; + + // `zcashd` doesn't check if it is synced to the tip here, so we don't either. + let response = mempool + .ready() + .and_then(|service| service.call(request)) + .await + .map_err(|error| Error { + code: ErrorCode::ServerError(0), + message: error.to_string(), + data: None, + })?; + + if let mempool::Response::FullTransactions(mut transactions) = response { + transactions.sort_by(|tx, tx2| { + match tx_sort_score(tx2, tx).cmp(&tx_sort_score(tx, tx2)) { + Ordering::Equal => { + tracing::info!("tx_sort_scores are equal"); + + tx.transaction + .id + .mined_id() + .cmp(&tx2.transaction.id.mined_id()) + } + gt_or_lt => gt_or_lt, + } + }); + + let tx_ids: Vec = transactions + .iter() + .map(|unmined_tx| unmined_tx.transaction.id.mined_id().encode_hex()) + .collect(); + + Ok(tx_ids) + } else { + unreachable!("unmatched response to a mempool::FullTransactions request") + } + } + .boxed() + } + + #[cfg(not(feature = "getblocktemplate-rpcs"))] // TODO: use a generic error constructor (#5548) fn get_raw_mempool(&self) -> BoxFuture>> { let mut mempool = self.mempool.clone(); From cd11c81c67c53a3fbc710f330fabbf9bc2688132 Mon Sep 17 00:00:00 2001 From: arya2 Date: Wed, 18 Jan 2023 17:35:49 -0500 Subject: [PATCH 02/11] Simplifies sort logic and condenses duplicate code --- zebra-rpc/src/methods.rs | 81 +++++++++++++--------------------------- 1 file changed, 26 insertions(+), 55 deletions(-) diff --git a/zebra-rpc/src/methods.rs b/zebra-rpc/src/methods.rs index ad42ef48733..acaa148da2c 100644 --- a/zebra-rpc/src/methods.rs +++ b/zebra-rpc/src/methods.rs @@ -648,66 +648,20 @@ where #[cfg(feature = "getblocktemplate-rpcs")] // TODO: use a generic error constructor (#5548) fn get_raw_mempool(&self) -> BoxFuture>> { - use std::cmp::Ordering; - use zebra_chain::transaction::VerifiedUnminedTx; - - /// Returns relative score for sorting [`VerifiedUnminedTx`]s (fee/size) - fn tx_sort_score(tx: &VerifiedUnminedTx, tx2: &VerifiedUnminedTx) -> Option { - u64::try_from(tx2.transaction.size) - .expect("transaction size should fit in u64") - .checked_mul(u64::from(tx.miner_fee)) - } + /// 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 { - let request = mempool::Request::FullTransactions; - - // `zcashd` doesn't check if it is synced to the tip here, so we don't either. - let response = mempool - .ready() - .and_then(|service| service.call(request)) - .await - .map_err(|error| Error { - code: ErrorCode::ServerError(0), - message: error.to_string(), - data: None, - })?; - - if let mempool::Response::FullTransactions(mut transactions) = response { - transactions.sort_by(|tx, tx2| { - match tx_sort_score(tx2, tx).cmp(&tx_sort_score(tx, tx2)) { - Ordering::Equal => { - tracing::info!("tx_sort_scores are equal"); - - tx.transaction - .id - .mined_id() - .cmp(&tx2.transaction.id.mined_id()) - } - gt_or_lt => gt_or_lt, - } - }); - - let tx_ids: Vec = transactions - .iter() - .map(|unmined_tx| unmined_tx.transaction.id.mined_id().encode_hex()) - .collect(); - - Ok(tx_ids) + #[cfg(feature = "getblocktemplate-rpcs")] + let request = if SHOULD_USE_ZCASHD_ORDER { + mempool::Request::FullTransactions } else { - unreachable!("unmatched response to a mempool::FullTransactions request") - } - } - .boxed() - } - - #[cfg(not(feature = "getblocktemplate-rpcs"))] - // TODO: use a generic error constructor (#5548) - fn get_raw_mempool(&self) -> BoxFuture>> { - let mut mempool = self.mempool.clone(); + mempool::Request::TransactionIds + }; - async move { + #[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. @@ -722,6 +676,23 @@ where })?; match response { + #[cfg(feature = "getblocktemplate-rpcs")] + mempool::Response::FullTransactions(mut transactions) => { + transactions.sort_by_cached_key(|tx| { + std::cmp::Reverse(( + u64::from(tx.miner_fee) / tx.transaction.size as u64, + tx.transaction.id.mined_id(), + )) + }); + + let tx_ids: Vec = 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 = unmined_transaction_ids .iter() @@ -729,11 +700,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"), } } From 25be33d9094bc671fef33e57a354f58214153f1e Mon Sep 17 00:00:00 2001 From: arya2 Date: Wed, 18 Jan 2023 17:40:39 -0500 Subject: [PATCH 03/11] adds comment clarifying the intended byte order for transaction hashes --- zebra-rpc/src/methods.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/zebra-rpc/src/methods.rs b/zebra-rpc/src/methods.rs index acaa148da2c..dfa8780407a 100644 --- a/zebra-rpc/src/methods.rs +++ b/zebra-rpc/src/methods.rs @@ -681,6 +681,7 @@ where transactions.sort_by_cached_key(|tx| { std::cmp::Reverse(( u64::from(tx.miner_fee) / tx.transaction.size as u64, + // transaction hashes are compared in their serialized byte-order. tx.transaction.id.mined_id(), )) }); From 2d7cf5004d277ed2cfbb16d6be58767d79b7e1cb Mon Sep 17 00:00:00 2001 From: arya2 Date: Wed, 18 Jan 2023 20:58:01 -0500 Subject: [PATCH 04/11] Multiplies fee by MAX_BLOCK_BYTES/tx-size instead of tx-size - Removes feature flag on get_raw_mempool --- zebra-rpc/src/methods.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/zebra-rpc/src/methods.rs b/zebra-rpc/src/methods.rs index dfa8780407a..15aef0a8bca 100644 --- a/zebra-rpc/src/methods.rs +++ b/zebra-rpc/src/methods.rs @@ -645,9 +645,10 @@ where }) } - #[cfg(feature = "getblocktemplate-rpcs")] // TODO: use a generic error constructor (#5548) fn get_raw_mempool(&self) -> BoxFuture>> { + use zebra_chain::block::MAX_BLOCK_BYTES; + /// Determines whether the output of this RPC is sorted like zcashd const SHOULD_USE_ZCASHD_ORDER: bool = true; @@ -679,8 +680,11 @@ where #[cfg(feature = "getblocktemplate-rpcs")] mempool::Response::FullTransactions(mut transactions) => { transactions.sort_by_cached_key(|tx| { + // zcashd uses modified fee here but Zebra doesn't currently + // support prioritizing transactions std::cmp::Reverse(( - u64::from(tx.miner_fee) / tx.transaction.size as u64, + 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(), )) From c2097bd5553204c8a399349a47be063e24540b0b Mon Sep 17 00:00:00 2001 From: Arya Date: Wed, 18 Jan 2023 22:57:23 -0500 Subject: [PATCH 05/11] Apply suggestions from code review Co-authored-by: teor --- zebra-rpc/src/methods.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/zebra-rpc/src/methods.rs b/zebra-rpc/src/methods.rs index 15aef0a8bca..f3ca764b0ba 100644 --- a/zebra-rpc/src/methods.rs +++ b/zebra-rpc/src/methods.rs @@ -647,6 +647,7 @@ where // TODO: use a generic error constructor (#5548) fn get_raw_mempool(&self) -> BoxFuture>> { + #[cfg(feature = "getblocktemplate-rpcs")] use zebra_chain::block::MAX_BLOCK_BYTES; /// Determines whether the output of this RPC is sorted like zcashd @@ -655,16 +656,12 @@ where let mut mempool = self.mempool.clone(); async move { - #[cfg(feature = "getblocktemplate-rpcs")] - let request = if SHOULD_USE_ZCASHD_ORDER { + let request = if SHOULD_USE_ZCASHD_ORDER && cfg!(feature = "getblocktemplate-rpcs") { 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. let response = mempool .ready() @@ -679,6 +676,7 @@ 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 From faf73190ab90d3d309dc4de5f9f0fa98cc40efb3 Mon Sep 17 00:00:00 2001 From: arya2 Date: Thu, 19 Jan 2023 03:13:05 -0500 Subject: [PATCH 06/11] reverts switch from #[cfg()] to cfg!() --- zebra-rpc/src/methods.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/zebra-rpc/src/methods.rs b/zebra-rpc/src/methods.rs index f3ca764b0ba..525f2f68f5d 100644 --- a/zebra-rpc/src/methods.rs +++ b/zebra-rpc/src/methods.rs @@ -656,12 +656,16 @@ where let mut mempool = self.mempool.clone(); async move { - let request = if SHOULD_USE_ZCASHD_ORDER && cfg!(feature = "getblocktemplate-rpcs") { + #[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. let response = mempool .ready() From 78a90afbed82e242b2c3a74784f62c04046929f9 Mon Sep 17 00:00:00 2001 From: arya2 Date: Thu, 19 Jan 2023 04:43:45 -0500 Subject: [PATCH 07/11] Always uses `TransactionIds` request in tests --- zebra-rpc/src/methods.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zebra-rpc/src/methods.rs b/zebra-rpc/src/methods.rs index 525f2f68f5d..e690f3f74c3 100644 --- a/zebra-rpc/src/methods.rs +++ b/zebra-rpc/src/methods.rs @@ -657,7 +657,7 @@ where async move { #[cfg(feature = "getblocktemplate-rpcs")] - let request = if SHOULD_USE_ZCASHD_ORDER { + let request = if SHOULD_USE_ZCASHD_ORDER && cfg!(not(test)) { mempool::Request::FullTransactions } else { mempool::Request::TransactionIds From b675f755b981940c39c25f63c06ed8617e795ee3 Mon Sep 17 00:00:00 2001 From: arya2 Date: Thu, 19 Jan 2023 16:27:19 -0500 Subject: [PATCH 08/11] Adds feature flag to constant --- zebra-rpc/src/methods.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/zebra-rpc/src/methods.rs b/zebra-rpc/src/methods.rs index e690f3f74c3..374d9a4637d 100644 --- a/zebra-rpc/src/methods.rs +++ b/zebra-rpc/src/methods.rs @@ -650,6 +650,7 @@ where #[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; From fdeb2efa7e7d9c61f22554cfdcb645d22e47bfa4 Mon Sep 17 00:00:00 2001 From: arya2 Date: Thu, 19 Jan 2023 17:11:05 -0500 Subject: [PATCH 09/11] Updates tests and removes !cfg(not(test)) --- zebra-rpc/src/methods.rs | 2 +- zebra-rpc/src/methods/tests/prop.rs | 57 ++++++++++++++++++++----- zebra-rpc/src/methods/tests/snapshot.rs | 9 ++++ 3 files changed, 56 insertions(+), 12 deletions(-) diff --git a/zebra-rpc/src/methods.rs b/zebra-rpc/src/methods.rs index 374d9a4637d..4331383cf4a 100644 --- a/zebra-rpc/src/methods.rs +++ b/zebra-rpc/src/methods.rs @@ -658,7 +658,7 @@ where async move { #[cfg(feature = "getblocktemplate-rpcs")] - let request = if SHOULD_USE_ZCASHD_ORDER && cfg!(not(test)) { + let request = if SHOULD_USE_ZCASHD_ORDER { mempool::Request::FullTransactions } else { mempool::Request::TransactionIds diff --git a/zebra-rpc/src/methods/tests/prop.rs b/zebra-rpc/src/methods/tests/prop.rs index 01692c343d0..59707c67de8 100644 --- a/zebra-rpc/src/methods/tests/prop.rs +++ b/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::>()) { + fn mempool_transactions_are_sent_to_caller(transactions in any::>()) { let (runtime, _init_guard) = zebra_test::init_async(); let _guard = runtime.enter(); @@ -334,16 +334,51 @@ proptest! { ); let call_task = tokio::spawn(rpc.get_raw_mempool()); - let mut expected_response: Vec = 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 mut expected_response = { + let mut expected_response: Vec = transaction_ids + .iter() + .map(|tx| tx.transaction.id.mined_id().encode_hex()) + .collect(); + expected_response.sort(); + + mempool + .expect_request(mempool::Request::TransactionIds) + .await? + .respond(mempool::Response::TransactionIds(transaction_ids)); + + expected_response + }; + + #[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(); + + // Note: this depends on `SHOULD_USE_ZCASHD_ORDER` being true. + mempool + .expect_request(mempool::Request::FullTransactions) + .await? + .respond(mempool::Response::FullTransactions(transactions)); + + expected_response + }; mempool.expect_no_requests().await?; state.expect_no_requests().await?; diff --git a/zebra-rpc/src/methods/tests/snapshot.rs b/zebra-rpc/src/methods/tests/snapshot.rs index ca333a909b7..f25593e4ca0 100644 --- a/zebra-rpc/src/methods/tests/snapshot.rs +++ b/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| { From 0e04938e836291019812f7bd89c005df238a27f7 Mon Sep 17 00:00:00 2001 From: arya2 Date: Thu, 19 Jan 2023 17:12:05 -0500 Subject: [PATCH 10/11] Moves up comment --- zebra-rpc/src/methods/tests/prop.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zebra-rpc/src/methods/tests/prop.rs b/zebra-rpc/src/methods/tests/prop.rs index 59707c67de8..210812b9073 100644 --- a/zebra-rpc/src/methods/tests/prop.rs +++ b/zebra-rpc/src/methods/tests/prop.rs @@ -352,6 +352,7 @@ proptest! { 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(); @@ -371,7 +372,6 @@ proptest! { .map(|tx| tx.transaction.id.mined_id().encode_hex()) .collect(); - // Note: this depends on `SHOULD_USE_ZCASHD_ORDER` being true. mempool .expect_request(mempool::Request::FullTransactions) .await? From 6114a969005a49a87c4dd414cd1a9e4e7a11d52f Mon Sep 17 00:00:00 2001 From: arya2 Date: Fri, 20 Jan 2023 16:59:19 -0500 Subject: [PATCH 11/11] adds missing transaction_ids --- zebra-rpc/src/methods/tests/prop.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/zebra-rpc/src/methods/tests/prop.rs b/zebra-rpc/src/methods/tests/prop.rs index 210812b9073..822a7820011 100644 --- a/zebra-rpc/src/methods/tests/prop.rs +++ b/zebra-rpc/src/methods/tests/prop.rs @@ -337,10 +337,15 @@ proptest! { #[cfg(not(feature = "getblocktemplate-rpcs"))] - let mut expected_response = { + let expected_response = { + let transaction_ids: HashSet<_> = transactions + .iter() + .map(|tx| tx.transaction.id) + .collect(); + let mut expected_response: Vec = transaction_ids .iter() - .map(|tx| tx.transaction.id.mined_id().encode_hex()) + .map(|id| id.mined_id().encode_hex()) .collect(); expected_response.sort();