From 6a3ac2f9a91371ea23fd4cafd638110b0d28120b Mon Sep 17 00:00:00 2001 From: arya2 Date: Thu, 19 Jan 2023 17:39:32 -0500 Subject: [PATCH 1/2] Removes candidate_tx from candidate_txs - Creates a new WeightedIndex instead of updating the weights - Logs tx hashes and unpaid_actions in getblocktemplate --- .../src/methods/get_block_template_rpcs.rs | 16 ++++++++++++ .../types/get_block_template.rs | 13 ++++++++-- .../methods/get_block_template_rpcs/zip317.rs | 25 +++++++------------ 3 files changed, 36 insertions(+), 18 deletions(-) diff --git a/zebra-rpc/src/methods/get_block_template_rpcs.rs b/zebra-rpc/src/methods/get_block_template_rpcs.rs index 8f56d77eb30..447b68c0de3 100644 --- a/zebra-rpc/src/methods/get_block_template_rpcs.rs +++ b/zebra-rpc/src/methods/get_block_template_rpcs.rs @@ -568,6 +568,14 @@ where let next_block_height = (chain_tip_and_local_time.tip_height + 1).expect("tip is far below Height::MAX"); + tracing::info!( + mempool_tx_hashes = ?mempool_txs + .iter() + .map(|tx| tx.transaction.id.mined_id()) + .collect::>(), + "Selecting transactions for the template from the mempool" + ); + // Randomly select some mempool transactions. // // TODO: sort these transactions to match zcashd's order, to make testing easier. @@ -580,6 +588,14 @@ where ) .await; + tracing::info!( + mempool_tx_hashes = ?mempool_txs + .iter() + .map(|tx| tx.transaction.id.mined_id()) + .collect::>(), + "Selected transactions for the template from the mempool" + ); + // - After this point, the template only depends on the previously fetched data. let response = GetBlockTemplate::new( diff --git a/zebra-rpc/src/methods/get_block_template_rpcs/types/get_block_template.rs b/zebra-rpc/src/methods/get_block_template_rpcs/types/get_block_template.rs index 4135fb1232b..e35d5196ee4 100644 --- a/zebra-rpc/src/methods/get_block_template_rpcs/types/get_block_template.rs +++ b/zebra-rpc/src/methods/get_block_template_rpcs/types/get_block_template.rs @@ -209,11 +209,12 @@ impl GetBlockTemplate { if like_zcashd { // Sort in serialized data order, excluding the length byte. // `zcashd` sometimes seems to do this, but other times the order is arbitrary. - mempool_txs_with_templates.sort_by_key(|(tx_template, _tx)| tx_template.data.clone()); + mempool_txs_with_templates + .sort_by_cached_key(|(tx_template, _tx)| tx_template.data.clone()); } else { // Sort by hash, this is faster. mempool_txs_with_templates - .sort_by_key(|(tx_template, _tx)| tx_template.hash.bytes_in_display_order()); + .sort_by_cached_key(|(tx_template, _tx)| tx_template.hash.bytes_in_display_order()); } let (mempool_tx_templates, mempool_txs): (Vec<_>, Vec<_>) = @@ -244,6 +245,14 @@ impl GetBlockTemplate { .map(ToString::to_string) .collect(); + tracing::info!( + selected_txs = ?mempool_txs + .iter() + .map(|tx| (tx.transaction.id.mined_id(), tx.unpaid_actions)) + .collect::>(), + "Creating template ... " + ); + GetBlockTemplate { capabilities, diff --git a/zebra-rpc/src/methods/get_block_template_rpcs/zip317.rs b/zebra-rpc/src/methods/get_block_template_rpcs/zip317.rs index 185f569c4ba..e42707a25c5 100644 --- a/zebra-rpc/src/methods/get_block_template_rpcs/zip317.rs +++ b/zebra-rpc/src/methods/get_block_template_rpcs/zip317.rs @@ -53,7 +53,7 @@ pub async fn select_mempool_transactions( fake_coinbase_transaction(network, next_block_height, miner_address, like_zcashd); // Setup the transaction lists. - let (conventional_fee_txs, low_fee_txs): (Vec<_>, Vec<_>) = mempool_txs + let (mut conventional_fee_txs, mut low_fee_txs): (Vec<_>, Vec<_>) = mempool_txs .into_iter() .partition(VerifiedUnminedTx::pays_conventional_fee); @@ -74,7 +74,7 @@ pub async fn select_mempool_transactions( while let Some(tx_weights) = conventional_fee_tx_weights { conventional_fee_tx_weights = checked_add_transaction_weighted_random( - &conventional_fee_txs, + &mut conventional_fee_txs, tx_weights, &mut selected_txs, &mut remaining_block_bytes, @@ -90,7 +90,7 @@ pub async fn select_mempool_transactions( while let Some(tx_weights) = low_fee_tx_weights { low_fee_tx_weights = checked_add_transaction_weighted_random( - &low_fee_txs, + &mut low_fee_txs, tx_weights, &mut selected_txs, &mut remaining_block_bytes, @@ -160,7 +160,7 @@ fn setup_fee_weighted_index(transactions: &[VerifiedUnminedTx]) -> Option, tx_weights: WeightedIndex, selected_txs: &mut Vec, remaining_block_bytes: &mut usize, @@ -201,20 +201,13 @@ fn checked_add_transaction_weighted_random( /// If some transactions have not yet been chosen, returns the weighted index and the transaction. /// Otherwise, just returns the transaction. fn choose_transaction_weighted_random( - candidate_txs: &[VerifiedUnminedTx], - mut weighted_index: WeightedIndex, + candidate_txs: &mut Vec, + weighted_index: WeightedIndex, ) -> (Option>, VerifiedUnminedTx) { let candidate_position = weighted_index.sample(&mut thread_rng()); - let candidate_tx = candidate_txs[candidate_position].clone(); + let candidate_tx = candidate_txs.swap_remove(candidate_position); // Only pick each transaction once, by setting picked transaction weights to zero - if weighted_index - .update_weights(&[(candidate_position, &0.0)]) - .is_err() - { - // All weights are zero, so each transaction has either been selected or rejected - (None, candidate_tx) - } else { - (Some(weighted_index), candidate_tx) - } + // All weights are zero, so each transaction has either been selected or rejected + (setup_fee_weighted_index(&candidate_txs), candidate_tx) } From b1cec4fb44d64521b16adca1532ead59a9104d51 Mon Sep 17 00:00:00 2001 From: arya2 Date: Fri, 20 Jan 2023 17:06:38 -0500 Subject: [PATCH 2/2] Applies suggestions from PR review. --- zebra-rpc/src/methods/get_block_template_rpcs.rs | 10 +++++----- .../types/get_block_template.rs | 9 ++++----- .../src/methods/get_block_template_rpcs/zip317.rs | 7 ++++--- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/zebra-rpc/src/methods/get_block_template_rpcs.rs b/zebra-rpc/src/methods/get_block_template_rpcs.rs index 447b68c0de3..a2dbbf57e3f 100644 --- a/zebra-rpc/src/methods/get_block_template_rpcs.rs +++ b/zebra-rpc/src/methods/get_block_template_rpcs.rs @@ -568,12 +568,12 @@ where let next_block_height = (chain_tip_and_local_time.tip_height + 1).expect("tip is far below Height::MAX"); - tracing::info!( + tracing::debug!( mempool_tx_hashes = ?mempool_txs .iter() .map(|tx| tx.transaction.id.mined_id()) .collect::>(), - "Selecting transactions for the template from the mempool" + "selecting transactions for the template from the mempool" ); // Randomly select some mempool transactions. @@ -588,12 +588,12 @@ where ) .await; - tracing::info!( - mempool_tx_hashes = ?mempool_txs + tracing::debug!( + selected_mempool_tx_hashes = ?mempool_txs .iter() .map(|tx| tx.transaction.id.mined_id()) .collect::>(), - "Selected transactions for the template from the mempool" + "selected transactions for the template from the mempool" ); // - After this point, the template only depends on the previously fetched data. diff --git a/zebra-rpc/src/methods/get_block_template_rpcs/types/get_block_template.rs b/zebra-rpc/src/methods/get_block_template_rpcs/types/get_block_template.rs index e35d5196ee4..5a223795392 100644 --- a/zebra-rpc/src/methods/get_block_template_rpcs/types/get_block_template.rs +++ b/zebra-rpc/src/methods/get_block_template_rpcs/types/get_block_template.rs @@ -209,12 +209,11 @@ impl GetBlockTemplate { if like_zcashd { // Sort in serialized data order, excluding the length byte. // `zcashd` sometimes seems to do this, but other times the order is arbitrary. - mempool_txs_with_templates - .sort_by_cached_key(|(tx_template, _tx)| tx_template.data.clone()); + mempool_txs_with_templates.sort_by_key(|(tx_template, _tx)| tx_template.data.clone()); } else { // Sort by hash, this is faster. mempool_txs_with_templates - .sort_by_cached_key(|(tx_template, _tx)| tx_template.hash.bytes_in_display_order()); + .sort_by_key(|(tx_template, _tx)| tx_template.hash.bytes_in_display_order()); } let (mempool_tx_templates, mempool_txs): (Vec<_>, Vec<_>) = @@ -245,12 +244,12 @@ impl GetBlockTemplate { .map(ToString::to_string) .collect(); - tracing::info!( + tracing::debug!( selected_txs = ?mempool_txs .iter() .map(|tx| (tx.transaction.id.mined_id(), tx.unpaid_actions)) .collect::>(), - "Creating template ... " + "creating template ... " ); GetBlockTemplate { diff --git a/zebra-rpc/src/methods/get_block_template_rpcs/zip317.rs b/zebra-rpc/src/methods/get_block_template_rpcs/zip317.rs index e42707a25c5..adb4af608d2 100644 --- a/zebra-rpc/src/methods/get_block_template_rpcs/zip317.rs +++ b/zebra-rpc/src/methods/get_block_template_rpcs/zip317.rs @@ -207,7 +207,8 @@ fn choose_transaction_weighted_random( let candidate_position = weighted_index.sample(&mut thread_rng()); let candidate_tx = candidate_txs.swap_remove(candidate_position); - // Only pick each transaction once, by setting picked transaction weights to zero - // All weights are zero, so each transaction has either been selected or rejected - (setup_fee_weighted_index(&candidate_txs), candidate_tx) + // We have to regenerate this index each time we choose a transaction, due to floating-point sum inaccuracies. + // If we don't, some chosen transactions can end up with a tiny non-zero weight, leading to duplicates. + // + (setup_fee_weighted_index(candidate_txs), candidate_tx) }