From add4741a520abf832a371e87685bf746a94747c9 Mon Sep 17 00:00:00 2001 From: teor Date: Thu, 24 Nov 2022 11:27:35 +1000 Subject: [PATCH] change(mempool): Evict transactions from the mempool using the ZIP-317 conventional fee (#5703) - manual fix changelog * Add a ZIP-317 conventional fee module * Add a conventional fee calculation stub, and use it for mempool size limiting * Just return a usize from zcash_serialized_size(), removing the unused Result * Add ZIP-317 constants * Calculate the ZIP-317 conventional fee * Update tests * Add a CHANGELOG entry * Fix a comment typo Co-authored-by: Daira Hopwood * Fix some missing words in a comment Co-authored-by: Arya Co-authored-by: Daira Hopwood Co-authored-by: Arya --- zebra-chain/src/serialization/tests/prop.rs | 4 +- .../src/serialization/zcash_serialize.rs | 4 +- zebra-chain/src/transaction/unmined.rs | 79 +++++++++------- zebra-chain/src/transaction/unmined/zip317.rs | 89 +++++++++++++++++++ zebra-rpc/src/methods/tests/vectors.rs | 2 + 5 files changed, 143 insertions(+), 35 deletions(-) create mode 100644 zebra-chain/src/transaction/unmined/zip317.rs diff --git a/zebra-chain/src/serialization/tests/prop.rs b/zebra-chain/src/serialization/tests/prop.rs index e21f397281e..11830950532 100644 --- a/zebra-chain/src/serialization/tests/prop.rs +++ b/zebra-chain/src/serialization/tests/prop.rs @@ -1,6 +1,6 @@ //! Property-based tests for basic serialization primitives. -use std::{convert::TryFrom, io::Cursor}; +use std::io::Cursor; use proptest::prelude::*; @@ -110,6 +110,6 @@ proptest! { fn transaction_serialized_size(transaction in any::()) { let _init_guard = zebra_test::init(); - prop_assert_eq!(transaction.transaction.zcash_serialized_size().unwrap(), transaction.size); + prop_assert_eq!(transaction.transaction.zcash_serialized_size(), transaction.size); } } diff --git a/zebra-chain/src/serialization/zcash_serialize.rs b/zebra-chain/src/serialization/zcash_serialize.rs index d00948bf11d..7bb4204144f 100644 --- a/zebra-chain/src/serialization/zcash_serialize.rs +++ b/zebra-chain/src/serialization/zcash_serialize.rs @@ -37,11 +37,11 @@ pub trait ZcashSerialize: Sized { } /// Get the size of `self` by using a fake writer. - fn zcash_serialized_size(&self) -> Result { + fn zcash_serialized_size(&self) -> usize { let mut writer = FakeWriter(0); self.zcash_serialize(&mut writer) .expect("writer should never fail"); - Ok(writer.0) + writer.0 } } diff --git a/zebra-chain/src/transaction/unmined.rs b/zebra-chain/src/transaction/unmined.rs index 1a5d694b983..0d0d422d4b0 100644 --- a/zebra-chain/src/transaction/unmined.rs +++ b/zebra-chain/src/transaction/unmined.rs @@ -17,9 +17,6 @@ use std::{fmt, sync::Arc}; -#[cfg(any(test, feature = "proptest-impl"))] -use proptest_derive::Arbitrary; - use crate::{ amount::{Amount, NonNegative}, serialization::ZcashSerialize, @@ -32,6 +29,11 @@ use crate::{ use UnminedTxId::*; +#[cfg(any(test, feature = "proptest-impl"))] +use proptest_derive::Arbitrary; + +mod zip317; + /// The minimum cost value for a transaction in the mempool. /// /// Contributes to the randomized, weighted eviction of transactions from the @@ -50,6 +52,12 @@ use UnminedTxId::*; /// [ZIP-401]: https://zips.z.cash/zip-0401 const MEMPOOL_TRANSACTION_COST_THRESHOLD: u64 = 4000; +/// When a transaction pays a fee less than the conventional fee, +/// this low fee penalty is added to its cost for mempool eviction. +/// +/// See [VerifiedUnminedTx::eviction_weight()] for details. +const MEMPOOL_TRANSACTION_LOW_FEE_PENALTY: u64 = 16_000; + /// A unique identifier for an unmined transaction, regardless of version. /// /// "The transaction ID of a version 4 or earlier transaction is the SHA-256d hash @@ -201,6 +209,11 @@ pub struct UnminedTx { /// The size in bytes of the serialized transaction data pub size: usize, + + /// The conventional fee for this transaction, as defined by [ZIP-317]. + /// + /// [ZIP-317]: https://zips.z.cash/zip-0317#fee-calculation + pub conventional_fee: Amount, } impl fmt::Display for UnminedTx { @@ -208,6 +221,7 @@ impl fmt::Display for UnminedTx { f.debug_struct("UnminedTx") .field("transaction", &self.transaction) .field("serialized_size", &self.size) + .field("conventional_fee", &self.conventional_fee) .finish() } } @@ -217,15 +231,15 @@ impl fmt::Display for UnminedTx { impl From for UnminedTx { fn from(transaction: Transaction) -> Self { - let size = transaction.zcash_serialized_size().expect( - "unexpected serialization failure: all structurally valid transactions have a size", - ); + let size = transaction.zcash_serialized_size(); + let conventional_fee = zip317::conventional_fee(&transaction); // The borrow is actually needed to avoid taking ownership #[allow(clippy::needless_borrow)] Self { id: (&transaction).into(), size, + conventional_fee, transaction: Arc::new(transaction), } } @@ -233,42 +247,42 @@ impl From for UnminedTx { impl From<&Transaction> for UnminedTx { fn from(transaction: &Transaction) -> Self { - let size = transaction.zcash_serialized_size().expect( - "unexpected serialization failure: all structurally valid transactions have a size", - ); + let size = transaction.zcash_serialized_size(); + let conventional_fee = zip317::conventional_fee(transaction); Self { id: transaction.into(), - transaction: Arc::new(transaction.clone()), size, + conventional_fee, + transaction: Arc::new(transaction.clone()), } } } impl From> for UnminedTx { fn from(transaction: Arc) -> Self { - let size = transaction.zcash_serialized_size().expect( - "unexpected serialization failure: all structurally valid transactions have a size", - ); + let size = transaction.zcash_serialized_size(); + let conventional_fee = zip317::conventional_fee(&transaction); Self { id: transaction.as_ref().into(), - transaction, size, + conventional_fee, + transaction, } } } impl From<&Arc> for UnminedTx { fn from(transaction: &Arc) -> Self { - let size = transaction.zcash_serialized_size().expect( - "unexpected serialization failure: all structurally valid transactions have a size", - ); + let size = transaction.zcash_serialized_size(); + let conventional_fee = zip317::conventional_fee(transaction); Self { id: transaction.as_ref().into(), - transaction: transaction.clone(), size, + conventional_fee, + transaction: transaction.clone(), } } } @@ -317,31 +331,34 @@ impl VerifiedUnminedTx { /// [ZIP-401]: https://zips.z.cash/zip-0401 pub fn cost(&self) -> u64 { std::cmp::max( - self.transaction.size as u64, + u64::try_from(self.transaction.size).expect("fits in u64"), MEMPOOL_TRANSACTION_COST_THRESHOLD, ) } /// The computed _eviction weight_ of a verified unmined transaction as part - /// of the mempool set. + /// of the mempool set, as defined in [ZIP-317] and [ZIP-401]. /// - /// Consensus rule: + /// Standard rule: /// /// > Each transaction also has an eviction weight, which is cost + /// > low_fee_penalty, where low_fee_penalty is 16000 if the transaction pays - /// > a fee less than the conventional fee, otherwise 0. The conventional fee - /// > is currently defined as 1000 zatoshis + /// > a fee less than the conventional fee, otherwise 0. /// + /// > zcashd and zebrad limit the size of the mempool as described in [ZIP-401]. + /// > This specifies a low fee penalty that is added to the "eviction weight" if the transaction + /// > pays a fee less than the conventional transaction fee. This threshold is + /// > modified to use the new conventional fee formula. + /// + /// [ZIP-317]: https://zips.z.cash/zip-0317#mempool-size-limiting /// [ZIP-401]: https://zips.z.cash/zip-0401 - pub fn eviction_weight(self) -> u64 { - let conventional_fee = 1000; + pub fn eviction_weight(&self) -> u64 { + let mut cost = self.cost(); - let low_fee_penalty = if u64::from(self.miner_fee) < conventional_fee { - 16_000 - } else { - 0 - }; + if self.miner_fee < self.transaction.conventional_fee { + cost += MEMPOOL_TRANSACTION_LOW_FEE_PENALTY + } - self.cost() + low_fee_penalty + cost } } diff --git a/zebra-chain/src/transaction/unmined/zip317.rs b/zebra-chain/src/transaction/unmined/zip317.rs new file mode 100644 index 00000000000..74309104a8e --- /dev/null +++ b/zebra-chain/src/transaction/unmined/zip317.rs @@ -0,0 +1,89 @@ +//! The [ZIP-317 conventional fee calculation](https://zips.z.cash/zip-0317#fee-calculation) +//! for [UnminedTx]s. + +use std::cmp::max; + +use crate::{ + amount::{Amount, NonNegative}, + serialization::ZcashSerialize, + transaction::Transaction, +}; + +// For doc links +#[allow(unused_imports)] +use crate::transaction::UnminedTx; + +/// The marginal fee for the ZIP-317 fee calculation, in zatoshis per logical action. +// +// TODO: allow Amount in constants +const MARGINAL_FEE: i64 = 5_000; + +/// The number of grace logical actions allowed by the ZIP-317 fee calculation. +const GRACE_ACTIONS: u64 = 2; + +/// The standard size of p2pkh inputs for the ZIP-317 fee calculation, in bytes. +const P2PKH_STANDARD_INPUT_SIZE: usize = 150; + +/// The standard size of p2pkh outputs for the ZIP-317 fee calculation, in bytes. +const P2PKH_STANDARD_OUTPUT_SIZE: usize = 34; + +/// Returns the conventional fee for `transaction`, as defined by [ZIP-317]. +/// +/// [ZIP-317]: https://zips.z.cash/zip-0317#fee-calculation +pub fn conventional_fee(transaction: &Transaction) -> Amount { + // zcash_primitives checks for non-p2pkh inputs, but Zebra doesn't. + // Conventional fees are only used in the standard rules for mempool eviction + // and block production, so these implementations are compatible. + // + // + + let marginal_fee: Amount = MARGINAL_FEE.try_into().expect("fits in amount"); + + let tx_in_total_size: usize = transaction + .inputs() + .iter() + .map(|input| input.zcash_serialized_size()) + .sum(); + + let tx_out_total_size: usize = transaction + .outputs() + .iter() + .map(|output| output.zcash_serialized_size()) + .sum(); + + let n_join_split = transaction.joinsplit_count(); + let n_spends_sapling = transaction.sapling_spends_per_anchor().count(); + let n_outputs_sapling = transaction.sapling_outputs().count(); + let n_actions_orchard = transaction.orchard_actions().count(); + + let tx_in_logical_actions = div_ceil(tx_in_total_size, P2PKH_STANDARD_INPUT_SIZE); + let tx_out_logical_actions = div_ceil(tx_out_total_size, P2PKH_STANDARD_OUTPUT_SIZE); + + let logical_actions = max(tx_in_logical_actions, tx_out_logical_actions) + + 2 * n_join_split + + max(n_spends_sapling, n_outputs_sapling) + + n_actions_orchard; + let logical_actions: u64 = logical_actions + .try_into() + .expect("transaction items are limited by serialized size limit"); + + let conventional_fee = marginal_fee * max(GRACE_ACTIONS, logical_actions); + + conventional_fee.expect("conventional fee is positive and limited by serialized size limit") +} + +/// Divide `quotient` by `divisor`, rounding the result up to the nearest integer. +/// +/// # Correctness +/// +/// `quotient + divisor` must be less than `usize::MAX`. +/// `divisor` must not be zero. +// +// TODO: replace with usize::div_ceil() when int_roundings stabilises: +// https://github.com/rust-lang/rust/issues/88581 +fn div_ceil(quotient: usize, divisor: usize) -> usize { + // Rust uses truncated integer division, so this is equivalent to: + // `ceil(quotient/divisor)` + // as long as the addition doesn't overflow or underflow. + (quotient + divisor - 1) / divisor +} diff --git a/zebra-rpc/src/methods/tests/vectors.rs b/zebra-rpc/src/methods/tests/vectors.rs index ab09932d856..50088e56151 100644 --- a/zebra-rpc/src/methods/tests/vectors.rs +++ b/zebra-rpc/src/methods/tests/vectors.rs @@ -6,6 +6,7 @@ use jsonrpc_core::ErrorCode; use tower::buffer::Buffer; use zebra_chain::{ + amount::Amount, block::Block, chain_tip::NoChainTip, parameters::Network::*, @@ -288,6 +289,7 @@ async fn rpc_getrawtransaction() { id: UnminedTxId::Legacy(tx.hash()), transaction: tx.clone(), size: 0, + conventional_fee: Amount::zero(), }])); }); let get_tx_req = rpc.get_raw_transaction(tx.hash().encode_hex(), 0u8);