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

zcash_client_sqlite: Disallow selection of dust notes. #1312

Merged
merged 2 commits into from
Mar 25, 2024

Conversation

nuttycom
Copy link
Contributor

At present, we don't take builder padding rules into account in the determination of whether or not we can include spends of dust notes in grace actions. It's too complex to attempt a complete fix for Zashi 1.0, but we can have a tolerable workaround for launch by just never selecting dust-valued notes.

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.

Reviewed d6f841d. Concept ACK, but a test is failing.

zcash_client_sqlite/src/wallet/common.rs Outdated Show resolved Hide resolved
At present, we don't take builder padding rules into account in the
determination of whether or not we can include spends of dust notes in
grace actions. It's too complex to attempt a complete fix for Zashi 1.0,
but we can have a tolerable workaround for launch by just never
selecting dust-valued notes.

This is a temporary solution; opened #1316 to track the resolution.
@nuttycom nuttycom force-pushed the avoid_dust_note_selection branch from d6f841d to 0d8f569 Compare March 25, 2024 19:02
str4d
str4d previously approved these changes Mar 25, 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 0d8f569

Copy link

codecov bot commented Mar 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.35%. Comparing base (181e898) to head (0d8f569).
Report is 10 commits behind head on main.

❗ Current head 0d8f569 differs from pull request most recent head bda72e3. Consider uploading reports for the commit bda72e3 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1312      +/-   ##
==========================================
- Coverage   63.44%   63.35%   -0.09%     
==========================================
  Files         121      121              
  Lines       14142    14143       +1     
==========================================
- Hits         8972     8960      -12     
- Misses       5170     5183      +13     

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

@@ -597,6 +597,7 @@ pub(crate) mod tests {

#[test]
#[ignore] // FIXME: #1316 This requires support for dust outputs.
#[cfg(not(feature = "expensive-tests"))]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are these cfg(not(...)) instead of just cfg(...)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, it's annoying. CI uses the -- --ignored flag to run just the ignored tests. So I want this test to actually be ignored, so I made the slow tests CI workflow use the expensive-tests feature, and then I opt this out of the slow tester by adding this cfg attr.

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 bda72e3

@nuttycom nuttycom merged commit 304e565 into main Mar 25, 2024
24 checks passed
@nuttycom nuttycom deleted the avoid_dust_note_selection branch March 25, 2024 19:45
Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

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

Post-hoc ACK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants