Add dynamic note selection that is responsive to changes in required fees. by nuttycom · Pull Request #689 · zcash/librustzcash

nuttycom · 2022-10-31T22:53:57Z

This PR is best reviewed hiding whitespace changes.

codecov · 2022-11-03T00:24:42Z

Codecov Report

Base: 72.58% // Head: 71.29% // Decreases project coverage by -1.29% ⚠️

Coverage data is based on head (847ba49) compared to base (6f4a6aa).
Patch coverage: 53.21% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #689      +/-   ##
==========================================
- Coverage   72.58%   71.29%   -1.30%     
==========================================
  Files         107      113       +6     
  Lines       10932    11387     +455     
==========================================
+ Hits         7935     8118     +183     
- Misses       2997     3269     +272

Impacted Files	Coverage Δ
zcash_client_backend/src/address.rs	`86.40% <ø> (ø)`
zcash_client_backend/src/data_api.rs	`25.00% <0.00%> (+0.64%)`	⬆️
zcash_client_backend/src/zip321.rs	`96.20% <0.00%> (-0.82%)`	⬇️
zcash_client_sqlite/src/error.rs	`4.76% <0.00%> (-3.94%)`	⬇️
zcash_extensions/src/transparent/demo.rs	`76.62% <ø> (ø)`
zcash_primitives/src/sapling.rs	`86.93% <ø> (ø)`
...sh_primitives/src/transaction/components/amount.rs	`68.83% <0.00%> (-5.82%)`	⬇️
...imitives/src/transaction/components/transparent.rs	`91.46% <ø> (+1.21%)`	⬆️
zcash_client_backend/src/data_api/error.rs	`7.84% <12.12%> (-8.16%)`	⬇️
zcash_client_backend/src/wallet.rs	`68.00% <20.00%> (-32.00%)`	⬇️
... and 30 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

pacu · 2022-11-03T17:48:07Z

This will surely be a UX problem. Is there a way of doing a dry-run to know the estimated fee for a given amount to show on wallets' send flow? Waiting for the user to complete the whole transaction to deny it because of variable fees not calculated well beforehand seems pretty inconvenient

Usually insufficient funds will be detected at note selection time, so the build-time check is really just a fall-back to ensure that the transaction doesn't violate balance rules. Also, the next generation of transaction construction will provide an intermediate result that has the privacy implications of the transaction listed out for the user's approval.

Also, the next generation of transaction construction will provide an intermediate result that has the privacy implications of the transaction listed out for the user's approval.

I'm not following you here. In terms of wallet UX, can we have a function that given an amount, it will return an approximate fee so that wallet developers can validate their "send funds" forms UI?

There's a subtlety here which is that we would need such a function to essentially prepare the transaction and "reserve" the notes/UTXOs that would be spent, so that we can be sure that the fee doesn't change between the time that the UI displays the fee and the time that the user goes to actually confirm the transaction; for example, if a new note comes in that would be preferentially selected by the note selection algorithm, the fees and the notes being spent could change in that interim. So, what we need to do is what we've discussed before: prepare the transaction and lock the notes to be spent to it, and then return that prepared transaction (along with its privacy implications) to be confirmed by the user. The fee should be displayed on the confirmation page.

The problem with this approach is that it doesn't solve the problem I'm referring to, which is: "don't let the user proceed to a place that inevitably is an error state if it can be known beforehand".

The SDK will update every 20~ seconds and being the user in the send flow, it will refresh the available information and that information in the local state will be used to create the transaction. Any changes in balance should trigger a UI refresh in send flow so that it can be validated against the latest current state.

We do know that there are certain combinations of amount and notes used that will make the fees an impediment for the transaction to be sent. The "dry-run" function I'm proposing is aimed to catch and report that particular situation so that wallet developers can validate their send form before proceeding. There will be situations like the one you mentioned where timing and chance will plot against it. *

Also, it can be the case that a wallet developer does not want a confirmation screen. So this additional step to safety check will not be present. Which makes this validation function more helpful to avoid failed transactions for consensus reasons which are a pain to explain to users.

I hadn't thought of this. Can users be dusted into oblivion thanks to these new fee scheme?

No; it is the responsibility of the ZIP 321 note selection implementation to ignore notes that are below the value of the fee required to spend them (though low-valued notes can have their value recovered by using them as a free input when spending a note that is by itself large enough to cover the full transaction amount.)

