fix(chain): Validate header versions when serializing blocks

zebra-chain/src/block/serialize.rs

@@ -22,9 +22,49 @@ 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.
+///
+/// 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:
+        // "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<W: io::Write>(&self, mut writer: W) -> Result<(), io::Error> {
+        check_version(self.version).map_err(|msg| io::Error::new(io::ErrorKind::Other, msg))?;
+
         writer.write_u32::<LittleEndian>(self.version)?;
         self.previous_block_hash.zcash_serialize(&mut writer)?;
         writer.write_all(&self.merkle_root.0[..])?;
@@ -44,41 +84,8 @@ impl ZcashSerialize for Header {
 
 impl ZcashDeserialize for Header {
     fn zcash_deserialize<R: io::Read>(mut reader: R) -> Result<Self, SerializationError> {
-        // 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::<LittleEndian>()?;
-            (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::<LittleEndian>()?;
+        check_version(version).map_err(SerializationError::Parse)?;
 
         Ok(Header {
             version,

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::<Header>()
             .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::<Header>()
+            .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::<Header>()
+            .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");
     }
 }
 

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<block::Height, block::Hash> =