Simplify change strategies, and deprecate or remove fee rules that no longer produce minable transactions #40

daira · 2024-10-21T02:16:32Z

Remove fixed::FeeRule::standard (which was misleadingly named because fixed fees are not standard).
Deprecate fixed::FeeRule::non_standard.
Remove zcash_primitives::transaction::fees::StandardFeeRule::{PreZip313,Zip313}.
Rename zcash_client_backend::proto::ProposalDecodingError::FeeRuleNotSpecified to FeeRuleNotSupported and use it to report attempted use of the removed rules.
Refactor change strategies so that we can use a CommonChangeStrategy that is generic in the fee rule. This should simplify future generalizations of the change strategy.
Undo the changes to {fixed,standard,zip317}::SingleOutputChangeStrategy, and deprecate those types' new constructors. They are now implemented as trivial wrappers over CommonChangeStrategy.

… instead of a tuple.

…allet metadata. In the process this modifies input selection to take the change strategy as an explicit argument, rather than being wrapped as part of the input selector.

…ance`

Co-authored-by: Jack Grigg <thestr4d@gmail.com> Co-authored-by: Daira-Emma Hopwood <daira@jacaranda.org>

fixed fees are not standard), and deprecate `fixed::FeeRule::non_standard`. Signed-off-by: Daira-Emma Hopwood <daira@jacaranda.org>

…313,Zip313}`. Rename `zcash_client_backend::proto::ProposalDecodingError::FeeRuleNotSpecified` to `FeeRuleNotSupported` and use it to report attempted use of the removed rules. Signed-off-by: Daira-Emma Hopwood <daira@jacaranda.org>

that is generic in the fee rule. This should simplify future generalizations of the change strategy. We also undo the changes to `{fixed,standard,zip317}::SingleOutputChangeStrategy` in this commit, and deprecate those types' `new` constructors. Signed-off-by: Daira-Emma Hopwood <daira@jacaranda.org>

daira · 2024-10-21T02:32:08Z

zcash_client_backend/src/fees/zip317.rs

-/// A change strategy that attempts to split the change value into some number of equal-sized notes
-/// as dictated by the included [`SplitPolicy`] value.
-pub struct MultiOutputChangeStrategy<I> {
-    fee_rule: Zip317FeeRule,
-    change_memo: Option<MemoBytes>,
-    fallback_change_pool: ShieldedProtocol,
-    dust_output_policy: DustOutputPolicy,
-    split_policy: SplitPolicy,
-    meta_source: PhantomData<I>,
-}
-
-impl<I> MultiOutputChangeStrategy<I> {
-    /// Constructs a new [`MultiOutputChangeStrategy`] with the specified ZIP 317
-    /// fee parameters, change memo, and change splitting policy.
-    ///
-    /// This change strategy will fall back to creating a single change output if insufficient
-    /// change value is available to create notes with at least the minimum value dictated by the
-    /// split policy.
-    ///
-    /// - `fallback_change_pool`: the pool to which change will be sent if when more than one
-    ///   shielded pool is enabled via feature flags, and the transaction has no shielded inputs.
-    /// - `split_policy`: A policy value describing how the change value should be returned as
-    ///   multiple notes.
-    pub fn new(
-        fee_rule: Zip317FeeRule,
-        change_memo: Option<MemoBytes>,
-        fallback_change_pool: ShieldedProtocol,
-        dust_output_policy: DustOutputPolicy,
-        split_policy: SplitPolicy,
-    ) -> Self {
-        Self {
-            fee_rule,
-            change_memo,
-            fallback_change_pool,
-            dust_output_policy,
-            split_policy,
-            meta_source: PhantomData,
-        }
-    }
-}
-
-impl<I: InputSource> ChangeStrategy for MultiOutputChangeStrategy<I> {
-    type FeeRule = Zip317FeeRule;
-    type Error = Zip317FeeError;
-    type MetaSource = I;
-    type WalletMeta = WalletMeta;
-
-    fn fee_rule(&self) -> &Self::FeeRule {
-        &self.fee_rule
-    }
-
-    fn fetch_wallet_meta(
-        &self,
-        meta_source: &Self::MetaSource,
-        account: <Self::MetaSource as InputSource>::AccountId,
-        exclude: &[<Self::MetaSource as InputSource>::NoteRef],
-    ) -> Result<Self::WalletMeta, <Self::MetaSource as InputSource>::Error> {
-        meta_source.get_wallet_metadata(account, self.split_policy.min_split_output_size(), exclude)
-    }
-
-    fn compute_balance<P: consensus::Parameters, NoteRefT: Clone>(
-        &self,
-        params: &P,
-        target_height: BlockHeight,
-        transparent_inputs: &[impl transparent::InputView],
-        transparent_outputs: &[impl transparent::OutputView],
-        sapling: &impl sapling_fees::BundleView<NoteRefT>,
-        #[cfg(feature = "orchard")] orchard: &impl orchard_fees::BundleView<NoteRefT>,
-        ephemeral_balance: Option<&EphemeralBalance>,
-        wallet_meta: Option<&Self::WalletMeta>,
-    ) -> Result<TransactionBalance, ChangeError<Self::Error, NoteRefT>> {
-        let cfg = SinglePoolBalanceConfig::new(
-            params,
-            &self.fee_rule,
-            &self.dust_output_policy,
-            self.fee_rule.marginal_fee(),
-            &self.split_policy,
-            self.fallback_change_pool,
-            self.fee_rule.marginal_fee(),
-            self.fee_rule.grace_actions(),
-        );
-
-        single_pool_output_balance(
-            cfg,
-            wallet_meta,
-            target_height,
-            transparent_inputs,
-            transparent_outputs,
-            sapling,
-            #[cfg(feature = "orchard")]
-            orchard,
-            self.change_memo.as_ref(),


This all moved to CommonChangeStrategy. I intentionally used the most non-committal name possible so that we don't have to change it when we (inevitably) generalize again.

daira · 2024-10-21T02:34:34Z

zcash_client_backend/src/fees/common.rs

+            params,
+            &self.fee_rule,
+            &self.dust_output_policy,
+            self.fee_rule.default_dust_threshold(),


Note that we have to distinguish default_dust_threshold() from marginal_fee() in order to replicate the behaviour of the current fixed fee rule. (There was one test, fees::fixed::tests::dust_change, that failed if we didn't do that.)

codecov-commenter · 2024-10-21T02:35:46Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 61.76471% with 39 lines in your changes missing coverage. Please review.

Please upload report for BASE (wallet/multi_output_change_strategy@5afea2e). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
zcash_client_backend/src/fees/standard.rs	0.00%	12 Missing ⚠️
zcash_client_backend/src/fees.rs	0.00%	7 Missing ⚠️
zcash_client_backend/src/data_api/testing/pool.rs	50.00%	6 Missing ⚠️
zcash_client_backend/src/proto.rs	0.00%	5 Missing ⚠️
zcash_client_backend/src/fees/fixed.rs	0.00%	4 Missing ⚠️
zcash_client_backend/src/fees/zip317.rs	0.00%	4 Missing ⚠️
...ent_backend/src/data_api/wallet/input_selection.rs	66.66%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@                          Coverage Diff                           @@
##             wallet/multi_output_change_strategy      #40   +/-   ##
======================================================================
  Coverage                                       ?   62.04%           
======================================================================
  Files                                          ?      149           
  Lines                                          ?    19254           
  Branches                                       ?        0           
======================================================================
  Hits                                           ?    11947           
  Misses                                         ?     7307           
  Partials                                       ?        0

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

daira · 2024-10-21T02:41:24Z

zcash_primitives/src/transaction/fees/fixed.rs

@@ -60,6 +50,18 @@ impl super::FeeRule for FeeRule {
    ) -> Result<NonNegativeAmount, Self::Error> {
        Ok(self.fixed_fee)
    }
+
+    fn default_dust_threshold(&self) -> NonNegativeAmount {
+        self.fixed_fee


This is slightly weird but it is what the previous code did.

daira · 2024-10-21T02:42:16Z

zcash_primitives/src/transaction/fees/zip317.rs

@@ -83,7 +83,7 @@ impl FeeRule {
                 || 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. \
-                This API is likely to be removed. Use `[FeeRule::standard]` instead."
+                This API is likely to be removed. Use [`FeeRule::standard`] instead."


I'm itching to remove this one as well. Pretty please?

nuttycom · 2024-10-21T03:00:01Z

zcash_client_backend/CHANGELOG.md

@@ -13,8 +13,10 @@ and this library adheres to Rust's notion of
  - `WalletSummary::progress`
  - `WalletMeta`
  - `impl Default for wallet::input_selection::GreedyInputSelector`
- `zcash_client_backend::fees::SplitPolicy`
- `zcash_client_backend::fees::zip317::MultiOutputChangeStrategy`
+- `zcash_client_backend::fees::{SplitPolicy, CommonChangeStrategy}`. These


Instead of CommonChangeStrategy, we should probably call this SinglePoolChangeStrategy as the distinguishing feature is now that it sends change to a single pool.

nuttycom

Concept ACK on the consolidation, but I have a number of complaint about the type changes being introduced; specifically, DummyMetaSource.

Also the Common and simple names are not helpful.

nuttycom · 2024-10-21T03:01:42Z

zcash_client_backend/src/data_api.rs

@@ -907,7 +907,7 @@ pub trait InputSource {
        account: Self::AccountId,
        min_value: NonNegativeAmount,
        exclude: &[Self::NoteRef],
-    ) -> Result<WalletMeta, Self::Error>;
+    ) -> Result<Option<WalletMeta>, Self::Error>;


I'm not sure about this change semantically - I guess Ok(None) means that wallet metadata cannot be determined?

nuttycom · 2024-10-21T03:02:50Z

zcash_client_backend/src/data_api/testing/pool.rs

    let change_memo = "Test change memo".parse::<Memo>().unwrap();
-    let change_strategy = standard::SingleOutputChangeStrategy::new(
-        fee_rule,
+    let change_strategy = CommonChangeStrategy::simple(


nit: I don't like simple as a name for a constructor; it doesn't give me any information about what it actually does.

nuttycom · 2024-10-21T03:06:19Z

zcash_client_backend/src/fees.rs

@@ -506,6 +488,43 @@ pub trait ChangeStrategy {
    ) -> Result<TransactionBalance, ChangeError<Self::Error, NoteRefT>>;
 }

+pub struct DummyMetaSource;


I very much dislike this being part of the public API.

nuttycom · 2024-10-21T03:06:52Z

zcash_client_backend/src/fees/common.rs

 };

 #[cfg(feature = "orchard")]
 use super::orchard as orchard_fees;

+/// A change strategy that attempts to split the change value into some number of equal-sized notes
+/// as dictated by the included [`SplitPolicy`] value.
+pub struct CommonChangeStrategy<I, FR> {


Suggested change

pub struct CommonChangeStrategy<I, FR> {

pub struct SinglePoolChangeStrategy<I, FR> {

And related changes elsewhere

nuttycom · 2024-10-21T03:07:33Z

zcash_client_backend/src/fees/common.rs

+    /// and a single change output. This is equivalent to [`CommonChangeStrategy::new`] with
+    /// `dust_output_policy == DustOutputPolicy::default()` and
+    /// `split_policy == SplitPolicy::single_output()`.
+    pub fn simple(


Suggested change

pub fn simple(

pub fn single_output(

and affected call sites

nuttycom · 2024-10-21T03:09:07Z

zcash_client_backend/src/fees/common.rs

+
+impl<I: InputSource, FR: FeeRule + Clone> ChangeStrategy for CommonChangeStrategy<I, FR> {
+    type FeeRule = FR;
+    type Error = FR::FeeRuleOrBalanceError;


This doesn't look like something that should be an associated type; it should not be necessary to modify zcash_primitives to introduce this change. Instead, have an enum that wraps the fee rule error.

nuttycom · 2024-10-21T03:14:19Z

zcash_client_backend/src/fees.rs

+impl InputSource for DummyMetaSource {
+    type Error = ();
+    type AccountId = ();
+    type NoteRef = ();
+
+    fn get_spendable_note(
+        &self,
+        _txid: &TxId,
+        _protocol: ShieldedProtocol,
+        _index: u32,
+    ) -> Result<Option<ReceivedNote<Self::NoteRef, Note>>, Self::Error> {
+        Err(())
+    }
+
+    fn select_spendable_notes(
+        &self,
+        _account: Self::AccountId,
+        _target_value: NonNegativeAmount,
+        _sources: &[ShieldedProtocol],
+        _anchor_height: BlockHeight,
+        _exclude: &[Self::NoteRef],
+    ) -> Result<SpendableNotes<Self::NoteRef>, Self::Error> {
+        Err(())
+    }
+
+    fn get_wallet_metadata(
+        &self,
+        _account: Self::AccountId,
+        _min_value: NonNegativeAmount,
+        _exclude: &[Self::NoteRef],
+    ) -> Result<Option<WalletMeta>, Self::Error> {
+        Err(())
+    }
+}


NACK on this introduction; a InputSource type that errors on every call is the wrong way here. The PhantomData solution was fine.

nuttycom · 2024-10-21T03:22:16Z

zcash_primitives/src/transaction/fees.rs

+
+    /// Returns the default dust threshold.
+    fn default_dust_threshold(&self) -> NonNegativeAmount;
+
+    /// Returns the marginal fee, i.e. the amount by which the fee increases for each
+    /// logical action as defined in ZIP 317.
+    fn marginal_fee(&self) -> NonNegativeAmount;
+
+    /// Returns the number of grace actions, or 0 if this rule does not use grace actions.
+    fn grace_actions(&self) -> usize;


NACK, these are all ZIP 317 specific.

nuttycom · 2024-10-21T03:22:25Z

zcash_primitives/src/transaction/fees.rs

    Zip317,
 }

 impl FeeRule for StandardFeeRule {
    type Error = zip317::FeeError;
+    type FeeRuleOrBalanceError = zip317::FeeError;


NACK on this change; this error type is unused in the zcash_primitives crate and doesn't justify its existence.

nuttycom · 2024-10-21T03:23:27Z

zcash_primitives/src/transaction/fees/zip317.rs

-    /// Returns the ZIP 317 marginal fee.
-    pub fn marginal_fee(&self) -> NonNegativeAmount {
-        self.marginal_fee
-    }
-    /// Returns the ZIP 317 number of grace actions
-    pub fn grace_actions(&self) -> usize {
-        self.grace_actions
-    }


These are intentionally here, instead of in the general trait, because they're concepts specific to ZIP 317.

nuttycom · 2024-10-21T03:48:30Z

In general, the way that I would approach this change instead is to deprecate/delete zcash_client_backend::fees::{standard, fixed} but leave the fee infrastructure in zcash_primitives unchanged.

zcash_client_backend is intended to be an opinionated client layer, and I'm fine with the opinion "we only support ZIP 317". zcash_primitives is intended to represent the Zcash protocol; as such, it should not be opinionated in the same fashion.

nuttycom and others added 9 commits October 20, 2024 13:55

zcash_client_backend: Use an explicit struct for change output counts…

c80893b

… instead of a tuple.

zcash_client_backend: Add WalletMeta type & retrieval method.

f8970b0

zcash_client_backend: Make it possible for change strategies to use w…

ff00f1a

…allet metadata. In the process this modifies input selection to take the change strategy as an explicit argument, rather than being wrapped as part of the input selector.

zcash_client_backend: Clean up arguments to `single_change_output_bal…

56f5c94

…ance`

zcash_client_backend: Add fees::zip317::MultiOutputChangeStrategy.

ebca2ec

Apply suggestions from code review

5afea2e

Co-authored-by: Jack Grigg <thestr4d@gmail.com> Co-authored-by: Daira-Emma Hopwood <daira@jacaranda.org>

Remove fixed::FeeRule::standard (which was misleadingly named because

f58988a

fixed fees are not standard), and deprecate `fixed::FeeRule::non_standard`. Signed-off-by: Daira-Emma Hopwood <daira@jacaranda.org>

daira commented Oct 21, 2024

View reviewed changes

nuttycom reviewed Oct 21, 2024

View reviewed changes

nuttycom requested changes Oct 21, 2024

View reviewed changes

nuttycom reviewed Oct 21, 2024

View reviewed changes

nuttycom force-pushed the wallet/multi_output_change_strategy branch 2 times, most recently from 4059a29 to 47b1065 Compare October 23, 2024 17:24

nuttycom deleted the branch nuttycom:wallet/multi_output_change_strategy October 24, 2024 14:54

nuttycom closed this Oct 24, 2024

nuttycom mentioned this pull request Oct 24, 2024

zcash_client_backend: Remove deprecated and duplicative wallet APIs. zcash/librustzcash#1590

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify change strategies, and deprecate or remove fee rules that no longer produce minable transactions #40

Simplify change strategies, and deprecate or remove fee rules that no longer produce minable transactions #40

daira commented Oct 21, 2024

daira Oct 21, 2024

daira Oct 21, 2024

codecov-commenter commented Oct 21, 2024

daira Oct 21, 2024

daira Oct 21, 2024

nuttycom Oct 21, 2024

nuttycom left a comment

nuttycom Oct 21, 2024

nuttycom Oct 21, 2024

nuttycom Oct 21, 2024

nuttycom Oct 21, 2024

nuttycom Oct 21, 2024

nuttycom Oct 21, 2024

nuttycom Oct 21, 2024

nuttycom Oct 21, 2024

nuttycom Oct 21, 2024

nuttycom Oct 21, 2024

nuttycom commented Oct 21, 2024 •

edited

Loading

	pub struct CommonChangeStrategy<I, FR> {
	pub struct SinglePoolChangeStrategy<I, FR> {

Simplify change strategies, and deprecate or remove fee rules that no longer produce minable transactions #40

Simplify change strategies, and deprecate or remove fee rules that no longer produce minable transactions #40

Conversation

daira commented Oct 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 21, 2024

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nuttycom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nuttycom commented Oct 21, 2024 • edited Loading

nuttycom commented Oct 21, 2024 •

edited

Loading