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_backend: Allow sharing instances of ScanningKeys between threads #1400

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

upbqdn
Copy link
Contributor

@upbqdn upbqdn commented May 29, 2024

Motivation

It would be handy if Zebra could share instances of ScanningKeys between threads.

Solution

  • Bound ScanningKeyOps by Send + Sync so ScanningKeys auto-implements Send and Sync instead of !Send and !Sync.

@upbqdn upbqdn added enhancement rust Pull requests that update Rust code labels May 29, 2024
Bound `ScanningKeyOps` by `Send + Sync` so `ScanningKeys`
auto-implements `Send` and `Sync` instead of `!Send` and `!Sync`.
Copy link

codecov bot commented May 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.13%. Comparing base (976a4d2) to head (99dfbb9).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1400   +/-   ##
=======================================
  Coverage   63.13%   63.13%           
=======================================
  Files         126      126           
  Lines       14720    14720           
=======================================
  Hits         9293     9293           
  Misses       5427     5427           

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

@@ -120,7 +120,7 @@ pub struct ScanningKey<Ivk, Nk, AccountId> {
key_scope: Option<Scope>,
}

impl<AccountId> ScanningKeyOps<SaplingDomain, AccountId, sapling::Nullifier>
impl<AccountId: Send + Sync> ScanningKeyOps<SaplingDomain, AccountId, sapling::Nullifier>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this bound (repeated everywhere) avoidable if we add the equivalent bound on AccountId in ScanningKeyOps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, to be able to move an instance of ScannningKeys to tokio::task::spawn_blocking for example, the compiler requires Send and Sync to be implemented for ScanningKeyOps. Otherwise, I get this error:

`(dyn ScanningKeyOps<sapling_crypto::note_encryption::SaplingDomain, AccountId, sapling_crypto::note::nullifier::Nullifier> + 'static)` cannot be sent between threads safely

I tried bounding AccountId by Send and Sync in ScanningKeyOps, but that didn't help.

Bounding ScanningKeyOps itself eliminates the error, but once I do that, I have to make the rest of the changes in this PR to fix subsequent bound requirements.

Copy link
Contributor Author

@upbqdn upbqdn Sep 24, 2024

Choose a reason for hiding this comment

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

This PR is not important though, feel free to close it without merging if it's of no use. The workaround was to create an instance of ScanningKeys in the scanning thread itself. Moreover, we have no plans to work on the scanner any time soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants