From ae58d3eb498c99b35e2d778bb1940b58d5c59183 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Fri, 25 Oct 2024 07:29:06 -0600 Subject: [PATCH] Apply suggestions from code review & zcash/librustzcash#1579 Co-authored-by: Daira-Emma Hopwood --- zcash_client_backend/CHANGELOG.md | 8 ++++---- zcash_client_backend/src/fees/common.rs | 8 ++++---- .../src/wallet/init/migrations/fix_bad_change_flagging.rs | 7 ++----- zcash_client_sqlite/src/wallet/orchard.rs | 2 +- zcash_primitives/CHANGELOG.md | 2 +- zcash_primitives/src/transaction/fees/zip317.rs | 8 ++++++++ 6 files changed, 20 insertions(+), 15 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index 214924973..b08cd2e49 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -64,6 +64,8 @@ and this library adheres to Rust's notion of and `WalletMeta`, and its `FeeRule` associated type now has an additional `Clone` bound. In addition, it defines a new `fetch_wallet_meta` method, and the arguments to `compute_balance` have changed. + - `zip317::SingleOutputChangeStrategy` has been made polymorphic in the fee + rule type, and takes an additional type parameter as a consequence. - The following methods now take an additional `DustOutputPolicy` argument, and carry an additional type parameter: - `fixed::SingleOutputChangeStrategy::new` @@ -72,10 +74,8 @@ and this library adheres to Rust's notion of - `zcash_client_backend::proto::ProposalDecodingError` has modified variants. `ProposalDecodingError::FeeRuleNotSpecified` has been removed, and `ProposalDecodingError::FeeRuleNotSupported` has been added to replace it. - -### Deprecated -- `zcash_client_backend::data_api::fees::fixed`; also, this module is now - available only via the use of the `non-standard-fees` feature flag. +- `zcash_client_backend::data_api::fees::fixed` is now available only via the + use of the `non-standard-fees` feature flag. ### Removed - `zcash_client_backend::data_api`: diff --git a/zcash_client_backend/src/fees/common.rs b/zcash_client_backend/src/fees/common.rs index 45b77e5df..2a1a4a4e2 100644 --- a/zcash_client_backend/src/fees/common.rs +++ b/zcash_client_backend/src/fees/common.rs @@ -500,7 +500,7 @@ where .dust_threshold() .unwrap_or(cfg.default_dust_threshold); - if per_output_change.quotient() < &change_dust_threshold { + if proposed_change < change_dust_threshold { match cfg.dust_output_policy.action() { DustAction::Reject => { // Always allow zero-valued change even for the `Reject` policy: @@ -509,11 +509,11 @@ where // * this case occurs in practice when sending all funds from an account; // * zero-valued notes do not require witness tracking; // * the effect on trial decryption overhead is small. - if per_output_change.quotient().is_zero() { + if proposed_change.is_zero() && excess_fee.is_zero() { simple_case() } else { - let shortfall = (change_dust_threshold - *per_output_change.quotient()) - .ok_or_else(underflow)?; + let shortfall = + (change_dust_threshold - proposed_change).ok_or_else(underflow)?; return Err(ChangeError::InsufficientFunds { available: total_in, diff --git a/zcash_client_sqlite/src/wallet/init/migrations/fix_bad_change_flagging.rs b/zcash_client_sqlite/src/wallet/init/migrations/fix_bad_change_flagging.rs index 9773ab19b..efab795a6 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations/fix_bad_change_flagging.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations/fix_bad_change_flagging.rs @@ -86,15 +86,12 @@ mod tests { wallet::input_selection::GreedyInputSelector, Account as _, WalletRead as _, WalletWrite as _, }, - fees::{standard, DustOutputPolicy}, + fees::{standard, DustOutputPolicy, StandardFeeRule}, wallet::WalletTransparentOutput, }, zcash_primitives::{ block::BlockHash, - transaction::{ - components::{OutPoint, TxOut}, - fees::StandardFeeRule, - }, + transaction::components::{OutPoint, TxOut}, }, zcash_protocol::value::Zatoshis, }; diff --git a/zcash_client_sqlite/src/wallet/orchard.rs b/zcash_client_sqlite/src/wallet/orchard.rs index 1223c3852..046b81f6b 100644 --- a/zcash_client_sqlite/src/wallet/orchard.rs +++ b/zcash_client_sqlite/src/wallet/orchard.rs @@ -401,7 +401,7 @@ pub(crate) mod tests { #[test] fn send_with_multiple_change_outputs() { - testing::pool::send_with_multiple_change_outputs::() + testing::pool::send_with_multiple_change_outputs::() } #[test] diff --git a/zcash_primitives/CHANGELOG.md b/zcash_primitives/CHANGELOG.md index c25cccc5f..8c357adc5 100644 --- a/zcash_primitives/CHANGELOG.md +++ b/zcash_primitives/CHANGELOG.md @@ -31,7 +31,7 @@ and this library adheres to Rust's notion of ### Removed - `zcash_primitives::transaction::fees`: - `StandardFeeRule` itself has been removed; it was not used in this crate. - Is use in `zcash_client_backend` has been replaced with + Its use in `zcash_client_backend` has been replaced with `zcash_client_backend::fees::StandardFeeRule`. - `fixed::FeeRule::standard`. This constructor was misleadingly named: using a fixed fee does not conform to any current Zcash standard. To calculate the diff --git a/zcash_primitives/src/transaction/fees/zip317.rs b/zcash_primitives/src/transaction/fees/zip317.rs index f59ae925d..63c8089d0 100644 --- a/zcash_primitives/src/transaction/fees/zip317.rs +++ b/zcash_primitives/src/transaction/fees/zip317.rs @@ -76,6 +76,14 @@ impl FeeRule { /// Construct a new FeeRule instance with the specified parameter values. /// + /// Using this fee rule with + /// ```compile_fail + /// marginal_fee < 5000 || grace_actions < 2 \ + /// || p2pkh_standard_input_size > P2PKH_STANDARD_INPUT_SIZE \ + /// || p2pkh_standard_output_size > P2PKH_STANDARD_OUTPUT_SIZE + /// ``` + /// violates ZIP 317, and might cause transactions built with it to fail. + /// /// Returns `None` if either `p2pkh_standard_input_size` or `p2pkh_standard_output_size` are /// zero. #[cfg(feature = "non-standard-fees")]