Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
23 changes: 20 additions & 3 deletions zebra-chain/src/primitives/zcash_primitives.rs
Original file line number Diff line number Diff line change
Expand Up @@ -180,13 +180,30 @@ impl TryFrom<&Transaction> for zp_tx::Transaction {
Transaction::V1 { .. }
| Transaction::V2 { .. }
| Transaction::V3 { .. }
| Transaction::V4 { .. } => panic!("Zebra only uses librustzcash for V5 transactions"),
| Transaction::V4 { .. } => {
#[cfg(not(feature = "tx-v6"))]
panic!("Zebra only uses librustzcash for V5 transactions");

#[cfg(feature = "tx-v6")]
panic!("Zebra only uses librustzcash for V5/V6 transactions");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I don't see the advantage of this over the original no flag option.
Prefer simplicity when it is only about the error msg.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done.

}
};

convert_tx_to_librustzcash(
#[cfg(not(feature = "tx-v6"))]
let result = convert_tx_to_librustzcash(
trans,
network_upgrade.branch_id().expect("V5 txs have branch IDs"),
)
);

#[cfg(feature = "tx-v6")]
let result = convert_tx_to_librustzcash(
trans,
network_upgrade
.branch_id()
.expect("V5/V6 txs have branch IDs"),
);

result

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

let's just go with .expect("V5/V6 txs have branch IDs"), for both to avoid the flag compexity.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done.

}
}

Expand Down
137 changes: 123 additions & 14 deletions zebra-chain/src/transaction/tests/vectors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,18 @@ lazy_static! {
sapling_shielded_data: None,
orchard_shielded_data: None,
};

#[cfg(feature = "tx-v6")]
pub static ref EMPTY_V6_TX: Transaction = Transaction::V6 {
network_upgrade: NetworkUpgrade::Nu7,
lock_time: LockTime::min_lock_time_timestamp(),
expiry_height: block::Height(0),
inputs: Vec::new(),
outputs: Vec::new(),
sapling_shielded_data: None,
orchard_shielded_data: None,
orchard_zsa_issue_data: None
};
}

/// Build a mock output list for pre-V5 transactions, with (index+1)
Expand Down Expand Up @@ -257,18 +269,9 @@ fn deserialize_large_transaction() {
.expect_err("transaction should not deserialize due to its size");
}

// Transaction V5 test vectors