In any case, this error needs to exist because we must always validate the transaction at construction time anyway. The transaction preparation step you're talking about is effectively the input selection step itself; it's been renamed 'propose_transaction' to make this a bit clearer: https://github.com/zcash/librustzcash/pull/689/files#diff-fedecff319cb43f7a4f8b913cdee2332ba452a6310e2470cf6cc7631ca83291cR126

str4d

Partial review.

nuttycom · 2022-11-08T17:15:59Z

force-pushed to address review comments and integrate change selection more intimately with fee estimation.

str4d

Reviewed a9e955e862986abcf1c88a418b1234def97126ce. Overall this is looking good!

nuttycom · 2022-11-10T01:35:21Z

force-pushed to address review comments and add handling of dust inputs.

str4d

utACK bd16035c77163606ff24658569d29a0bfbcbe892 modulo comments.

str4d · 2022-11-10T05:42:54Z

Suggested change

- `impl {PartialOrd, Ord} for zcash_primitves::transaction::components::transparent::OutPoint`

- `impl {PartialOrd, Ord} for zcash_primitives::transaction::components::transparent::OutPoint`

This adds a set of abstractions that allow wallets to provide independent strategies for fee estimation and note selection, and implementations of these strategies that perform these operations in the same fashion as the existing `spend` and `shield_transparent_funds` functions. This required a somewhat hefty rework of the error handling in zcash_client_backend. It fixes an issue with the error types whereby callees needed to have a bit too much information about the error types produced by their callers. Reflect the updated note selection and error handling in zcash_client_sqlite.

The change selection algorithm has the most useful information for determining whether or not a note is dust, so this adds a new error case to `ChangeError` that allows the change selection to report the presence of input notes without economic value back to its caller.

nuttycom · 2022-11-10T19:24:41Z

@str4d I realized that I hadn't properly accounted for handling dust outputs in the change APIs, so I have force-pushed the second commit here to address the problem: https://github.com/zcash/librustzcash/compare/bd16035c77163606ff24658569d29a0bfbcbe892..847ba49761fdfec1213a6d6417db6165c549241e

str4d

utACK 847ba49 mod comments.

str4d · 2022-11-10T20:55:01Z

  - `TransactionBalance`
  - `BasicFixedFeeChangeStrategy` - a `ChangeStrategy` implementation that
    reproduces current wallet change behavior
+  - `fixed` a new module containing of change selection strategies for the


Suggested change

- `fixed` a new module containing of change selection strategies for the

- `fixed`, a new module containing of change selection strategies for the

(or : as was used above)

str4d · 2022-11-10T20:59:31Z

  and types related to fee calculations.
  - `FeeRule` a trait that describes how to compute the fee required for a
    transaction given inputs and outputs to the transaction.
+  - `zcash_primitives::transaction::fees::fixed` a new module containing an implementation


Suggested change

- `zcash_primitives::transaction::fees::fixed` a new module containing an implementation

- `fixed`, a new module containing an implementation

nuttycom · 2022-11-10T21:01:53Z

I will address the changelog comments in #694, which will follow immediately.

daira · 2022-11-11T00:52:38Z

+  - `chain::error`: a module containing error types type that that can occur only
+    in chain validation and sync have been separated out from errors related to
+    other wallet operations.


Suggested change

- `chain::error`: a module containing error types type that that can occur only

in chain validation and sync have been separated out from errors related to

other wallet operations.

- `chain::error`: a module containing error types that can occur only in chain

validation and sync, which have been separated out from errors related to

other wallet operations.

daira · 2022-11-11T00:56:08Z

+  to the function has been modified to take a unified spending key instead of a
+  Sapling extended spending key, and now also requires a `min_confirmations`


Suggested change

to the function has been modified to take a unified spending key instead of a

Sapling extended spending key, and now also requires a `min_confirmations`

to the function have been modified to take a unified spending key instead of a

Sapling extended spending key, and it now also requires a `min_confirmations`

daira · 2022-11-12T01:29:07Z

+    /// Performs input selection and returns a proposal for the construction of a shielding
+    /// transaction.
+    ///
+    /// Implementations should return the maximum possible number of economically useful inputs


Is "maximum possible" correct? That would be all the notes held by the wallet.

I don't think "minimum possible" is correct either; while it's not useful to return additional unneeded notes, there's no guarantee or requirement that the returned set of notes be the minimum possible number.

nuttycom mentioned this pull request Oct 31, 2022

Update the transaction builder to make change outputs explicit #658

Merged

nuttycom force-pushed the wallet/fee_abstraction branch 2 times, most recently from 26858fb to 0f8465a Compare November 2, 2022 23:47

pacu reviewed Nov 3, 2022

View reviewed changes

nuttycom force-pushed the wallet/fee_abstraction branch 2 times, most recently from ec80d6e to d685530 Compare November 4, 2022 01:57

nuttycom mentioned this pull request Nov 4, 2022

Add ZIP 317 fee calculation strategy. #694

Merged

nuttycom force-pushed the wallet/fee_abstraction branch 7 times, most recently from d66cbfc to 3c6af22 Compare November 5, 2022 20:03

nuttycom marked this pull request as ready for review November 5, 2022 20:03

nuttycom requested review from daira, sellout and str4d November 5, 2022 20:03

nuttycom force-pushed the wallet/fee_abstraction branch 3 times, most recently from e400f8a to f120a01 Compare November 8, 2022 01:17

str4d reviewed Nov 8, 2022

View reviewed changes

nuttycom force-pushed the wallet/fee_abstraction branch 4 times, most recently from 0504f41 to f0cff43 Compare November 8, 2022 17:13

nuttycom force-pushed the wallet/fee_abstraction branch 2 times, most recently from e70366d to d14b52d Compare November 9, 2022 00:54

nuttycom force-pushed the wallet/fee_abstraction branch 2 times, most recently from bca3122 to a9e955e Compare November 9, 2022 02:57

str4d requested changes Nov 9, 2022

View reviewed changes

nuttycom force-pushed the wallet/fee_abstraction branch 4 times, most recently from 254f20c to ff72cfe Compare November 9, 2022 23:55

nuttycom requested a review from str4d November 10, 2022 01:14

nuttycom force-pushed the wallet/fee_abstraction branch 3 times, most recently from e55a83c to bd16035 Compare November 10, 2022 01:33

str4d approved these changes Nov 10, 2022

View reviewed changes

nuttycom added 2 commits November 10, 2022 12:17

nuttycom force-pushed the wallet/fee_abstraction branch from bd16035 to 847ba49 Compare November 10, 2022 19:23

nuttycom requested a review from str4d November 10, 2022 19:24

daira reviewed Nov 10, 2022

View reviewed changes

Comment thread zcash_client_backend/src/fees.rs

daira reviewed Nov 10, 2022

View reviewed changes

Comment thread zcash_client_backend/src/fees.rs

r3ld3v added this to the UAs in Mobile SDKs milestone Nov 10, 2022

str4d approved these changes Nov 10, 2022

View reviewed changes

daira reviewed Nov 11, 2022

View reviewed changes

nuttycom merged commit 847ba49 into zcash:main Nov 11, 2022

nuttycom deleted the wallet/fee_abstraction branch November 11, 2022 02:08

daira reviewed Nov 12, 2022

View reviewed changes

nuttycom mentioned this pull request Dec 1, 2022

Expose transaction proposal via the zcash_client_backend API #716

Closed

str4d mentioned this pull request Feb 6, 2024

Implement ZIP 317 fees #1166

Closed

	- `impl {PartialOrd, Ord} for zcash_primitves::transaction::components::transparent::OutPoint`
	- `impl {PartialOrd, Ord} for zcash_primitives::transaction::components::transparent::OutPoint`

	- `fixed` a new module containing of change selection strategies for the
	- `fixed`, a new module containing of change selection strategies for the

	- `zcash_primitives::transaction::fees::fixed` a new module containing an implementation
	- `fixed`, a new module containing an implementation

		to the function has been modified to take a unified spending key instead of a
		Sapling extended spending key, and now also requires a `min_confirmations`

Conversation

nuttycom commented Oct 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pacu Nov 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

str4d left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nuttycom commented Nov 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

str4d left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nuttycom commented Nov 10, 2022

Uh oh!

str4d left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nuttycom commented Nov 10, 2022

Uh oh!

Uh oh!

Uh oh!

str4d left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nuttycom commented Nov 10, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nuttycom commented Oct 31, 2022 •

edited

Loading

codecov Bot commented Nov 3, 2022 •

edited

Loading

pacu Nov 8, 2022 •

edited

Loading

nuttycom commented Nov 8, 2022 •

edited

Loading