-
Notifications
You must be signed in to change notification settings - Fork 250
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: Introduce an "orchard" feature flag. #1080
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1080 +/- ##
==========================================
+ Coverage 66.41% 66.48% +0.07%
==========================================
Files 113 113
Lines 10821 10856 +35
==========================================
+ Hits 7187 7218 +31
- Misses 3634 3638 +4 ☔ View full report in Codecov by Sentry. |
ce44169
to
fd14b00
Compare
97f31e2
to
c272c0c
Compare
We plan to also introduce a similar flag to gate access to Sapling functionality. Since introduction of Orchard functionality is still nascent, it's the correct time to introduce this isolation, before there's more functionality that needs to be isolated in this fashion.
c272c0c
to
f18d082
Compare
…commitment and nullifier data.
5c155ce
to
857e44a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed as of 857e44a.
#[cfg(feature = "orchard")] | ||
let has_orchard = orchard.is_some(); | ||
#[cfg(not(feature = "orchard"))] | ||
let has_orchard = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: this is fine for now, but when sapling
is given a feature flag, we will need to ensure that we can never produce a USK if both sapling
and orchard
are disabled (probably by gating the entire UnifiedSpendingKey
type on #[cfg(any(feature = "orchard", feature = "sapling"))]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After having done this exercise, I think that we should go ahead with adding the sapling
feature flag to zcash_client_backend
in the near future; it's not that much work now that the pattern is established and it really does help tease out coupling and improve modularity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given transparent support is also under a feature flag, what will happen when no feature flag is specified? Do we expect that to build, or would the error be helpful in directing the consumer to specify at least one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can just have that cause a compilation error.
@@ -80,6 +80,10 @@ multicore = ["maybe-rayon/threads", "zcash_primitives/multicore"] | |||
## client is configured for use with the Zcash testnet. | |||
mainnet = [] | |||
|
|||
## Enables support for storing data related to the sending and receiving of | |||
## Orchard funds. | |||
orchard = ["zcash_client_backend/orchard"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should orchard
be default-enabled (both here and in zcash_client_backend
)? I think that's likely what we want as the default here eventually (and for compatibility it will be the default for the eventual sapling
feature flag), and then downstreams that don't want Orchard can use default-features = false
(like is done to disable multicore
). But maybe while developing it we leave it default-disabled? IDK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, ultimately, but I want it to be default-disabled until there are no more FIXME
or unimplemented()
sections.
@@ -841,6 +841,8 @@ pub(crate) fn fake_compact_block_from_tx( | |||
ctx.outputs.push(output.into()); | |||
} | |||
} | |||
|
|||
#[cfg(feature = "orchard")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forget: does the client have a way to tell lightwalletd
to only send one or the other of Sapling and Orchard? Or will real CompactBlock
s always contain both Sapling and Orchard components of transactions even if the local client only supports one of them? If the latter, we should probably have some way to test that case (but maybe not here, given that this is predicated on transaction parsing support for Orchard types).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now I think they'll always contain both.
// We test the full roundtrip only with the `orchard` feature enabled, because we will | ||
// not generate the `orchard` part of the encoding if the UFVK does not have an Orchard | ||
// part. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking: we should instead update these test vectors to cover the matrix of flags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created #1084
#[cfg(feature = "orchard")] | ||
if _params |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per my comment above, if we're still going to have the database tables for Orchard in the schema, then for values that don't depend on orchard
crate types and aren't wallet-specific (like the Orchard tree size for a given block), we probably don't want to feature flag this off.
The main reason for doing so would be if the presence of an Orchard tree size is used as some kind of indicator or switch for later logic. Do we need to use it as such?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to use the presence or absence of data to be used as a logic switch, for sure. However, as I stated above, I think that we should try to fully segregate protocol-specific functionality so that as much orchard
stuff as possible is removed from the API when the orchard
flag is disabled; a smaller API surface is going to be less error-prone.
sapling_anchor: Option<sapling::Anchor>, | ||
orchard_anchor: Option<orchard::Anchor>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Asking so I can keep track of the design:) Why are the anchors now optional again? (And why the change in this PR - what about the orchard
flag necessitates this?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, the anchors are optional so that we don't have to depend on the orchard
crate in zcash_client_backend
, since the feature flagging doesn't yet extend to zcash_primitives
. We will likely want to do that work in zcash_primitives
, but that's a bigger job that I didn't want to tackle yet. So making these optional is the compromise.
@@ -522,20 +528,22 @@ impl<'a, P: consensus::Parameters, U: sapling::builder::ProverProgress> Builder< | |||
transparent_inputs, | |||
self.transparent_builder.outputs(), | |||
sapling_spends, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking: I think we need to alter this to call bundle_type.num_spends()
after the changes in zcash/sapling-crypto#114. This should happen in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is happening as part of #1060
Co-authored-by: str4d <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 967fd36
let ua = UnifiedAddress::from_receivers(orchard, sapling, transparent).unwrap(); | ||
|
||
#[cfg(not(feature = "orchard"))] | ||
let ua = UnifiedAddress::from_receivers(sapling, transparent).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could just be
let ua = UnifiedAddress::from_receivers(
#[cfg(feature = "orchard")] orchard,
sapling,
transparent,
).unwrap();
(which will be simpler once we add the "sapling" feature).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Post-hoc ACK
We plan to also introduce a similar flag to gate access to Sapling functionality. Since introduction of Orchard functionality is still nascent, it's the correct time to introduce this isolation, before there's more functionality that needs to be isolated in this fashion.