-
Notifications
You must be signed in to change notification settings - Fork 249
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
Refactor Sapling builder to separate out proof generation #1023
Conversation
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.
Flushing first-pass comments. I will need to make another pass to better verify how the new behavior compares to the old.
@@ -494,7 +495,7 @@ pub(crate) mod tests { | |||
zcash_primitives::transaction::components::{OutPoint, TxOut}, | |||
}; | |||
|
|||
pub(crate) fn test_prover() -> impl TxProver { | |||
pub(crate) fn test_prover() -> impl SpendProver + OutputProver { | |||
match LocalTxProver::with_default_location() { |
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.
Note: #1017
type Output = TrapdoorSum; | ||
|
||
fn sub(self, rhs: Self) -> Self::Output { | ||
TrapdoorSum(self.0 - rhs.0) |
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.
Based upon the other uses of TrapdoorSum
it doesn't look like there's any potential for underflow, so this seems okay. One question: when we implement arithmetic operations on a type, it seems like we often do so in kind of a piecemeal fashion. Is there a systemic approach we should take to implementing arithmetic operations? Ideally it seems like we should be guided by abstract algebra - it looks like TrapdoorSum
might be an abelian group? What's the algebraic relationship between TrapdoorSum
and ValueCommitTrapdoor
?
None of this is blocking; it's probably fine for this to be done ad-hoc, but just something I'd like us to think about, given that I've been considering similar issues for ZEC amounts.
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.
Indeed these Sapling types have been somewhat ad-hoc constructed, and we should do a full pass over both these Sapling types (some of which pre-date the orchard
crate, and others were partly duplicated from orchard::value
after we figured out a nice set of structs there), as well as the Orchard types (e.g. TrapdoorSum
here is not present in orchard
, but maybe should be).
What's the algebraic relationship between
TrapdoorSum
andValueCommitTrapdoor
?
A ValueCommitTrapdoor
is a specific rcv
value for a specific Sapling Spend or Output, corresponding to its cv
value in the transaction.
A TrapdoorSum
is the sum or difference of one or more ValueCommitTrapdoor
s, corresponding to the sum or difference of one or more Sapling Spends or Outputs. As such, it doesn't necessarily correspond to any particular cv
value, but instead is a valid bsk
value for that set of Spends and Outputs.
In the orchard
crate we have ValueSum
(a signed 64-bit integer) to distinguish from NoteValue
(an unsigned 64-bit integer) or ValueBalance
(a signed 63-bit integer). TrapdoorSum
fills a similar spot in the type graph for bsk
, and IIRC arose here from how we previously did value balancing in the SaplingProvingContext
and SaplingVerificationContext
types (which become obsolete after this PR, as their job is now done inside the Sapling builder). Currently in the orchard
crate we just allow summing ValueCommitTrapdoor
s directly into the same type, but we could choose to introduce this same distinction (or alternatively remove TrapdoorSum
).
@@ -439,23 +452,25 @@ impl<'a, P: consensus::Parameters, R: RngCore + CryptoRng> Builder<'a, P, R> { | |||
/// | |||
/// Upon success, returns a tuple containing the final transaction, and the | |||
/// [`SaplingMetadata`] generated during the build process. | |||
pub fn build<FR: FeeRule>( | |||
pub fn build<SP: SpendProver, OP: OutputProver, FR: FeeRule>( |
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: I feel like these should be qualified, and the corresponding variables named, to reflect their being specific to Sapling, given that this API is part of the top-level builder. Also, if the objective is to align the Sapling and Orchard designs, why does this need the Sapling prover(s) passed in but not an Orchard prover? Is this PR just an intermediate step?
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 feel like these should be qualified, and the corresponding variables named, to reflect their being specific to Sapling, given that this API is part of the top-level builder.
[...]
Is this PR just an intermediate step?
The type names are qualified, with the sapling::prover
module prefix. And when these eventually move into the sapling-crypto
crate, we wouldn't want to include redundant prefixes on the type or variable names. At the top-level transaction builder however (that is staying in zcash_primitives
) I'd be fine with a prefix.
Also note that the presence of these traits is an artifact of how the Sapling transaction builder was originally constructed to allow remote proving (in environments where local proving could be too costly), as well as different types to represent loading the parameters in different ways. Orchard doesn't have these intermediate traits; it instead has concrete types for its prover. But keeping with the trait-based approach kept this (and the prior PRs) simpler to implement and review.
So in this regard I do consider this PR to be an intermediate step: the TxBuilder
trait is unused after this PR and can be removed, and similarly we could remove the ability to do remote proving (which AFAIK has never been done beyond a demo I put together years ago) and simplify the types to be more concrete like Orchard.
Also, if the objective is to align the Sapling and Orchard designs, why does this need the Sapling prover(s) passed in but not an Orchard prover?
For the current transaction builder implementation, the Orchard proving parameters are being constructed on-the-fly. That adds overhead to the transaction building process that we won't want in wallets, but we haven't noticed it yet because we don't have Orchard support in zcash_client_backend
. So yes, eventually we will want to pass in the Orchard proving parameters and let the caller decide when to build them.
// We need to create proofs before signatures, because we still support | ||
// creating V4 transactions, which commit to the Sapling proofs in the | ||
// transaction digest. |
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 think this answers my question about why we need to provide the Sapling prover but not the Orchard one. Should we consider deprecating V4 transaction creation as a preparatory step here?
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.
If you look a few lines lower, you'll see:
b.create_proof(&orchard::circuit::ProvingKey::build(), &mut rng)
which is where the Orchard parameters are being constructed on-the-fly like I mentioned above.
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.
Separately, yes I think we should at some point deprecate creating v4 transactions to remove this hard dependency (and allow us to move proof creation down next to signature creation in the top-level builder, and eventually refactor the top-level builder to be like the Sapling and Orchard builders). But that will require additional refactoring work elsewhere, because currently the type system forces us to always build proofs before constructing TransactionData
, so that we are able to derive the txid no matter what version is used. We will likely need to introduce a type-level distinction between pre-v5 and v5+ transactions to address this (instead of always resolving versions at runtime like we currently do).
Codecov ReportAttention:
... and 3 files with indirect coverage changes 📢 Thoughts on this report? Let us know!. |
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 with minor suggestions & clippy fixes.
c4d9de2
to
082b70a
Compare
Rebased on |
082b70a
to
d43cc77
Compare
Force-pushed to address review comments. |
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.
re-utACK d43cc77
d43cc77
to
0989147
Compare
Force-pushed to fix clippy lint. This PR doesn't have changelog entries, but there are other changes that need to be made to the changelog that overlap / conflict with that this PR needs, so I'll make them in a separate PR. |
0989147
to
927d32b
Compare
Force-pushed to fix copy-pasta bugs that resulted in broken intra-doc links. |
927d32b
to
b2ff29d
Compare
Rebased on |
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 b2ff29d
Closes #741.