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

deps(ecc): Update zcash_primitives and orchard only for zebra-chain crate #8522

Closed
wants to merge 8 commits into from
Closed

deps(ecc): Update zcash_primitives and orchard only for zebra-chain crate #8522

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4ec2607
upgrade zcash_primitives and orchard only for zebra-chain crate
oxarbitrage May 13, 2024
617fc03
update and use TryFrom for amounts
oxarbitrage May 17, 2024
1da71d8
fix imports in serialize
oxarbitrage May 17, 2024
66894df
leave doc as it was
oxarbitrage May 17, 2024
ba7fc7b
leave doc as it was 2
oxarbitrage May 17, 2024
bca2c28
use `try_into` for amount
oxarbitrage May 18, 2024
3e4aa04
use `zip_212_enforcement`
oxarbitrage May 18, 2024
71dc000
Merge remote-tracking branch 'origin/main' into upgrade-primitives-or…
oxarbitrage May 18, 2024
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
8 changes: 3 additions & 5 deletions Cargo.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5825,7 +5825,6 @@ dependencies = [
"ff",
"fpe",
"group",
"hdwallet",
"hex",
"incrementalmerkletree",
"jubjub",
Expand All @@ -5835,8 +5834,6 @@ dependencies = [
"orchard 0.6.0",
"rand 0.8.5",
"rand_core 0.6.4",
"ripemd",
"secp256k1",
"sha2",
"subtle",
"zcash_address",
Expand Down Expand Up @@ -6012,7 +6009,7 @@ dependencies = [
"jubjub",
"lazy_static",
"num-integer",
"orchard 0.6.0",
"orchard 0.7.1",
"primitive-types",
"proptest",
"proptest-derive",
Expand All @@ -6023,6 +6020,7 @@ dependencies = [
"reddsa",
"redjubjub",
"ripemd",
"sapling-crypto",
"secp256k1",
"serde",
"serde-big-array",
Expand All @@ -6042,7 +6040,7 @@ dependencies = [
"zcash_encoding",
"zcash_history",
"zcash_note_encryption",
"zcash_primitives 0.13.0",
"zcash_primitives 0.14.0",
"zcash_protocol",
"zebra-test",
]
Expand Down
5 changes: 3 additions & 2 deletions zebra-chain/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,11 +93,12 @@ x25519-dalek = { version = "2.0.1", features = ["serde"] }

# ECC deps
halo2 = { package = "halo2_proofs", version = "0.3.0" }
orchard = "0.6.0"
orchard = "0.7.0"
zcash_encoding = "0.2.0"
zcash_history = "0.4.0"
zcash_note_encryption = "0.4.0"
zcash_primitives = { version = "0.13.0", features = ["transparent-inputs"] }
zcash_primitives = { version = "0.14.0", features = ["transparent-inputs"] }
sapling = { package = "sapling-crypto", version = "0.1" }
zcash_protocol = { version = "0.1.1" }
zcash_address = { version = "0.3.2" }

Expand Down
1 change: 0 additions & 1 deletion zebra-chain/src/primitives/address.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
//! Usage: <https://docs.rs/zcash_address/0.2.0/zcash_address/trait.TryFromAddress.html#examples>

use zcash_address::unified::{self, Container};
use zcash_primitives::sapling;

use crate::{parameters::NetworkKind, transparent, BoxError};

Expand Down
10 changes: 5 additions & 5 deletions zebra-chain/src/primitives/viewing_key/sapling.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
//! Defines types and implements methods for parsing Sapling viewing keys and converting them to `zebra-chain` types

use zcash_client_backend::encoding::decode_extended_full_viewing_key;
use zcash_primitives::{
constants::*,
sapling::keys::{FullViewingKey as SaplingFvk, SaplingIvk},
zip32::DiversifiableFullViewingKey as SaplingDfvk,
use sapling::keys::{FullViewingKey as SaplingFvk, SaplingIvk};
use zcash_client_backend::{
encoding::decode_extended_full_viewing_key,
keys::sapling::DiversifiableFullViewingKey as SaplingDfvk,
};
use zcash_primitives::constants::*;

use crate::parameters::Network;

Expand Down
8 changes: 3 additions & 5 deletions zebra-chain/src/primitives/zcash_note_encryption.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,14 @@ pub fn decrypts_successfully(transaction: &Transaction, network: &Network, heigh
let alt_tx = convert_tx_to_librustzcash(transaction, network_upgrade)
.expect("zcash_primitives and Zebra transaction formats must be compatible");

let alt_height = height.0.into();
let null_sapling_ovk = zcash_primitives::keys::OutgoingViewingKey([0u8; 32]);
let null_sapling_ovk = sapling::keys::OutgoingViewingKey([0u8; 32]);

if let Some(bundle) = alt_tx.sapling_bundle() {
for output in bundle.shielded_outputs().iter() {
let recovery = zcash_primitives::sapling::note_encryption::try_sapling_output_recovery(
network,
alt_height,
let recovery = sapling::note_encryption::try_sapling_output_recovery(
&null_sapling_ovk,
output,
sapling::note_encryption::Zip212Enforcement::GracePeriod,
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 was because the coinbase_outputs_are_decryptable_for_historical_blocks test in zebra-consensus pass only if this variable isin the grace period, however i think we need to compare with the height of the output to decide what goes here ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh good, they've changed this already, I thought this was in 0.15.0.

ZIP-212 enforcement can always be on here because we only call this function after Canopy activation. We should leave a comment about needing to check the height if Zebra's mandatory checkpoint height ever falls below the last block before Canopy activation (or we could just double check the height here anyways).

Copy link
Contributor Author

@oxarbitrage oxarbitrage May 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If we change to sapling::note_encryption::Zip212Enforcement::On then the test will fail with:

Message:  coinbase outputs must be decryptable with an all-zero key: CoinbaseOutputsNotDecryptable
Location: zebra-consensus/src/transaction/tests.rs:2796

We might need to fix the test.

Copy link
Contributor

@arya2 arya2 May 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

So the test will pass, and so we don't have to update this code if Zebra's mandatory checkpoint height is ever lowered below the last block before Canopy activation, we could derive Ord/PartialOrd impls on NetworkUpgrade and do:

    let zip_212_enforcement = if network_upgrade >= NetworkUpgrade::Canopy {
        sapling::note_encryption::Zip212Enforcement::On
    } else {
        sapling::note_encryption::Zip212Enforcement::Off
    };

    if let Some(bundle) = alt_tx.sapling_bundle() {
        for output in bundle.shielded_outputs().iter() {
            let recovery = sapling::note_encryption::try_sapling_output_recovery(
                &null_sapling_ovk,
                output,
                zip_212_enforcement,
            );
            if recovery.is_none() {
                return false;
            }
        }
    }

It could also just get the Canopy activation height and check that it's past it.

Zebra skips the check in production because of the mandatory checkpoint height but does check historical blocks before Canopy in the test, so the test actually covers more than strictly necessary here.

This is the relevant section of the ZIP: https://zips.z.cash/zip-0212#consensus-rule-change-for-coinbase-transactions

Note: This seems to be wrong in zcashd, see zcash/zcash#6890. It may be a good idea to do a sync test from Canopy activation until the end of the grace period with checkpoint sync disabled to make sure that all of the blocks in the grace period follow the ZIP, though I think it was implemented correctly in zcashd prior to v5.9.0.

Update: Actually, there's no point in a sync test, the mandatory checkpoint height is still after the grace period. We can do a sync test with checkpoint sync disabled when we're lowering the mandatory checkpoint height.

);
if recovery.is_none() {
return false;
Expand Down
48 changes: 20 additions & 28 deletions zebra-chain/src/primitives/zcash_primitives.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,11 @@ impl zp_tx::components::transparent::Authorization for TransparentAuth<'_> {
// In this block we convert our Output to a librustzcash to TxOut.
// (We could do the serialize/deserialize route but it's simple enough to convert manually)
impl zp_tx::sighash::TransparentAuthorizingContext for TransparentAuth<'_> {
fn input_amounts(&self) -> Vec<zp_tx::components::amount::Amount> {
fn input_amounts(&self) -> Vec<zp_tx::components::amount::NonNegativeAmount> {
self.all_prev_outputs
.iter()
.map(|prevout| {
zp_tx::components::amount::Amount::from_nonnegative_i64_le_bytes(
zp_tx::components::amount::NonNegativeAmount::from_nonnegative_i64_le_bytes(
oxarbitrage marked this conversation as resolved.
Show resolved Hide resolved
prevout.value.to_bytes(),
)
oxarbitrage marked this conversation as resolved.
Show resolved Hide resolved
.expect("will not fail since it was previously validated")
Expand Down Expand Up @@ -83,39 +83,31 @@ impl<'a>

struct IdentityMap;

impl
zp_tx::components::sapling::MapAuth<
zp_tx::components::sapling::Authorized,
zp_tx::components::sapling::Authorized,
> for IdentityMap
impl zp_tx::components::sapling::MapAuth<sapling::bundle::Authorized, sapling::bundle::Authorized>
for IdentityMap
{
fn map_spend_proof(
&self,
p: <zp_tx::components::sapling::Authorized as zp_tx::components::sapling::Authorization>::SpendProof,
) -> <zp_tx::components::sapling::Authorized as zp_tx::components::sapling::Authorization>::SpendProof
{
&mut self,
p: <sapling::bundle::Authorized as sapling::bundle::Authorization>::SpendProof,
) -> <sapling::bundle::Authorized as sapling::bundle::Authorization>::SpendProof {
p
}

fn map_output_proof(
&self,
p: <zp_tx::components::sapling::Authorized as zp_tx::components::sapling::Authorization>::OutputProof,
) -> <zp_tx::components::sapling::Authorized as zp_tx::components::sapling::Authorization>::OutputProof
{
&mut self,
p: <sapling::bundle::Authorized as sapling::bundle::Authorization>::OutputProof,
) -> <sapling::bundle::Authorized as sapling::bundle::Authorization>::OutputProof {
p
}

fn map_auth_sig(
&self,
s: <zp_tx::components::sapling::Authorized as zp_tx::components::sapling::Authorization>::AuthSig,
) -> <zp_tx::components::sapling::Authorized as zp_tx::components::sapling::Authorization>::AuthSig{
&mut self,
s: <sapling::bundle::Authorized as sapling::bundle::Authorization>::AuthSig,
) -> <sapling::bundle::Authorized as sapling::bundle::Authorization>::AuthSig {
s
}

fn map_authorization(
&self,
a: zp_tx::components::sapling::Authorized,
) -> zp_tx::components::sapling::Authorized {
fn map_authorization(&mut self, a: sapling::bundle::Authorized) -> sapling::bundle::Authorized {
a
}
}
Expand All @@ -141,7 +133,7 @@ struct PrecomputedAuth<'a> {

impl<'a> zp_tx::Authorization for PrecomputedAuth<'a> {
type TransparentAuth = TransparentAuth<'a>;
type SaplingAuth = zp_tx::components::sapling::Authorized;
type SaplingAuth = sapling::bundle::Authorized;
type OrchardAuth = orchard::bundle::Authorized;
}

Expand Down Expand Up @@ -213,12 +205,12 @@ impl TryFrom<transparent::Output> for zp_tx::components::TxOut {
}
}

/// Convert a Zebra Amount into a librustzcash one.
impl TryFrom<Amount<NonNegative>> for zp_tx::components::Amount {
/// Convert a Zebra non-negative Amount into a librustzcash one.
impl TryFrom<Amount<NonNegative>> for zp_tx::components::amount::NonNegativeAmount {
type Error = ();

fn try_from(amount: Amount<NonNegative>) -> Result<Self, Self::Error> {
zp_tx::components::Amount::from_u64(amount.into())
zp_tx::components::amount::NonNegativeAmount::from_nonnegative_i64(amount.into())
}
}

Expand Down Expand Up @@ -327,10 +319,10 @@ pub(crate) fn transparent_output_address(
let alt_addr = tx_out.recipient_address();

match alt_addr {
Some(zcash_primitives::legacy::TransparentAddress::PublicKey(pub_key_hash)) => Some(
Some(zcash_primitives::legacy::TransparentAddress::PublicKeyHash(pub_key_hash)) => Some(
transparent::Address::from_pub_key_hash(network.kind(), pub_key_hash),
),
Some(zcash_primitives::legacy::TransparentAddress::Script(script_hash)) => Some(
Some(zcash_primitives::legacy::TransparentAddress::ScriptHash(script_hash)) => Some(
transparent::Address::from_script_hash(network.kind(), script_hash),
),
None => None,
Expand Down
4 changes: 2 additions & 2 deletions zebra-chain/src/transaction/serialize.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use crate::{
};

use super::*;
use sapling::{Output, SharedAnchor, Spend};
use crate::sapling::{Output, SharedAnchor, Spend};

impl ZcashDeserialize for jubjub::Fq {
fn zcash_deserialize<R: io::Read>(mut reader: R) -> Result<Self, SerializationError> {
Expand Down Expand Up @@ -134,7 +134,7 @@ impl ZcashSerialize for sapling::ShieldedData<SharedAnchor> {
let (spend_prefixes, spend_proofs_sigs): (Vec<_>, Vec<_>) = self
.spends()
.cloned()
.map(sapling::Spend::<SharedAnchor>::into_v5_parts)
.map(Spend::<SharedAnchor>::into_v5_parts)
.map(|(prefix, proof, sig)| (prefix, (proof, sig)))
.unzip();
let (spend_proofs, spend_sigs) = spend_proofs_sigs.into_iter().unzip();
Expand Down
Loading