Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improvements to the strategy implemented by {fixed,standard,zip317}::SingleOutputChangeStrategy #1430

Merged
merged 1 commit into from
Jun 19, 2024

Conversation

daira
Copy link
Contributor

@daira daira commented Jun 18, 2024

zcash_client_backend::{fixed,standard,zip317}::SingleOutputChangeStrategy now implement a different strategy for choosing whether there will be any change, and its value. The aims are:

  • Ensure that it is still possible to create fully transparent transactions with no change (this will be needed for ZIP 320). The ChangeError::InsufficientFunds error in this case should have a required field that reflects the additional amount needed, according to the fee calculated without an extra change output.
  • Avoid leaking information about note amounts in some cases: an adversary that knew the number of external recipients and the sum of their outputs was able to learn the sum of the inputs if no change output was present.
  • Defend against losing money by using DustAction::AddDustToFee with a too-high dust threshold.
  • Ensure that if a "change memo" is requested, there will always be a shielded change output in which to put it. Previously, this would not be the case when using DustAction::AddDustToFee.

Copy link

codecov bot commented Jun 18, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 21 lines in your changes missing coverage. Please review.

Project coverage is 63.16%. Comparing base (06c0895) to head (7710919).

Current head 7710919 differs from pull request most recent head 21d5731

Please upload reports for the commit 21d5731 to get more accurate results.

Files Patch % Lines
zcash_client_backend/src/fees/common.rs 75.00% 21 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1430      +/-   ##
==========================================
+ Coverage   63.08%   63.16%   +0.08%     
==========================================
  Files         127      127              
  Lines       14821    14868      +47     
==========================================
+ Hits         9350     9392      +42     
- Misses       5471     5476       +5     

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

zcash_client_sqlite/src/testing/pool.rs Outdated Show resolved Hide resolved
zcash_client_sqlite/src/testing/pool.rs Outdated Show resolved Hide resolved
zcash_client_sqlite/src/testing/pool.rs Outdated Show resolved Hide resolved
str4d
str4d previously approved these changes Jun 18, 2024
Copy link
Contributor

@str4d str4d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 977e6c8 modulo questions about tests. Any that are not addressed here, should have issues opened.

@daira daira added the A-wallet Area: light wallet backend. label Jun 19, 2024
…ategy`

now implement a different strategy for choosing whether there will be any
change, and its value. The aims are:

* Ensure that it is possible to create fully transparent transactions with
  no change (this will be needed for ZIP 320). The `InsufficientFunds`
  error in this case should have a `required` field that reflects the
  additional amount needed, according to the fee calculated without an
  extra change output.
* Avoid leaking information about note amounts in some cases: an adversary
  that knew the number of external recipients and the sum of their outputs
  was able to learn the sum of the inputs if no change output was present.
* Defend against losing money by using `DustAction::AddDustToFee` with a
  too-high dust threshold.
* Ensure that if a "change memo" is requested, there will always be a
  shielded change output in which to put it. Previously, this would not
  be the case when using `DustAction::AddDustToFee`.

Co-authored-by: Jack Grigg <jack@electriccoin.co>
Co-authored-by: Kris Nuttycombe <kris@nutty.land>
Signed-off-by: Daira-Emma Hopwood <daira@jacaranda.org>
Copy link
Contributor

@str4d str4d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 21d5731

@str4d str4d merged commit 4b2a942 into zcash:main Jun 19, 2024
25 checks passed
@daira daira deleted the fee-semantic-change branch June 19, 2024 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-wallet Area: light wallet backend.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants