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: Add Orchard wallet support #1182

Merged
merged 14 commits into from
Mar 12, 2024

Conversation

nuttycom
Copy link
Contributor

@nuttycom nuttycom commented Feb 8, 2024

Closes #1087.
Closes #1177.

@nuttycom nuttycom force-pushed the sqlite_wallet/orchard_support branch 2 times, most recently from 614a424 to c118979 Compare March 6, 2024 03:02
Copy link

codecov bot commented Mar 6, 2024

Codecov Report

Attention: Patch coverage is 60.90909% with 172 lines in your changes are missing coverage. Please review.

Project coverage is 63.30%. Comparing base (a9aabb2) to head (5a28970).

Files Patch % Lines
zcash_client_sqlite/src/wallet/orchard.rs 0.00% 67 Missing ⚠️
zcash_client_sqlite/src/wallet.rs 51.14% 64 Missing ⚠️
zcash_client_sqlite/src/lib.rs 14.81% 23 Missing ⚠️
zcash_client_backend/src/fees.rs 0.00% 9 Missing ⚠️
zcash_client_sqlite/src/wallet/common.rs 94.33% 6 Missing ⚠️
zcash_client_backend/src/fees/common.rs 95.00% 2 Missing ⚠️
...ent_backend/src/data_api/wallet/input_selection.rs 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1182      +/-   ##
==========================================
- Coverage   63.38%   63.30%   -0.08%     
==========================================
  Files         120      121       +1     
  Lines       13124    13450     +326     
==========================================
+ Hits         8318     8514     +196     
- Misses       4806     4936     +130     

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

@nuttycom nuttycom force-pushed the sqlite_wallet/orchard_support branch 3 times, most recently from fc37367 to 464bcf7 Compare March 7, 2024 17:02
@nuttycom nuttycom changed the title WIP: zcash_client_sqlite: Add support for Orchard scanning to the scan queue. zcash_client_sqlite: Add Orchard wallet support Mar 7, 2024
@nuttycom nuttycom force-pushed the sqlite_wallet/orchard_support branch 2 times, most recently from ebe35e7 to bc0507b Compare March 8, 2024 21:29
@nuttycom nuttycom marked this pull request as ready for review March 8, 2024 21:29
@str4d str4d force-pushed the sqlite_wallet/orchard_support branch 2 times, most recently from 8f58e9e to 7a48efb Compare March 8, 2024 23:00
"UPDATE sapling_received_notes SET spent = NULL WHERE EXISTS (
SELECT id_tx FROM transactions
WHERE id_tx = sapling_received_notes.spent AND block IS NULL AND expiry_height < ?
)",
)?;
stmt_update_expired.execute([u32::from(expiry_height)])?;
stmt_update_sapling_expired.execute([u32::from(expiry_height)])?;
let mut stmt_update_orchard_expired = conn.prepare_cached(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This technically isn't necessary unless the orchard feature is enabled. It is, however, safe regardless.

@nuttycom nuttycom force-pushed the sqlite_wallet/orchard_support branch from 7a48efb to 95d3c85 Compare March 9, 2024 17:52
@str4d str4d force-pushed the sqlite_wallet/orchard_support branch from 95d3c85 to 0084326 Compare March 9, 2024 21:13
@nuttycom nuttycom force-pushed the sqlite_wallet/orchard_support branch 5 times, most recently from 68667c4 to 4ef9452 Compare March 10, 2024 20:12
@str4d str4d force-pushed the sqlite_wallet/orchard_support branch from 48ab24b to a017717 Compare March 10, 2024 21:42
@nuttycom nuttycom force-pushed the sqlite_wallet/orchard_support branch 3 times, most recently from 0036600 to e61fd81 Compare March 11, 2024 00:01
@str4d str4d force-pushed the sqlite_wallet/orchard_support branch 2 times, most recently from d260064 to 08d131b Compare March 11, 2024 01:50
@nuttycom nuttycom force-pushed the sqlite_wallet/orchard_support branch 2 times, most recently from 8cd2f2b to 52257f3 Compare March 11, 2024 03:22
@str4d str4d force-pushed the sqlite_wallet/orchard_support branch from b32d449 to 85932db Compare March 11, 2024 11:38
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 d68a01a

)
)?);
#[cfg(feature = "orchard")]
let received_iter = received_iter.chain(wallet::orchard::select_spendable_orchard_notes(
Copy link
Contributor

Choose a reason for hiding this comment

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

This is ignoring the _sources parameter to select_spendable_notes, and always selecting from both pools. It happens to cause no bugs because zcash_client_backend hard-codes both pools as sources if the feature flags are enabled, but we should fix this anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be fixed by the third commit of #1244 (which I'm in the process of extracting out on its own.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Whether or not we include the third commit of #1244 in Zashi 1.0 (and we might not), we should fix this because it's a bug in the current implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Opened #1260.

zcash_client_sqlite/src/wallet/orchard.rs Show resolved Hide resolved
zcash_client_sqlite/src/wallet/common.rs Outdated Show resolved Hide resolved
zcash_client_sqlite/src/wallet/common.rs Outdated Show resolved Hide resolved
zcash_client_sqlite/src/wallet/orchard.rs Outdated Show resolved Hide resolved
zcash_client_sqlite/src/wallet/orchard.rs Outdated Show resolved Hide resolved
zcash_client_sqlite/src/wallet.rs Show resolved Hide resolved
@nuttycom nuttycom force-pushed the sqlite_wallet/orchard_support branch from 97e5e9f to 264fc95 Compare March 12, 2024 16:15
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 264fc95.

zcash_client_sqlite/src/wallet.rs Outdated Show resolved Hide resolved
@nuttycom nuttycom force-pushed the sqlite_wallet/orchard_support branch from 264fc95 to 9452698 Compare March 12, 2024 16:29
str4d
str4d previously approved these changes Mar 12, 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 9452698

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.

Re-utACK 5a28970

@nuttycom nuttycom merged commit bb466de into zcash:main Mar 12, 2024
20 of 25 checks passed
@nuttycom nuttycom deleted the sqlite_wallet/orchard_support branch March 12, 2024 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants