From 1090d66eb16b5ad19e6e8cfb4b75845140fdce97 Mon Sep 17 00:00:00 2001 From: teor Date: Wed, 23 Nov 2022 12:13:39 +1000 Subject: [PATCH 1/9] Add a ZIP-317 conventional fee module --- zebra-chain/src/transaction/unmined.rs | 8 +++++--- zebra-chain/src/transaction/unmined/zip317.rs | 6 ++++++ 2 files changed, 11 insertions(+), 3 deletions(-) create mode 100644 zebra-chain/src/transaction/unmined/zip317.rs diff --git a/zebra-chain/src/transaction/unmined.rs b/zebra-chain/src/transaction/unmined.rs index e6b80174cc2..b7e5fc17d29 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 diff --git a/zebra-chain/src/transaction/unmined/zip317.rs b/zebra-chain/src/transaction/unmined/zip317.rs new file mode 100644 index 00000000000..5cba2b84925 --- /dev/null +++ b/zebra-chain/src/transaction/unmined/zip317.rs @@ -0,0 +1,6 @@ +//! The [ZIP-317 conventional fee calculation](https://zips.z.cash/zip-0317#fee-calculation) +//! for [UnminedTx]s. + +// For doc links +#[allow(unused_imports)] +use crate::transaction::UnminedTx; From 0232a6a0acdada0dc0c9aa262524c2da9109e7d9 Mon Sep 17 00:00:00 2001 From: teor Date: Wed, 23 Nov 2022 12:36:38 +1000 Subject: [PATCH 2/9] Add a conventional fee calculation stub, and use it for mempool size limiting --- zebra-chain/src/transaction/unmined.rs | 59 ++++++++++++++----- zebra-chain/src/transaction/unmined/zip317.rs | 12 ++++ 2 files changed, 55 insertions(+), 16 deletions(-) diff --git a/zebra-chain/src/transaction/unmined.rs b/zebra-chain/src/transaction/unmined.rs index b7e5fc17d29..bf0492b008d 100644 --- a/zebra-chain/src/transaction/unmined.rs +++ b/zebra-chain/src/transaction/unmined.rs @@ -52,6 +52,12 @@ mod zip317; /// [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 penatly 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 @@ -203,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 { @@ -210,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() } } @@ -223,11 +235,14 @@ impl From for UnminedTx { "unexpected serialization failure: all structurally valid transactions have a 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), } } @@ -239,10 +254,13 @@ impl From<&Transaction> for UnminedTx { "unexpected serialization failure: all structurally valid transactions have a 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()), } } } @@ -253,10 +271,13 @@ impl From> for UnminedTx { "unexpected serialization failure: all structurally valid transactions have a size", ); + let conventional_fee = zip317::conventional_fee(&transaction); + Self { id: transaction.as_ref().into(), - transaction, size, + conventional_fee, + transaction, } } } @@ -267,10 +288,13 @@ impl From<&Arc> for UnminedTx { "unexpected serialization failure: all structurally valid transactions have a size", ); + let conventional_fee = zip317::conventional_fee(transaction); + Self { id: transaction.as_ref().into(), - transaction: transaction.clone(), size, + conventional_fee, + transaction: transaction.clone(), } } } @@ -329,31 +353,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 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 index 5cba2b84925..75210258bbf 100644 --- a/zebra-chain/src/transaction/unmined/zip317.rs +++ b/zebra-chain/src/transaction/unmined/zip317.rs @@ -1,6 +1,18 @@ //! The [ZIP-317 conventional fee calculation](https://zips.z.cash/zip-0317#fee-calculation) //! for [UnminedTx]s. +use crate::{ + amount::{Amount, NonNegative}, + transaction::Transaction, +}; + // For doc links #[allow(unused_imports)] use crate::transaction::UnminedTx; + +/// 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 { + todo!() +} From 6b76eb1d7fadd21b5117b80f349650fed6384c2b Mon Sep 17 00:00:00 2001 From: teor Date: Wed, 23 Nov 2022 13:07:36 +1000 Subject: [PATCH 3/9] Just return a usize from zcash_serialized_size(), removing the unused Result --- zebra-chain/src/serialization/tests/prop.rs | 4 ++-- .../src/serialization/zcash_serialize.rs | 4 ++-- zebra-chain/src/transaction/unmined.rs | 20 ++++--------------- 3 files changed, 8 insertions(+), 20 deletions(-) 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 bf0492b008d..97144963a48 100644 --- a/zebra-chain/src/transaction/unmined.rs +++ b/zebra-chain/src/transaction/unmined.rs @@ -231,10 +231,7 @@ 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 @@ -250,10 +247,7 @@ 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 { @@ -267,10 +261,7 @@ impl From<&Transaction> for UnminedTx { 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 { @@ -284,10 +275,7 @@ impl From> for UnminedTx { 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 { From f47439e1c238a38b0b5126a9d3e564c46c71f097 Mon Sep 17 00:00:00 2001 From: teor Date: Wed, 23 Nov 2022 13:08:22 +1000 Subject: [PATCH 4/9] Add ZIP-317 constants --- zebra-chain/src/transaction/unmined/zip317.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/zebra-chain/src/transaction/unmined/zip317.rs b/zebra-chain/src/transaction/unmined/zip317.rs index 75210258bbf..ca27f19793f 100644 --- a/zebra-chain/src/transaction/unmined/zip317.rs +++ b/zebra-chain/src/transaction/unmined/zip317.rs @@ -10,6 +10,20 @@ use crate::{ #[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 From 2a06584a34a5c30fe323afd5c1d8925d1bb6d160 Mon Sep 17 00:00:00 2001 From: teor Date: Wed, 23 Nov 2022 13:39:43 +1000 Subject: [PATCH 5/9] Calculate the ZIP-317 conventional fee --- zebra-chain/src/transaction/unmined/zip317.rs | 59 ++++++++++++++++++- 1 file changed, 58 insertions(+), 1 deletion(-) diff --git a/zebra-chain/src/transaction/unmined/zip317.rs b/zebra-chain/src/transaction/unmined/zip317.rs index ca27f19793f..74309104a8e 100644 --- a/zebra-chain/src/transaction/unmined/zip317.rs +++ b/zebra-chain/src/transaction/unmined/zip317.rs @@ -1,8 +1,11 @@ //! 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, }; @@ -28,5 +31,59 @@ const P2PKH_STANDARD_OUTPUT_SIZE: usize = 34; /// /// [ZIP-317]: https://zips.z.cash/zip-0317#fee-calculation pub fn conventional_fee(transaction: &Transaction) -> Amount { - todo!() + // 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 } From 08dc2805ad002be88d984f8ae69207b1f585c9c0 Mon Sep 17 00:00:00 2001 From: teor Date: Wed, 23 Nov 2022 13:42:40 +1000 Subject: [PATCH 6/9] Update tests --- zebra-rpc/src/methods/tests/vectors.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/zebra-rpc/src/methods/tests/vectors.rs b/zebra-rpc/src/methods/tests/vectors.rs index b99fa58819b..09262f1af21 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); From 3d2d7cbcb7386bf3447748b3a093c62fa7ef4a33 Mon Sep 17 00:00:00 2001 From: teor Date: Wed, 23 Nov 2022 14:05:51 +1000 Subject: [PATCH 7/9] Add a CHANGELOG entry --- CHANGELOG.md | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e404446ae5..50eea94bc83 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,18 +6,26 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ## [Zebra 1.0.0-rc.2](https://github.com/ZcashFoundation/zebra/releases/tag/v1.0.0-rc.2) - 2022-11-TODO -Zebra's latest release continues work on mining pool RPCs, and fixes a rare RPC crash that could lead to memory corruption. +Zebra's latest release continues work on mining pool RPCs, fixes a rare RPC crash that could lead to memory corruption, and uses the ZIP-317 conventional fee for mempool size limits. Zebra's consensus rules, node sync, and `lightwalletd` RPCs are ready for user testing and experimental use. Zebra has not been audited yet. ### Breaking Changes This release has the following breaking changes: +- Evict transactions from the mempool using the ZIP-317 conventional fee ([#5703](https://github.com/ZcashFoundation/zebra/pull/5703)) + - If there are a lot of unmined transactions on the Zcash network, and Zebra's mempool + becomes full, Zebra will penalise transactions that don't pay at least the ZIP-317 + conventional fee. These transactions will be more likely to get evicted. + - The ZIP-317 convention fee increases based on the number of logical transparent or + shielded actions in a transaction. + - This change has no impact under normal network conditions. - TODO: search the changelog for breaking changes ### Security - Fix a rare crash and memory errors when Zebra's RPC server shuts down ([#5591](https://github.com/ZcashFoundation/zebra/pull/5591)) +- Evict transactions from the mempool using the ZIP-317 conventional fee ([#5703](https://github.com/ZcashFoundation/zebra/pull/5703)) TODO: the rest of the changelog From 736ae8e71ffd27524fb2e980c179def55b3d1c60 Mon Sep 17 00:00:00 2001 From: teor Date: Thu, 24 Nov 2022 06:56:51 +1000 Subject: [PATCH 8/9] Fix a comment typo Co-authored-by: Daira Hopwood --- zebra-chain/src/transaction/unmined.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zebra-chain/src/transaction/unmined.rs b/zebra-chain/src/transaction/unmined.rs index 97144963a48..d3f274c0cd4 100644 --- a/zebra-chain/src/transaction/unmined.rs +++ b/zebra-chain/src/transaction/unmined.rs @@ -53,7 +53,7 @@ mod zip317; const MEMPOOL_TRANSACTION_COST_THRESHOLD: u64 = 4000; /// When a transaction pays a fee less than the conventional fee, -/// this low fee penatly is added to its cost for mempool eviction. +/// 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; From a111fd6ad6e266e300312e75b6b9fe9346cf46ea Mon Sep 17 00:00:00 2001 From: teor Date: Thu, 24 Nov 2022 06:57:23 +1000 Subject: [PATCH 9/9] Fix some missing words in a comment Co-authored-by: Arya --- zebra-chain/src/transaction/unmined.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zebra-chain/src/transaction/unmined.rs b/zebra-chain/src/transaction/unmined.rs index d3f274c0cd4..68361e20b6d 100644 --- a/zebra-chain/src/transaction/unmined.rs +++ b/zebra-chain/src/transaction/unmined.rs @@ -356,7 +356,7 @@ impl VerifiedUnminedTx { /// > 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 that is added to the "eviction weight" if the transaction + /// > 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. ///