From e730e84a09fb4b17574246758d0a6cfcf0f553f9 Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Fri, 23 Apr 2021 03:25:44 -0300 Subject: [PATCH] remove the `rest` field of v5 transaction (#2057) --- zebra-chain/src/transaction.rs | 3 +- zebra-chain/src/transaction/arbitrary.rs | 4 +- zebra-chain/src/transaction/serialize.rs | 19 ++++++--- zebra-chain/src/transaction/tests/vectors.rs | 45 ++------------------ 4 files changed, 18 insertions(+), 53 deletions(-) diff --git a/zebra-chain/src/transaction.rs b/zebra-chain/src/transaction.rs index f55c259ab..929563fd7 100644 --- a/zebra-chain/src/transaction.rs +++ b/zebra-chain/src/transaction.rs @@ -108,8 +108,7 @@ pub enum Transaction { outputs: Vec, /// The sapling shielded data for this transaction, if any. sapling_shielded_data: Option>, - /// The rest of the transaction as bytes - rest: Vec, + // TODO: The orchard data for this transaction, if any. }, } diff --git a/zebra-chain/src/transaction/arbitrary.rs b/zebra-chain/src/transaction/arbitrary.rs index ca79271cb..d460d5c72 100644 --- a/zebra-chain/src/transaction/arbitrary.rs +++ b/zebra-chain/src/transaction/arbitrary.rs @@ -109,17 +109,15 @@ impl Transaction { transparent::Input::vec_strategy(ledger_state, 10), vec(any::(), 0..10), option::of(any::>()), - any::>(), ) .prop_map( - |(lock_time, expiry_height, inputs, outputs, sapling_shielded_data, rest)| { + |(lock_time, expiry_height, inputs, outputs, sapling_shielded_data)| { Transaction::V5 { lock_time, expiry_height, inputs, outputs, sapling_shielded_data, - rest, } }, ) diff --git a/zebra-chain/src/transaction/serialize.rs b/zebra-chain/src/transaction/serialize.rs index f3485f0be..f6845d74c 100644 --- a/zebra-chain/src/transaction/serialize.rs +++ b/zebra-chain/src/transaction/serialize.rs @@ -346,7 +346,6 @@ impl ZcashSerialize for Transaction { inputs, outputs, sapling_shielded_data, - rest, } => { // Transaction V5 spec: // https://zips.z.cash/protocol/nu5.pdf#txnencodingandconsensus @@ -367,8 +366,10 @@ impl ZcashSerialize for Transaction { sapling_shielded_data.zcash_serialize(&mut writer)?; // orchard - // TODO: parse orchard into structs - writer.write_all(rest)?; + // TODO: Parse orchard into structs + // In the meantime, to keep the transaction valid, we add `0` + // as the nActionsOrchard field + writer.write_compactsize(0)?; } } Ok(()) @@ -501,9 +502,14 @@ impl ZcashDeserialize for Transaction { let sapling_shielded_data = (&mut reader).zcash_deserialize_into()?; // orchard - // TODO: parse orchard into structs - let mut rest = Vec::new(); - reader.read_to_end(&mut rest)?; + // TODO: Parse orchard into structs + // In the meantime, check the orchard section is just `0` + let empty_orchard_section = reader.read_compactsize()?; + if empty_orchard_section != 0 { + return Err(SerializationError::Parse( + "expected orchard section to be just a zero", + )); + } Ok(Transaction::V5 { lock_time, @@ -511,7 +517,6 @@ impl ZcashDeserialize for Transaction { inputs, outputs, sapling_shielded_data, - rest, }) } (_, _) => Err(SerializationError::Parse("bad tx header")), diff --git a/zebra-chain/src/transaction/tests/vectors.rs b/zebra-chain/src/transaction/tests/vectors.rs index 09892e037..1dc154a44 100644 --- a/zebra-chain/src/transaction/tests/vectors.rs +++ b/zebra-chain/src/transaction/tests/vectors.rs @@ -3,7 +3,7 @@ use super::super::*; use crate::{ block::{Block, MAX_BLOCK_BYTES}, sapling::{PerSpendAnchor, SharedAnchor}, - serialization::{WriteZcashExt, ZcashDeserialize, ZcashDeserializeInto, ZcashSerialize}, + serialization::{ZcashDeserialize, ZcashDeserializeInto, ZcashSerialize}, }; use itertools::Itertools; @@ -110,7 +110,6 @@ fn empty_v5_round_trip() { inputs: Vec::new(), outputs: Vec::new(), sapling_shielded_data: None, - rest: empty_v5_orchard_data(), }; let data = tx.zcash_serialize_to_vec().expect("tx should serialize"); @@ -245,31 +244,13 @@ fn fake_v5_round_trip() { ); // skip fake blocks which exceed the block size limit - // because of the changes we made if fake_bytes.len() > MAX_BLOCK_BYTES.try_into().unwrap() { continue; } - let fake_block2 = match fake_bytes.zcash_deserialize_into::() { - Ok(fake_block2) => fake_block2, - Err(err) => { - // TODO: work out why transaction parsing succeeds, - // but block parsing doesn't - tracing::info!( - ?err, - ?original_block, - ?fake_block, - hex_original_bytes = ?hex::encode(&original_bytes), - hex_fake_bytes = ?hex::encode(&fake_bytes), - original_bytes_len = %original_bytes.len(), - fake_bytes_len = %fake_bytes.len(), - %MAX_BLOCK_BYTES, - "unexpected structurally invalid block during deserialization" - ); - - continue; - } - }; + let fake_block2 = fake_bytes + .zcash_deserialize_into::() + .expect("block is structurally valid"); assert_eq!(fake_block, fake_block2); @@ -286,20 +267,6 @@ fn fake_v5_round_trip() { // Utility functions -/// Return serialized empty Transaction::V5 Orchard data. -/// -/// TODO: replace with orchard::ShieldedData (#1979) -fn empty_v5_orchard_data() -> Vec { - let mut buf = Vec::new(); - - // nActionsOrchard - buf.write_compactsize(0) - .expect("serialize to Vec always succeeds"); - - // all other orchard fields are only present when `nActionsOrchard > 0` - buf -} - /// Convert `trans` into a fake v5 transaction, /// converting sapling shielded data from v4 to v5 if possible. fn transaction_to_fake_v5(trans: &Transaction) -> Transaction { @@ -316,7 +283,6 @@ fn transaction_to_fake_v5(trans: &Transaction) -> Transaction { lock_time: *lock_time, expiry_height: block::Height(0), sapling_shielded_data: None, - rest: empty_v5_orchard_data(), }, V2 { inputs, @@ -329,7 +295,6 @@ fn transaction_to_fake_v5(trans: &Transaction) -> Transaction { lock_time: *lock_time, expiry_height: block::Height(0), sapling_shielded_data: None, - rest: empty_v5_orchard_data(), }, V3 { inputs, @@ -343,7 +308,6 @@ fn transaction_to_fake_v5(trans: &Transaction) -> Transaction { lock_time: *lock_time, expiry_height: *expiry_height, sapling_shielded_data: None, - rest: empty_v5_orchard_data(), }, V4 { inputs, @@ -361,7 +325,6 @@ fn transaction_to_fake_v5(trans: &Transaction) -> Transaction { .clone() .map(sapling_shielded_v4_to_fake_v5) .flatten(), - rest: empty_v5_orchard_data(), }, v5 @ V5 { .. } => v5.clone(), }