/// An empty transaction v5, with no Orchard, Sapling, or Transparent data
///
/// empty transaction are invalid, but Zebra only checks this rule in
/// zebra_consensus::transaction::Verifier
#[test]
fn empty_v5_round_trip() {
fn tx_round_trip(tx: &Transaction) {
let _init_guard = zebra_test::init();

let tx: &Transaction = &EMPTY_V5_TX;

let data = tx.zcash_serialize_to_vec().expect("tx should serialize");
let tx2: &Transaction = &data
.zcash_deserialize_into()
Expand All @@ -283,6 +286,25 @@ fn empty_v5_round_trip() {
assert_eq!(data, data2, "data must be equal if structs are equal");
}

/// An empty transaction v5, with no Orchard, Sapling, or Transparent data
///
/// empty transaction are invalid, but Zebra only checks this rule in
/// zebra_consensus::transaction::Verifier
#[test]
fn empty_v5_round_trip() {
tx_round_trip(&EMPTY_V5_TX)
}

#[cfg(feature = "tx-v6")]
/// An empty transaction v6, with no Orchard/OrchardZSA, Sapling, or Transparent data
///
/// empty transaction are invalid, but Zebra only checks this rule in
/// zebra_consensus::transaction::Verifier
#[test]
fn empty_v6_round_trip() {
tx_round_trip(&EMPTY_V6_TX)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

the order should be

fn empty_v4_round_trip() {
   ..
}
..
fn empty_v5_round_trip() {
    ..
}
..
fn empty_v6_round_trip() {
   ..
}
``

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done.

/// An empty transaction v4, with no Sapling, Sprout, or Transparent data
///
/// empty transaction are invalid, but Zebra only checks this rule in
Expand Down Expand Up @@ -314,18 +336,28 @@ fn empty_v4_round_trip() {
assert_eq!(data, data2, "data must be equal if structs are equal");
}

/// Check if an empty V5 transaction can be deserialized by librustzcash too.
#[test]
fn empty_v5_librustzcash_round_trip() {
fn tx_librustzcash_round_trip(tx: &Transaction) {
let _init_guard = zebra_test::init();

let tx: &Transaction = &EMPTY_V5_TX;
let _alt_tx: zcash_primitives::transaction::Transaction = tx.try_into().expect(
"librustzcash deserialization might work for empty zebra serialized transactions. \
Hint: if empty transactions fail, but other transactions work, delete this test",
);
}

/// Check if an empty V5 transaction can be deserialized by librustzcash too.
#[test]
fn empty_v5_librustzcash_round_trip() {
tx_librustzcash_round_trip(&EMPTY_V5_TX);
}

#[cfg(feature = "tx-v6")]
/// Check if an empty V6 transaction can be deserialized by librustzcash too.
#[test]
fn empty_v6_librustzcash_round_trip() {
tx_librustzcash_round_trip(&EMPTY_V6_TX);
}

/// Do a round-trip test on fake v5 transactions created from v4 transactions
/// in the block test vectors.
///
Expand Down Expand Up @@ -450,6 +482,55 @@ fn fake_v5_round_trip_for_network(network: Network) {
}
}

#[cfg(feature = "tx-v6")]
/// Do a round-trip test on v6 transactions created from the block

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Please mention that this is a serialization round-trip test.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done: change it to:
/// Do a serialization round-trip test on v6 transactions ... .

Also added via librustzcast to the comment for v6_librustzcash_round_trip:
/// Do a round-trip test via librustzcash on v6 transactions ...

/// test vectors.
#[test]
fn v6_round_trip() {
use zebra_test::vectors::ORCHARD_ZSA_WORKFLOW_BLOCKS;

let _init_guard = zebra_test::init();

for block_bytes in ORCHARD_ZSA_WORKFLOW_BLOCKS.iter() {
let block = block_bytes
.zcash_deserialize_into::<Block>()
.expect("block is structurally valid");

// test each transaction
for tx in &block.transactions {
let tx_bytes = tx
.zcash_serialize_to_vec()
.expect("vec serialization is infallible");

let tx2 = tx_bytes
.zcash_deserialize_into::<Transaction>()
.expect("tx is structurally valid");

assert_eq!(tx.as_ref(), &tx2);

let tx_bytes2 = tx2
.zcash_serialize_to_vec()
.expect("vec serialization is infallible");

assert_eq!(
tx_bytes, tx_bytes2,
"data must be equal if structs are equal"
);
}

// test full blocks

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

remove empty line

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done.

let block_bytes2 = block
.zcash_serialize_to_vec()
.expect("vec serialization is infallible");

assert_eq!(
block_bytes, &block_bytes2,
"data must be equal if structs are equal"
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

let's move it right under

let block = block_bytes
            .zcash_deserialize_into::<Block>()

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done.

}
}

#[test]
fn invalid_orchard_nullifier() {
let _init_guard = zebra_test::init();
Expand Down Expand Up @@ -549,6 +630,34 @@ fn fake_v5_librustzcash_round_trip_for_network(network: Network) {
}
}

#[cfg(feature = "tx-v6")]
/// Do a round-trip test on v6 transactions created from the block
/// test vectors.
#[test]
fn v6_librustzcash_round_trip() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I don't see round trip, just a one way trip. Make the trip to be round or adjust the name

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

what this test proves?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done: added serialization after deserialization.

It proves that librustzcash can deserialize transactions serialized by zebra, which is important to test:

let _alt_tx: zcash_primitives::transaction::Transaction = tx
    .as_ref()
    .try_into()
    .expect("librustzcash deserialization must work for zebra serialized transactions");

use zebra_test::vectors::ORCHARD_ZSA_WORKFLOW_BLOCKS;

let _init_guard = zebra_test::init();

for block_bytes in ORCHARD_ZSA_WORKFLOW_BLOCKS.iter() {
let block = block_bytes
.zcash_deserialize_into::<Block>()
.expect("block is structurally valid");

// Test each V6 transaction
for tx in block
.transactions
.iter()
.filter(|tx| matches!(tx.as_ref(), &Transaction::V6 { .. }))
{
let _alt_tx: zcash_primitives::transaction::Transaction = tx
.as_ref()
.try_into()
.expect("librustzcash deserialization must work for zebra serialized transactions");
}
}
}

#[test]
fn zip244_round_trip() -> Result<()> {
let _init_guard = zebra_test::init();
Expand Down
4 changes: 2 additions & 2 deletions zebra-consensus/src/orchard_zsa/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,14 @@ use zebra_chain::{

use zebra_test::{
transcript::{ExpectedTranscriptError, Transcript},
vectors::ORCHARD_WORKFLOW_BLOCKS_ZSA,
vectors::ORCHARD_ZSA_WORKFLOW_BLOCKS,
};

use crate::{block::Request, Config};

fn create_transcript_data() -> impl Iterator<Item = (Request, Result<Hash, ExpectedTranscriptError>)>
{
let workflow_blocks = ORCHARD_WORKFLOW_BLOCKS_ZSA.iter().map(|block_bytes| {
let workflow_blocks = ORCHARD_ZSA_WORKFLOW_BLOCKS.iter().map(|block_bytes| {
Arc::new(Block::zcash_deserialize(&block_bytes[..]).expect("block should deserialize"))
});

Expand Down
4 changes: 2 additions & 2 deletions zebra-consensus/src/primitives/halo2/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ async fn verify_generated_halo2_proofs_vanilla() {
#[cfg(feature = "tx-v6")]
#[tokio::test(flavor = "multi_thread")]
async fn verify_generated_halo2_proofs_zsa() {
verify_generated_halo2_proofs::<OrchardZSA>(&zebra_test::vectors::ORCHARD_SHIELDED_DATA_ZSA)
verify_generated_halo2_proofs::<OrchardZSA>(&zebra_test::vectors::ORCHARD_ZSA_SHIELDED_DATA)
.await
}

Expand Down Expand Up @@ -291,7 +291,7 @@ async fn correctly_err_on_invalid_halo2_proofs_vanilla() {
#[tokio::test(flavor = "multi_thread")]
async fn correctly_err_on_invalid_halo2_proofs_zsa() {
correctly_err_on_invalid_halo2_proofs::<OrchardZSA>(
&zebra_test::vectors::ORCHARD_SHIELDED_DATA_ZSA,
&zebra_test::vectors::ORCHARD_ZSA_SHIELDED_DATA,
)
.await
}
8 changes: 4 additions & 4 deletions zebra-test/src/vectors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,20 @@ mod orchard_note_encryption;
mod orchard_shielded_data;

#[cfg(feature = "tx-v6")]
mod orchard_shielded_data_zsa;
mod orchard_zsa_shielded_data;

#[cfg(feature = "tx-v6")]
mod orchard_workflow_blocks_zsa;
mod orchard_zsa_workflow_blocks;

pub use block::*;
pub use orchard_note_encryption::*;
pub use orchard_shielded_data::*;

#[cfg(feature = "tx-v6")]
pub use orchard_shielded_data_zsa::*;
pub use orchard_zsa_shielded_data::*;

#[cfg(feature = "tx-v6")]
pub use orchard_workflow_blocks_zsa::*;
pub use orchard_zsa_workflow_blocks::*;

/// A testnet transaction test vector
///
Expand Down
34 changes: 0 additions & 34 deletions zebra-test/src/vectors/orchard_shielded_data_zsa.rs

This file was deleted.

34 changes: 34 additions & 0 deletions zebra-test/src/vectors/orchard_zsa_shielded_data.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
//! OrchardZSA shielded data (with Actions) test vectors
//!
//! Generated by `zebra_chain::primitives::halo2::tests::generate_test_vectors()`

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

remove // FIXME: Where is this function called from? in halo2::tests::generate_test_vectors()

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Removed.

//!
//! These are artificial/incomplete `zebra_chain::orchard::ShieldedData`
//! instances, care should be used when using them to test functionality beyond
//! verifying a standalone Orchard Acton Halo2 proof.

#![allow(missing_docs)]

use hex::FromHex;
use lazy_static::lazy_static;

lazy_static! {
pub static ref ORCHARD_ZSA_SHIELDED_DATA: Vec<&'static [u8]> = [
ORCHARD_ZSA_SHIELDED_DATA_1_BYTES.as_ref(),
ORCHARD_ZSA_SHIELDED_DATA_2_BYTES.as_ref(),
ORCHARD_ZSA_SHIELDED_DATA_3_BYTES.as_ref(),
ORCHARD_ZSA_SHIELDED_DATA_4_BYTES.as_ref(),
]
.to_vec();
pub static ref ORCHARD_ZSA_SHIELDED_DATA_1_BYTES: Vec<u8> =
<Vec<u8>>::from_hex(include_str!("orchard-zsa-shielded-data-1.txt").trim())
.expect("OrchardZSA shielded data bytes are in valid hex representation");
pub static ref ORCHARD_ZSA_SHIELDED_DATA_2_BYTES: Vec<u8> =
<Vec<u8>>::from_hex(include_str!("orchard-zsa-shielded-data-2.txt").trim())
.expect("OrchardZSA shielded data bytes are in valid hex representation");
pub static ref ORCHARD_ZSA_SHIELDED_DATA_3_BYTES: Vec<u8> =
<Vec<u8>>::from_hex(include_str!("orchard-zsa-shielded-data-3.txt").trim())
.expect("OrchardZSA shielded data bytes are in valid hex representation");
pub static ref ORCHARD_ZSA_SHIELDED_DATA_4_BYTES: Vec<u8> =
<Vec<u8>>::from_hex(include_str!("orchard-zsa-shielded-data-4.txt").trim())
.expect("OrchardZSA shielded data bytes are in valid hex representation");
}

Large diffs are not rendered by default.