Skip to content

Commit

Permalink
Avoids sorting getblocktemplate transactions in non-test compilations
Browse files Browse the repository at this point in the history
  • Loading branch information
arya2 committed Nov 7, 2024
1 parent 257567b commit 42dfc39
Show file tree
Hide file tree
Showing 3 changed files with 58 additions and 31 deletions.
2 changes: 1 addition & 1 deletion zebra-rpc/src/methods/get_block_template_rpcs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -904,7 +904,7 @@ where
tracing::debug!(
selected_mempool_tx_hashes = ?mempool_txs
.iter()
.map(|(_, tx)| tx.transaction.id.mined_id())
.map(|#[cfg(not(test))] tx, #[cfg(test)] (_, tx)| tx.transaction.id.mined_id())
.collect::<Vec<_>>(),
"selected transactions for the template from the mempool"
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,8 @@ impl GetBlockTemplate {
miner_address: &transparent::Address,
chain_tip_and_local_time: &GetBlockTemplateChainInfo,
long_poll_id: LongPollId,
mempool_txs: Vec<(InBlockTxDependenciesDepth, VerifiedUnminedTx)>,
#[cfg(not(test))] mempool_txs: Vec<VerifiedUnminedTx>,
#[cfg(test)] mempool_txs: Vec<(InBlockTxDependenciesDepth, VerifiedUnminedTx)>,
submit_old: Option<bool>,
like_zcashd: bool,
extra_coinbase_data: Vec<u8>,
Expand All @@ -242,14 +243,9 @@ impl GetBlockTemplate {
(chain_tip_and_local_time.tip_height + 1).expect("tip is far below Height::MAX");

// Convert transactions into TransactionTemplates
let mut mempool_txs_with_templates: Vec<(
InBlockTxDependenciesDepth,
TransactionTemplate<amount::NonNegative>,
VerifiedUnminedTx,
)> = mempool_txs
.into_iter()
.map(|(min_tx_index, tx)| (min_tx_index, (&tx).into(), tx))
.collect();
#[cfg(not(test))]
let (mempool_tx_templates, mempool_txs): (Vec<_>, Vec<_>) =
mempool_txs.into_iter().map(|tx| ((&tx).into(), tx)).unzip();

// Transaction selection returns transactions in an arbitrary order,
// but Zebra's snapshot tests expect the same order every time.
Expand All @@ -258,23 +254,34 @@ impl GetBlockTemplate {
//
// Transactions that spend outputs created in the same block must appear
// after the transactions that create those outputs.
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(|(min_tx_index, tx_template, _tx)| {
(*min_tx_index, tx_template.data.clone())
});
} else {
// Sort by hash, this is faster.
mempool_txs_with_templates.sort_by_key(|(min_tx_index, tx_template, _tx)| {
(*min_tx_index, tx_template.hash.bytes_in_display_order())
});
}

let (mempool_tx_templates, mempool_txs): (Vec<_>, Vec<_>) = mempool_txs_with_templates
.into_iter()
.map(|(_, template, tx)| (template, tx))
.unzip();
#[cfg(test)]
let (mempool_tx_templates, mempool_txs): (Vec<_>, Vec<_>) = {
let mut mempool_txs_with_templates: Vec<(
InBlockTxDependenciesDepth,
TransactionTemplate<amount::NonNegative>,
VerifiedUnminedTx,
)> = mempool_txs
.into_iter()
.map(|(min_tx_index, tx)| (min_tx_index, (&tx).into(), tx))
.collect();
#[cfg(test)]
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(|(min_tx_index, tx_template, _tx)| {
(*min_tx_index, tx_template.data.clone())
});
} else {
// Sort by hash, this is faster.
mempool_txs_with_templates.sort_by_key(|(min_tx_index, tx_template, _tx)| {
(*min_tx_index, tx_template.hash.bytes_in_display_order())
});
}
mempool_txs_with_templates
.into_iter()
.map(|(_, template, tx)| (template, tx))
.unzip()
};

// Generate the coinbase transaction and default roots
//
Expand Down
28 changes: 24 additions & 4 deletions zebra-rpc/src/methods/get_block_template_rpcs/zip317.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,20 @@ use crate::methods::get_block_template_rpcs::{
get_block_template::generate_coinbase_transaction, types::transaction::TransactionTemplate,
};

#[cfg(test)]
use super::get_block_template::InBlockTxDependenciesDepth;

#[cfg(test)]
mod tests;

/// Used in the return type of [`select_mempool_transactions()`] for test compilations.
#[cfg(test)]
type SelectedMempoolTx = (InBlockTxDependenciesDepth, VerifiedUnminedTx);

/// Used in the return type of [`select_mempool_transactions()`] for non-test compilations.
#[cfg(not(test))]
type SelectedMempoolTx = VerifiedUnminedTx;

/// Selects mempool transactions for block production according to [ZIP-317],
/// using a fake coinbase transaction and the mempool.
///
Expand All @@ -52,7 +61,7 @@ pub fn select_mempool_transactions(
mempool_tx_deps: TransactionDependencies,
like_zcashd: bool,
extra_coinbase_data: Vec<u8>,
) -> Vec<(InBlockTxDependenciesDepth, VerifiedUnminedTx)> {
) -> Vec<SelectedMempoolTx> {
// Use a fake coinbase transaction to break the dependency between transaction
// selection, the miner fee, and the fee payment in the coinbase transaction.
let fake_coinbase_tx = fake_coinbase_transaction(
Expand Down Expand Up @@ -183,14 +192,16 @@ fn setup_fee_weighted_index(transactions: &[VerifiedUnminedTx]) -> Option<Weight
/// Requires items in `selected_txs` to be unique to work correctly.
fn has_direct_dependencies(
candidate_tx_deps: Option<&HashSet<transaction::Hash>>,
selected_txs: &Vec<(InBlockTxDependenciesDepth, VerifiedUnminedTx)>,
selected_txs: &Vec<SelectedMempoolTx>,
) -> bool {
let Some(deps) = candidate_tx_deps else {
return true;
};

let mut num_available_deps = 0;
for (_, tx) in selected_txs {
for tx in selected_txs {
#[cfg(test)]
let (_, tx) = tx;
if deps.contains(&tx.transaction.id.mined_id()) {
num_available_deps += 1;
} else {
Expand All @@ -207,6 +218,7 @@ fn has_direct_dependencies(

/// Returns the depth of a transaction's dependencies in the block for a candidate
/// transaction with the provided dependencies.
#[cfg(test)]
fn dependencies_depth(
dependent_tx_id: &transaction::Hash,
mempool_tx_deps: &TransactionDependencies,
Expand Down Expand Up @@ -240,7 +252,7 @@ fn checked_add_transaction_weighted_random(
candidate_txs: &mut Vec<VerifiedUnminedTx>,
dependent_txs: &mut HashMap<transaction::Hash, VerifiedUnminedTx>,
tx_weights: WeightedIndex<f32>,
selected_txs: &mut Vec<(InBlockTxDependenciesDepth, VerifiedUnminedTx)>,
selected_txs: &mut Vec<SelectedMempoolTx>,
mempool_tx_deps: &TransactionDependencies,
remaining_block_bytes: &mut usize,
remaining_block_sigops: &mut u64,
Expand All @@ -266,6 +278,10 @@ fn checked_add_transaction_weighted_random(
"all candidate transactions should be independent"
);

#[cfg(not(test))]
selected_txs.push(candidate_tx);

#[cfg(test)]
selected_txs.push((0, candidate_tx));

// Try adding any dependent transactions if all of their dependencies have been selected.
Expand Down Expand Up @@ -294,6 +310,10 @@ fn checked_add_transaction_weighted_random(
continue;
}

#[cfg(not(test))]
selected_txs.push(candidate_tx);

#[cfg(test)]
selected_txs.push((
dependencies_depth(dependent_tx_id, mempool_tx_deps),
candidate_tx,
Expand Down

0 comments on commit 42dfc39

Please sign in to comment.