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.

Sign up for GitHub

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

Jump to bottom

Refactor Sapling builder to separate out proof generation #1023

Merged
merged 1 commit into from
Nov 2, 2023
Merged

Refactor Sapling builder to separate out proof generation #1023

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 38 additions & 9 deletions zcash_client_backend/src/data_api/wallet.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use zcash_primitives::{
sapling::{
self,
note_encryption::{try_sapling_note_decryption, PreparedIncomingViewingKey},
prover::TxProver as SaplingProver,
prover::{OutputProver, SpendProver},
Node,
},
transaction::{
Expand Down Expand Up @@ -191,7 +191,8 @@ where
pub fn create_spend_to_address<DbT, ParamsT>(
wallet_db: &mut DbT,
params: &ParamsT,
prover: impl SaplingProver,
spend_prover: &impl SpendProver,
output_prover: &impl OutputProver,
usk: &UnifiedSpendingKey,
to: &RecipientAddress,
amount: NonNegativeAmount,
Expand Down Expand Up @@ -231,7 +232,15 @@ where
change_memo,
)?;

create_proposed_transaction(wallet_db, params, prover, usk, ovk_policy, &proposal)
create_proposed_transaction(
wallet_db,
params,
spend_prover,
output_prover,
usk,
ovk_policy,
&proposal,
)
}

/// Constructs a transaction that sends funds as specified by the `request` argument
Expand Down Expand Up @@ -289,7 +298,8 @@ where
pub fn spend<DbT, ParamsT, InputsT>(
wallet_db: &mut DbT,
params: &ParamsT,
prover: impl SaplingProver,
spend_prover: &impl SpendProver,
output_prover: &impl OutputProver,
input_selector: &InputsT,
usk: &UnifiedSpendingKey,
request: zip321::TransactionRequest,
Expand Down Expand Up @@ -324,7 +334,15 @@ where
min_confirmations,
)?;

create_proposed_transaction(wallet_db, params, prover, usk, ovk_policy, &proposal)
create_proposed_transaction(
wallet_db,
params,
spend_prover,
output_prover,
usk,
ovk_policy,
&proposal,
)
}

/// Select transaction inputs, compute fees, and construct a proposal for a transaction
Expand Down Expand Up @@ -475,7 +493,8 @@ where
pub fn create_proposed_transaction<DbT, ParamsT, InputsErrT, FeeRuleT>(
wallet_db: &mut DbT,
params: &ParamsT,
prover: impl SaplingProver,
spend_prover: &impl SpendProver,
output_prover: &impl OutputProver,
usk: &UnifiedSpendingKey,
ovk_policy: OvkPolicy,
proposal: &Proposal<FeeRuleT, DbT::NoteRef>,
Expand Down Expand Up @@ -663,7 +682,8 @@ where
}

// Build the transaction with the specified fee rule
let (tx, sapling_build_meta) = builder.build(&prover, proposal.fee_rule())?;
let (tx, sapling_build_meta) =
builder.build(spend_prover, output_prover, proposal.fee_rule())?;

let internal_ivk = PreparedIncomingViewingKey::new(&dfvk.to_ivk(Scope::Internal));
let sapling_outputs =
Expand Down Expand Up @@ -768,7 +788,8 @@ where
pub fn shield_transparent_funds<DbT, ParamsT, InputsT>(
wallet_db: &mut DbT,
params: &ParamsT,
prover: impl SaplingProver,
spend_prover: &impl SpendProver,
output_prover: &impl OutputProver,
input_selector: &InputsT,
shielding_threshold: NonNegativeAmount,
usk: &UnifiedSpendingKey,
Expand Down Expand Up @@ -798,7 +819,15 @@ where
min_confirmations,
)?;

create_proposed_transaction(wallet_db, params, prover, usk, OvkPolicy::Sender, &proposal)
create_proposed_transaction(
wallet_db,
params,
spend_prover,
output_prover,
usk,
OvkPolicy::Sender,
&proposal,
)
}

