Sync Zebra with latest Orchard and librustzcash#92
Conversation
Cargo.toml
Outdated
| zcash_note_encryption = { version = "0.4.1", git = "https://github.com/zcash/zcash_note_encryption", rev = "9f7e93d42cef839d02b9d75918117941d453f8cb" } | ||
| sapling-crypto = { package = "sapling-crypto", version = "0.5", git = "https://github.com/QED-it/sapling-crypto", rev = "9393f93fe547d1b3738c9f4618c0f8a2fffed29f" } | ||
| orchard = { version = "0.11.0", git = "https://github.com/QED-it/orchard", rev = "2083efe8d57e6073914ae296db2d41f8bfe1de50" } | ||
| zcash_primitives = { version = "0.25.0", git = "https://github.com/QED-it/librustzcash", rev = "a7e5a5fe90181965aff5e42f071178dd3492e626" } |
There was a problem hiding this comment.
please use this commit for librustzcash (all):
There was a problem hiding this comment.
We merged it today and we need it in (minor update to fee api).
To integrate into zebra, start with no_new_assets() like in the example here: https://github.com/QED-it/librustzcash/blob/e27124a49a0e549227e12adf13e42f88593f1dee/zcash_primitives/src/transaction/builder.rs#L1420
Later, when state management is merged, add the real predicate (we will discuss in due time)
There was a problem hiding this comment.
As agreed, I added a TODO comment to the conventional_actions function in zebra-chain/src/transaction/unmined/zip317.rs.
There was a problem hiding this comment.
I don't see the change. we need QED-it/librustzcash@e27124a
There was a problem hiding this comment.
Fixed (upgraded librustzcash crates revisions to e271... in Cargo.toml).
| .map(|sig| orchard::AuthorizedAction::from_parts(action, sig)) | ||
| }) | ||
| .collect(); | ||
| .collect::<Result<Vec<_>, _>>()?; |
There was a problem hiding this comment.
.collect::<Result<_, _>>()?; is enough.
|
|
||
| // Denoted as `vSighashInfo` in the spec. | ||
| // There is one sighash info per transparent input. For now, only V0 is supported. | ||
| for _ in 0..inputs.len() { |
| // Denoted as `vSighashInfo` in the spec (ZIP-230). | ||
| // There is one `TransparentSighashInfo` per transparent input (tx_in_count entries). | ||
| // For now, only V0 is supported, which must decode to a Vector<u8> == [0x00]. | ||
| for _ in 0..inputs.len() { |
zebra-chain/src/versioned_sig.rs
Outdated
| } | ||
|
|
||
| Ok(SighashInfo { | ||
| version: SighashVersion(version[0]), |
There was a problem hiding this comment.
lets check that it is v0 here and avoid external calls to is_v0 (remove is_v0 completely.
There was a problem hiding this comment.
Your solution is more general but in this case we only want to support v0 at the parser level. Also this will eliminate the invariant of checking for is_v0 at serialize.rs
There was a problem hiding this comment.
I've refactored and simplified the the code so we don't have this function now, see #92 (comment)
zebra-chain/src/versioned_sig.rs
Outdated
| } | ||
|
|
||
| #[test] | ||
| fn sighash_info_with_data() { |
There was a problem hiding this comment.
I've refactored and simplified the the code so we don't have this test now, see #92 (comment)
zebra-chain/src/versioned_sig.rs
Outdated
| } | ||
|
|
||
| let sig = TestSig([0x11, 0x22, 0x33, 0x44]); | ||
| let versioned = VersionedSig::for_tx_v6(sig.clone()); |
zebra-chain/src/versioned_sig.rs
Outdated
| let versioned = VersionedSig::for_tx_v6(sig.clone()); | ||
|
|
||
| let bytes = versioned.zcash_serialize_to_vec().unwrap(); | ||
| // sighash info bytes (CompactSize(1) + version 0) + signature bytes |
There was a problem hiding this comment.
More clear
// Expected format: [CompactSize(1), version(0), sig_bytes...]
// 0x01 = CompactSize encoding of length 1 (just the version byte)
// 0x00 = sighash version 0
// 0x11, 0x22, 0x33, 0x44 = the test signature bytes….rs to use the include_str! macro for storing test blocks in external files
…or the Orchard ZSA workflow test in zebra-consensus to prevent it from getting stuck when block heights are not sequential
…review comments
…h-a7e5' into upgrade-orchard-2083-librustzcash-a7e5
PaulLaux
left a comment
There was a problem hiding this comment.
Looks good, added minor comments.
the requested upgrade of librustzcash is missing.
Cargo.toml
Outdated
| zcash_note_encryption = { version = "0.4.1", git = "https://github.com/zcash/zcash_note_encryption", rev = "9f7e93d42cef839d02b9d75918117941d453f8cb" } | ||
| sapling-crypto = { package = "sapling-crypto", version = "0.5", git = "https://github.com/QED-it/sapling-crypto", rev = "9393f93fe547d1b3738c9f4618c0f8a2fffed29f" } | ||
| orchard = { version = "0.11.0", git = "https://github.com/QED-it/orchard", rev = "2083efe8d57e6073914ae296db2d41f8bfe1de50" } | ||
| zcash_primitives = { version = "0.25.0", git = "https://github.com/QED-it/librustzcash", rev = "a7e5a5fe90181965aff5e42f071178dd3492e626" } |
There was a problem hiding this comment.
I don't see the change. we need QED-it/librustzcash@e27124a
| csm.zcash_serialize(&mut writer)?; | ||
| // Denoted as `nActionGroupsOrchard` in the spec (ZIP 230). | ||
| // TODO: Implement support for multiple groups as defined in ZIP 230 | ||
| // (now the number of action groups is hardcoded to 1 to support V5-like transactions only). |
There was a problem hiding this comment.
// Denoted as `nActionGroupsOrchard` in the spec (ZIP 230).
// TxV6 currently support only one action group.| // Denoted as `nActionGroupsOrchard` in the spec (ZIP 230). | ||
| // TODO: Implement support for multiple groups as defined in ZIP 230 | ||
| // (now the number of action groups is hardcoded to 1 to support V5-like transactions only). |
There was a problem hiding this comment.
// Denoted as `nActionGroupsOrchard` in the spec (ZIP 230).
// TxV6 currently support only one action group.| } | ||
|
|
||
| // TODO: After tx-v6 merge, refactor to share common serialization logic with V5. | ||
| // For now the only difference is versioned signatures in V6. |
There was a problem hiding this comment.
The line is incorrect, there are other changes. Let's remove completely (keep TODO: ..)
There was a problem hiding this comment.
Removed the second line.
| .check(block_verifier_router.clone()) | ||
| .await | ||
| timeout( | ||
| Duration::from_secs(60), |
There was a problem hiding this comment.
Let's reduce to 15 sec to keep it tight.
| .await | ||
| timeout( | ||
| Duration::from_secs(60), | ||
| Transcript::from(create_transcript_data()).check(block_verifier_router.clone()), |
This updates Zebra to the latest Orchard and librustzcash revisions and adjusts our code to compile against their breaking changes.