From 6bf2e286a8d38b818fa2176585f298d18907165b Mon Sep 17 00:00:00 2001 From: arya2 Date: Fri, 7 Apr 2023 13:29:38 -0400 Subject: [PATCH 1/4] checks block header version when serializing --- zebra-chain/src/block/serialize.rs | 68 +++++++++++++++--------------- 1 file changed, 33 insertions(+), 35 deletions(-) diff --git a/zebra-chain/src/block/serialize.rs b/zebra-chain/src/block/serialize.rs index 4082df023e3..a0db53bea54 100644 --- a/zebra-chain/src/block/serialize.rs +++ b/zebra-chain/src/block/serialize.rs @@ -22,9 +22,40 @@ use crate::{ /// transaction in the chain is approximately 1.5 kB smaller.) pub const MAX_BLOCK_BYTES: u64 = 2_000_000; +/// Checks if a block header version is valid +fn check_version(version: u32) -> Result<(), &'static str> { + match version { + // The Zcash specification says that: + // "The current and only defined block version number for Zcash is 4." + // but this is not actually part of the consensus rules, and in fact + // broken mining software created blocks that do not have version 4. + // There are approximately 4,000 blocks with version 536870912; this + // is the bit-reversal of the value 4, indicating that that mining pool + // reversed bit-ordering of the version field. Because the version field + // was not properly validated, these blocks were added to the chain. + // + // The only possible way to work around this is to do a similar hack + // as the overwintered field in transaction parsing, which we do here: + // treat the high bit (which zcashd interprets as a sign bit) as an + // indicator that the version field is meaningful. + version if version >> 31 != 0 => Err("high bit was set in version field"), + + // # Consensus + // + // > The block version number MUST be greater than or equal to 4. + // + // https://zips.z.cash/protocol/protocol.pdf#blockheader + version if version < ZCASH_BLOCK_VERSION => Err("version must be at least 4"), + + _ => Ok(()), + } +} + impl ZcashSerialize for Header { #[allow(clippy::unwrap_in_result)] fn zcash_serialize(&self, mut writer: W) -> Result<(), io::Error> { + check_version(self.version).map_err(|msg| io::Error::new(io::ErrorKind::Other, msg))?; + writer.write_u32::(self.version)?; self.previous_block_hash.zcash_serialize(&mut writer)?; writer.write_all(&self.merkle_root.0[..])?; @@ -44,41 +75,8 @@ impl ZcashSerialize for Header { impl ZcashDeserialize for Header { fn zcash_deserialize(mut reader: R) -> Result { - // The Zcash specification says that - // "The current and only defined block version number for Zcash is 4." - // but this is not actually part of the consensus rules, and in fact - // broken mining software created blocks that do not have version 4. - // There are approximately 4,000 blocks with version 536870912; this - // is the bit-reversal of the value 4, indicating that that mining pool - // reversed bit-ordering of the version field. Because the version field - // was not properly validated, these blocks were added to the chain. - // - // The only possible way to work around this is to do a similar hack - // as the overwintered field in transaction parsing, which we do here: - // treat the high bit (which zcashd interprets as a sign bit) as an - // indicator that the version field is meaningful. - // - // - let (version, future_version_flag) = { - const LOW_31_BITS: u32 = (1 << 31) - 1; - let raw_version = reader.read_u32::()?; - (raw_version & LOW_31_BITS, raw_version >> 31 != 0) - }; - - if future_version_flag { - return Err(SerializationError::Parse( - "high bit was set in version field", - )); - } - - // # Consensus - // - // > The block version number MUST be greater than or equal to 4. - // - // https://zips.z.cash/protocol/protocol.pdf#blockheader - if version < ZCASH_BLOCK_VERSION { - return Err(SerializationError::Parse("version must be at least 4")); - } + let version = reader.read_u32::()?; + check_version(version).map_err(SerializationError::Parse)?; Ok(Header { version, From 86ec19b4a39bcf2001818e098c9e2da6a82b5588 Mon Sep 17 00:00:00 2001 From: arya2 Date: Tue, 11 Apr 2023 16:41:58 -0400 Subject: [PATCH 2/4] fixes test panic --- zebra-consensus/src/checkpoint/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zebra-consensus/src/checkpoint/tests.rs b/zebra-consensus/src/checkpoint/tests.rs index 85fe894951e..66331310735 100644 --- a/zebra-consensus/src/checkpoint/tests.rs +++ b/zebra-consensus/src/checkpoint/tests.rs @@ -504,7 +504,7 @@ async fn wrong_checkpoint_hash_fail() -> Result<(), Report> { // Change the header hash let mut bad_block0 = good_block0.clone(); let bad_block0_mut = Arc::make_mut(&mut bad_block0); - Arc::make_mut(&mut bad_block0_mut.header).version = 0; + Arc::make_mut(&mut bad_block0_mut.header).version = 5; // Make a checkpoint list containing the genesis block checkpoint let genesis_checkpoint_list: BTreeMap = From 7d7594f86b9ae0dab79a2e41792b7f41c89a6a33 Mon Sep 17 00:00:00 2001 From: arya2 Date: Tue, 11 Apr 2023 16:52:03 -0400 Subject: [PATCH 3/4] adds docs --- zebra-chain/src/block/serialize.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/zebra-chain/src/block/serialize.rs b/zebra-chain/src/block/serialize.rs index a0db53bea54..e763915e499 100644 --- a/zebra-chain/src/block/serialize.rs +++ b/zebra-chain/src/block/serialize.rs @@ -22,7 +22,16 @@ use crate::{ /// transaction in the chain is approximately 1.5 kB smaller.) pub const MAX_BLOCK_BYTES: u64 = 2_000_000; -/// Checks if a block header version is valid +/// Checks if a block header version is valid. +/// +/// Zebra could encounter a [`Header`] with an invalid version when serializing a block header constructed +/// in memory with the wrong version in tests or the getblocktemplate RPC. +/// +/// The getblocktemplate RPC generates a template with version 4. The miner generates the actual block, +/// and then we deserialize it and do this check. +/// +/// All other blocks are deserialized when we receive them, and never modified, +/// so the deserialisation would pick up any errors. fn check_version(version: u32) -> Result<(), &'static str> { match version { // The Zcash specification says that: From 6b4ec1b79804e0bd9c7b3f61f3c060aa44a49b8c Mon Sep 17 00:00:00 2001 From: arya2 Date: Tue, 11 Apr 2023 20:10:09 -0400 Subject: [PATCH 4/4] test block header (de)serialization --- zebra-chain/src/block/tests/vectors.rs | 62 ++++++++++++++++++++++++-- 1 file changed, 59 insertions(+), 3 deletions(-) diff --git a/zebra-chain/src/block/tests/vectors.rs b/zebra-chain/src/block/tests/vectors.rs index 2d05cc39777..257614becf1 100644 --- a/zebra-chain/src/block/tests/vectors.rs +++ b/zebra-chain/src/block/tests/vectors.rs @@ -13,7 +13,9 @@ use crate::{ Network::{self, *}, NetworkUpgrade::*, }, - serialization::{sha256d, ZcashDeserialize, ZcashDeserializeInto, ZcashSerialize}, + serialization::{ + sha256d, SerializationError, ZcashDeserialize, ZcashDeserializeInto, ZcashSerialize, + }, transaction::LockTime, }; @@ -63,7 +65,7 @@ fn blockheaderhash_from_blockheader() { } #[test] -fn deserialize_blockheader() { +fn blockheader_serialization() { let _init_guard = zebra_test::init(); // Includes the 32-byte nonce and 3-byte equihash length field. @@ -73,11 +75,65 @@ fn deserialize_blockheader() { + crate::work::equihash::SOLUTION_SIZE; for block in zebra_test::vectors::BLOCKS.iter() { + // successful deserialization + let header_bytes = &block[..BLOCK_HEADER_LENGTH]; - let _header = header_bytes + let mut header = header_bytes .zcash_deserialize_into::
() .expect("blockheader test vector should deserialize"); + + // successful serialization + + let _serialized_header = header + .zcash_serialize_to_vec() + .expect("blockheader test vector should serialize"); + + // deserialiation errors + + let header_bytes = [&[255; 4], &header_bytes[4..]].concat(); + + let deserialization_err = header_bytes + .zcash_deserialize_into::
() + .expect_err("blockheader test vector should fail to deserialize"); + + let SerializationError::Parse(err_msg) = deserialization_err else { + panic!("SerializationError variant should be Parse") + }; + + assert_eq!(err_msg, "high bit was set in version field"); + + let header_bytes = [&[0; 4], &header_bytes[4..]].concat(); + + let deserialization_err = header_bytes + .zcash_deserialize_into::
() + .expect_err("blockheader test vector should fail to deserialize"); + + let SerializationError::Parse(err_msg) = deserialization_err else { + panic!("SerializationError variant should be Parse") + }; + + assert_eq!(err_msg, "version must be at least 4"); + + // serialiation errors + + header.version = u32::MAX; + + let serialization_err = header + .zcash_serialize_to_vec() + .expect_err("blockheader test vector with modified version should fail to serialize"); + + assert_eq!( + serialization_err.kind(), + std::io::ErrorKind::Other, + "error kind should be Other" + ); + + let err_msg = serialization_err + .into_inner() + .expect("there should be an inner error"); + + assert_eq!(err_msg.to_string(), "high bit was set in version field"); } }