#[allow(clippy::type_complexity)]
Expand Down
16 changes: 12 additions & 4 deletions zcash_client_sqlite/src/testing.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,10 +448,12 @@ impl<Cache> TestState<Cache> {
>,
> {
let params = self.network();
let prover = test_prover();
create_spend_to_address(
&mut self.db_data,
&params,
test_prover(),
&prover,
&prover,
usk,
to,
amount,
Expand Down Expand Up @@ -484,10 +486,12 @@ impl<Cache> TestState<Cache> {
InputsT: InputSelector<DataSource = WalletDb<Connection, Network>>,
{
let params = self.network();
let prover = test_prover();
spend(
&mut self.db_data,
&params,
test_prover(),
&prover,
&prover,
input_selector,
usk,
request,
Expand Down Expand Up @@ -614,10 +618,12 @@ impl<Cache> TestState<Cache> {
FeeRuleT: FeeRule,
{
let params = self.network();
let prover = test_prover();
create_proposed_transaction(
&mut self.db_data,
&params,
test_prover(),
&prover,
&prover,
usk,
ovk_policy,
proposal,
Expand Down Expand Up @@ -647,10 +653,12 @@ impl<Cache> TestState<Cache> {
InputsT: InputSelector<DataSource = WalletDb<Connection, Network>>,
{
let params = self.network();
let prover = test_prover();
shield_transparent_funds(
&mut self.db_data,
&params,
test_prover(),
&prover,
&prover,
input_selector,
shielding_threshold,
usk,
Expand Down
7 changes: 4 additions & 3 deletions zcash_client_sqlite/src/wallet/sapling.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -451,8 +451,9 @@ pub(crate) mod tests {
legacy::TransparentAddress,
memo::{Memo, MemoBytes},
sapling::{
note_encryption::try_sapling_output_recovery, prover::TxProver, Node, Note,
PaymentAddress,
note_encryption::try_sapling_output_recovery,
prover::{OutputProver, SpendProver},
Node, Note, PaymentAddress,
},
transaction::{
components::{amount::NonNegativeAmount, Amount},
Expand Down Expand Up @@ -497,7 +498,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() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: #1017

Some(tx_prover) => tx_prover,
None => {
Expand Down
6 changes: 3 additions & 3 deletions zcash_extensions/src/transparent/demo.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -839,7 +839,7 @@ mod tests {
.unwrap();
let (tx_a, _) = builder_a
.txn_builder
.build_zfuture(&prover, &fee_rule)
.build_zfuture(&prover, &prover, &fee_rule)
.map_err(|e| format!("build failure: {:?}", e))
.unwrap();
let tze_a = tx_a.tze_bundle().unwrap();
Expand All @@ -857,7 +857,7 @@ mod tests {
.unwrap();
let (tx_b, _) = builder_b
.txn_builder
.build_zfuture(&prover, &fee_rule)
.build_zfuture(&prover, &prover, &fee_rule)
.map_err(|e| format!("build failure: {:?}", e))
.unwrap();
let tze_b = tx_b.tze_bundle().unwrap();
Expand All @@ -882,7 +882,7 @@ mod tests {

let (tx_c, _) = builder_c
.txn_builder
.build_zfuture(&prover, &fee_rule)
.build_zfuture(&prover, &prover, &fee_rule)
.map_err(|e| format!("build failure: {:?}", e))
.unwrap();
let tze_c = tx_c.tze_bundle().unwrap();
Expand Down
12 changes: 5 additions & 7 deletions zcash_primitives/benches/note_decryption.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,11 @@ use zcash_primitives::{
try_sapling_compact_note_decryption, try_sapling_note_decryption,
PreparedIncomingViewingKey, SaplingDomain,
},
prover::mock::MockTxProver,
prover::mock::{MockOutputProver, MockSpendProver},
value::NoteValue,
Diversifier, SaplingIvk,
},
transaction::components::sapling::{
builder::SaplingBuilder, CompactOutputDescription, GrothProofBytes, OutputDescription,
},
transaction::components::sapling::{builder::SaplingBuilder, CompactOutputDescription},
};

#[cfg(unix)]
Expand All @@ -32,7 +30,7 @@ fn bench_note_decryption(c: &mut Criterion) {
let invalid_ivk = SaplingIvk(jubjub::Fr::random(&mut rng));

// Construct a Sapling output.
let output: OutputDescription<GrothProofBytes> = {
let output = {
let diversifier = Diversifier([0; 11]);
let pa = valid_ivk.to_payment_address(diversifier).unwrap();

Expand All @@ -46,8 +44,8 @@ fn bench_note_decryption(c: &mut Criterion) {
MemoBytes::empty(),
)
.unwrap();
let bundle = builder
.build(&MockTxProver, &mut (), &mut rng, height, None)
let (bundle, _) = builder
.build::<MockSpendProver, MockOutputProver, _>(&mut rng, height)
.unwrap()
.unwrap();
bundle.shielded_outputs()[0].clone()
Expand Down
27 changes: 25 additions & 2 deletions zcash_primitives/src/sapling.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,11 @@
sighash: &[u8; 32],
rng: &mut R,
) -> Signature {
spend_sig_internal(ask, ar, sighash, rng)
spend_sig_internal(&ask, ar, sighash, rng)

Check warning on line 39 in zcash_primitives/src/sapling.rs

View check run for this annotation

Codecov / codecov/patch

zcash_primitives/src/sapling.rs#L39 

Added line #L39 was not covered by tests
}

pub(crate) fn spend_sig_internal<R: RngCore>(
ask: PrivateKey,
ask: &PrivateKey,
ar: jubjub::Fr,
sighash: &[u8; 32],
rng: &mut R,
Expand All @@ -60,6 +60,29 @@
rsk.sign(&data_to_be_signed, rng, SPENDING_KEY_GENERATOR)
}

/// Verifies a spendAuthSig.
///
/// This only exists because the RedJubjub implementation inside `zcash_primitives` does
/// not implement key prefixing (which was added in response to a Sapling audit). This
/// will be fixed by migrating to the redjubjub crate.
pub(crate) fn verify_spend_sig(

Check warning on line 68 in zcash_primitives/src/sapling.rs

View check run for this annotation

Codecov / codecov/patch

zcash_primitives/src/sapling.rs#L68 

Added line #L68 was not covered by tests
ak: &PublicKey,
alpha: jubjub::Fr,
sighash: &[u8; 32],
sig: &Signature,
) -> bool {
// We compute `rk` (needed for key prefixing)
let rk = ak.randomize(alpha, SPENDING_KEY_GENERATOR);

Check warning on line 75 in zcash_primitives/src/sapling.rs

View check run for this annotation

Codecov / codecov/patch

zcash_primitives/src/sapling.rs#L75 

Added line #L75 was not covered by tests

// Compute the signature's message for rk/spend_auth_sig
let mut data_to_be_signed = [0u8; 64];
data_to_be_signed[0..32].copy_from_slice(&rk.0.to_bytes());
data_to_be_signed[32..64].copy_from_slice(&sighash[..]);

Check warning on line 80 in zcash_primitives/src/sapling.rs

View check run for this annotation

Codecov / codecov/patch

zcash_primitives/src/sapling.rs#L78-L80 

Added lines #L78 - L80 were not covered by tests

// Do the verifying
rk.verify(&data_to_be_signed, sig, SPENDING_KEY_GENERATOR)

Check warning on line 83 in zcash_primitives/src/sapling.rs

View check run for this annotation

Codecov / codecov/patch

zcash_primitives/src/sapling.rs#L83 

Added line #L83 was not covered by tests
}

#[cfg(any(test, feature = "test-dependencies"))]
pub mod testing {
pub use super::{
Expand Down
17 changes: 16 additions & 1 deletion zcash_primitives/src/sapling/value.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
//! [Rust documentation]: https://doc.rust-lang.org/stable/std/primitive.i64.html

use bitvec::{array::BitArray, order::Lsb0};
use ff::Field;
use ff::{Field, PrimeField};
use group::GroupEncoding;
use rand::RngCore;
use subtle::CtOption;
Expand Down Expand Up @@ -86,6 +86,21 @@
ValueCommitTrapdoor(jubjub::Scalar::random(rng))
}

/// Constructs `ValueCommitTrapdoor` from the byte representation of a scalar.
///
/// Returns a `None` [`CtOption`] if `bytes` is not a canonical representation of a
/// Jubjub scalar.
///
/// This is a low-level API, requiring a detailed understanding of the
/// [use of value commitment trapdoors][saplingbalance] in the Zcash protocol
/// to use correctly and securely. It is intended to be used in combination
/// with [`ValueCommitment::derive`].
///
/// [saplingbalance]: https://zips.z.cash/protocol/protocol.pdf#saplingbalance
pub fn from_bytes(bytes: [u8; 32]) -> CtOption<Self> {
jubjub::Scalar::from_repr(bytes).map(ValueCommitTrapdoor)

Check warning on line 101 in zcash_primitives/src/sapling/value.rs

View check run for this annotation

Codecov / codecov/patch

zcash_primitives/src/sapling/value.rs#L100-L101 

Added lines #L100 - L101 were not covered by tests
}

/// Returns the inner Jubjub scalar representing this trapdoor.
///
/// This is public for access by `zcash_proofs`.
Expand Down
16 changes: 16 additions & 0 deletions zcash_primitives/src/sapling/value/sums.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,14 @@ impl Sub<&ValueCommitTrapdoor> for ValueCommitTrapdoor {
}
}

impl Sub<TrapdoorSum> for TrapdoorSum {
type Output = TrapdoorSum;

fn sub(self, rhs: Self) -> Self::Output {
TrapdoorSum(self.0 - rhs.0)
Copy link
Contributor

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.

Copy link
Contributor Author

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 and ValueCommitTrapdoor?

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 ValueCommitTrapdoors, 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 ValueCommitTrapdoors directly into the same type, but we could choose to introduce this same distinction (or alternatively remove TrapdoorSum).

}
}

impl SubAssign<&ValueCommitTrapdoor> for TrapdoorSum {
fn sub_assign(&mut self, rhs: &ValueCommitTrapdoor) {
self.0 -= rhs.0;
Expand Down Expand Up @@ -207,6 +215,14 @@ impl SubAssign<&ValueCommitment> for CommitmentSum {
}
}

impl Sub<CommitmentSum> for CommitmentSum {
type Output = CommitmentSum;

fn sub(self, rhs: Self) -> Self::Output {
CommitmentSum(self.0 - rhs.0)
}
}

impl Sum<ValueCommitment> for CommitmentSum {
fn sum<I: Iterator<Item = ValueCommitment>>(iter: I) -> Self {
iter.fold(CommitmentSum::zero(), |acc, cv| acc + &cv)
Expand Down
Loading
Loading