Skip to content

Add more V6 tests to zebra-chain, refactor OrchardZSA tests in zebra-test#62

Merged
dmidem merged 6 commits intozsa1from
zsa-integration-consensus-tests
Aug 5, 2025
Merged

Add more V6 tests to zebra-chain, refactor OrchardZSA tests in zebra-test#62
dmidem merged 6 commits intozsa1from
zsa-integration-consensus-tests

Conversation

@dmidem
Copy link
Copy Markdown
Collaborator

@dmidem dmidem commented Jun 23, 2025

Add more V6 tests to zebra-chain, refactor OrchardZSA tests in zebra-test

@dmidem dmidem requested a review from PaulLaux July 8, 2025 13:27
@dmidem dmidem added the stage1.1 label Jul 8, 2025
Copy link
Copy Markdown

@PaulLaux PaulLaux left a comment

Choose a reason for hiding this comment

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

added comments

Comment on lines +184 to +188
#[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.

.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.

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.

}

// 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.

}

#[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 ...

Comment on lines +523 to +530
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.

/// 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");

@@ -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.

@dmidem dmidem merged commit 3c9bac2 into zsa1 Aug 5, 2025
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants