chain: fix consensus-critical coinbase encoding bug

The `CoinbaseData` parses the block height separately from the rest of the
free-form coinbase data.  However, it had two bugs:

1. It did not require that the height was canonically encoded;
2. Its canonical encoding was incorrect relative to the BIP34-inherited encoding.

This meant that we computed some transaction hashes incorrectly, because when
we re-serialized the coinbase transaction, we would canonically serialize the
coinbase transaction (using the incorrect definition of canonical, bug 2).  And
we didn't notice that the wrong definition of canonical encoding was being used
because we accepted what we thought were non-canonically encoded heights.

The relevant rules are here: https://github.com/zcash/zcash/blob/877212414ad40e37cf8e49884a90767c54d59ba2/src/script/script.h#L307-L346

This commit changes the encoding to reject non-canonically encoded heights, and
to match the correct encoding rules.  We check that at least one
non-canonically encoded height is correctly rejected using a new test vector.

The database format increments because we saved a bunch of wrongly encoded blocks.

This discrepancy was originally noticed by @teor2345, who pointed out that a
previous version of the block 202 test vector (now preserved as "bad block
202") did not match the block from zcashd.

zebra-chain/src/block/tests/vectors.rs

@@ -80,13 +80,28 @@ fn deserialize_block() {
         .zcash_deserialize_into::<Block>()
         .expect("block test vector should deserialize");
 
-    for block in zebra_test::vectors::BLOCKS.iter() {
-        block
+    for block_bytes in zebra_test::vectors::BLOCKS.iter() {
+        let block = block_bytes
             .zcash_deserialize_into::<Block>()
             .expect("block is structurally valid");
+
+        let round_trip_bytes = block
+            .zcash_serialize_to_vec()
+            .expect("vec serialization is infallible");
+
+        assert_eq!(&round_trip_bytes[..], *block_bytes);
     }
 }
 
+#[test]
+fn coinbase_parsing_rejects_above_0x80() {
+    zebra_test::init();
+
+    zebra_test::vectors::BAD_BLOCK_MAINNET_202_BYTES
+        .zcash_deserialize_into::<Block>()
+        .expect_err("parsing fails");
+}
+
 #[test]
 fn block_test_vectors_unique() {
     zebra_test::init();

zebra-chain/src/transparent/serialize.rs

@@ -58,17 +58,20 @@ fn parse_coinbase_height(
             Height((op_n - 0x50) as u32),
             CoinbaseData(data.split_off(1)),
         )),
-        // Blocks 17 through 256 exclusive encode block height with the `0x01` opcode.
-        (Some(0x01), len) if len >= 2 => {
+        // Blocks 17 through 128 exclusive encode block height with the `0x01` opcode.
+        // The Bitcoin encoding requires that the most significant byte is below 0x80.
+        (Some(0x01), len) if len >= 2 && data[1] < 0x80 => {
             Ok((Height(data[1] as u32), CoinbaseData(data.split_off(2))))
         }
-        // Blocks 256 through 65536 exclusive encode block height with the `0x02` opcode.
-        (Some(0x02), len) if len >= 3 => Ok((
+        // Blocks 128 through 32768 exclusive encode block height with the `0x02` opcode.
+        // The Bitcoin encoding requires that the most significant byte is below 0x80.
+        (Some(0x02), len) if len >= 3 && data[2] < 0x80 => Ok((
             Height(data[1] as u32 + ((data[2] as u32) << 8)),
             CoinbaseData(data.split_off(3)),
         )),
-        // Blocks 65536 through 2**24 exclusive encode block height with the `0x03` opcode.
-        (Some(0x03), len) if len >= 4 => Ok((
+        // Blocks 65536 through 2**23 exclusive encode block height with the `0x03` opcode.
+        // The Bitcoin encoding requires that the most significant byte is below 0x80.
+        (Some(0x03), len) if len >= 4 && data[3] < 0x80 => Ok((
             Height(data[1] as u32 + ((data[2] as u32) << 8) + ((data[3] as u32) << 16)),
             CoinbaseData(data.split_off(4)),
         )),
@@ -82,7 +85,8 @@ fn parse_coinbase_height(
             Ok((Height(0), CoinbaseData(data)))
         }
         // As noted above, this is included for completeness.
-        (Some(0x04), len) if len >= 5 => {
+        // The Bitcoin encoding requires that the most significant byte is below 0x80.
+        (Some(0x04), len) if len >= 5 && data[4] < 0x80 => {
             let h = data[1] as u32
                 + ((data[2] as u32) << 8)
                 + ((data[3] as u32) << 16)
@@ -106,13 +110,13 @@ fn coinbase_height_len(height: block::Height) -> usize {
         0
     } else if let _h @ 1..=16 = height.0 {
         1
-    } else if let _h @ 17..=255 = height.0 {
+    } else if let _h @ 17..=127 = height.0 {
         2
-    } else if let _h @ 256..=65535 = height.0 {
+    } else if let _h @ 128..=32767 = height.0 {
         3
-    } else if let _h @ 65536..=16_777_215 = height.0 {
+    } else if let _h @ 32768..=8_388_607 = height.0 {
         4
-    } else if let _h @ 16_777_216..=block::Height::MAX_AS_U32 = height.0 {
+    } else if let _h @ 8_388_608..=block::Height::MAX_AS_U32 = height.0 {
         5
     } else {
         panic!("Invalid coinbase height");
@@ -122,22 +126,24 @@ fn coinbase_height_len(height: block::Height) -> usize {
 fn write_coinbase_height<W: io::Write>(height: block::Height, mut w: W) -> Result<(), io::Error> {
     // We can't write this as a match statement on stable until exclusive range
     // guards are stabilized.
+    // The Bitcoin encoding requires that the most significant byte is below 0x80,
+    // so the ranges run up to 2^{n-1} rather than 2^n.
     if let 0 = height.0 {
         // Genesis block does not include height.
     } else if let h @ 1..=16 = height.0 {
         w.write_u8(0x50 + (h as u8))?;
-    } else if let h @ 17..=255 = height.0 {
+    } else if let h @ 17..=127 = height.0 {
         w.write_u8(0x01)?;
         w.write_u8(h as u8)?;
-    } else if let h @ 256..=65535 = height.0 {
+    } else if let h @ 128..=32767 = height.0 {
         w.write_u8(0x02)?;
         w.write_u16::<LittleEndian>(h as u16)?;
-    } else if let h @ 65536..=16_777_215 = height.0 {
+    } else if let h @ 32768..=8_388_607 = height.0 {
         w.write_u8(0x03)?;
         w.write_u8(h as u8)?;
         w.write_u8((h >> 8) as u8)?;
         w.write_u8((h >> 16) as u8)?;
-    } else if let h @ 16_777_216..=block::Height::MAX_AS_U32 = height.0 {
+    } else if let h @ 8_388_608..=block::Height::MAX_AS_U32 = height.0 {
         w.write_u8(0x04)?;
         w.write_u32::<LittleEndian>(h)?;
     } else {

zebra-state/src/constants.rs

@@ -12,4 +12,4 @@ pub const MIN_TRANSPARENT_COINBASE_MATURITY: u32 = 100;
 pub const MAX_BLOCK_REORG_HEIGHT: u32 = MIN_TRANSPARENT_COINBASE_MATURITY - 1;
 
 /// The database format version, incremented each time the database format changes.
-pub const DATABASE_FORMAT_VERSION: u32 = 3;
+pub const DATABASE_FORMAT_VERSION: u32 = 4;

zebra-test/src/vectors/block-main-0-000-202-bad.txt

@@ -0,0 +1 @@
+04000000efaac18270e32193ed370edc5248fc82c2f10631a41bac03c250eca374360000c04ad19e73aa798345431eb39c14094cc1f3a12298a5a942e6a9aba7c2bc6ae00000000000000000000000000000000000000000000000000000000000000000637e1358f636391e980c0038000000000000000038000c990a000000000000000000000000000003fd4005018016942ceb483b769826e7b73d025677fc97e9cc27e3c42e720f087fe35ab55d8bdde092bbed994b9e03cc1aa721c81529abdaa715ac6d9caa0ae69f3a814a64ccf1002bae0bd9a937af745a7d471bbc1b368707de40405e84f50de73be22ad07d1d8636e833e1d5223edeb0699e6f0507f304aecaf4b91f04d4bbd8a32af52c262792edc5cb3016ee267412b2113a94234030cc1abad860c2ef674346c2a861220e5af07742b705b503afd9d5d77f2a9443d70772814e1424d4ab7d0d4d2ba04db67711f0b8f0de0de179708fdcf541301419ff080bed8b47fbe5f1431bee9c7d5c99f480541c7bcabbdea2a16f19eb9400dfcf6191d62598da800a2634e3fc75411fc9b2e104a442fa34df6e36712d119b21cc1cb0cd5bc0ea614c194f6b554176d9e8190f120ecb53904996acfdd6ce467ac5a710a75fc4b811b60a34f588e644524311cef3b282a97ea373472c05056a8c8264d56f905c065495ce29320b22bd0f6f140d4fee47d98cc4d018136c3faed8f133de56c272080dc135756f452bb249f640a9f02c1240c019a88409295b9f78ed8c6d94d172dd52b03e2a10be7776aa0add3fd049d27049e91df2c5aa48268a29c8dc06fd0bef5d7ea8c5900a64552243a1cec338d8607d64ea0e6f66d5cac9a719ff8f25fa40ce84367b6f5483bb1cb12dfa62d0453360da72fdaf201ab93979d7780005384997726e29afcbdfd0d233a46786d98e3edd210c3fdd23b0c57d41c235d5459347514343ffde132f2febf54fc918943d0a53a35ad6f31bd4f6aebe954b832776e75ef80fffca98c90405d5f29a4f71d3c4e00e6c6c722a0f6ba1214793ac03e88b4682a31a6508289e233065a6013b965e72ba48e5af9936c5ce783919242aece8c6f38cbb8e638a5a372e66021615abb9321e4a02f4cf9e07e4eb65ca8b6d3de9ef42d50eaf01ec9a3b039bcdd3403cb273f5b6f57595073109210df040bbf1cceee39d7f493828df317f0c6e5dc00025b173191c8b0986d437b385294ad013cdc83f8ffb267c0d9fa9e7d4bfb320a547e433654deffdb079700246ec9de9225b3b7a28d26691e2645dc718736a740a64afd48a287a79da4900a917daddfa730437efbd0b0bd3f8c44cfba53da5221e459b604b37b7bc9ea21315e220fad31772ac4754198eb6374dbdfdf9f70301f1bbfe05d0ebb48a1d108df04cd7dd430cb40de913ab0db41095b02ceada44a90e4bab2980102c85961bf4565bb469718370fd6ba461604fab289cdfd7f258741b32b3699dab5d06b85d7e6478d21efbf38425025c502024116d5729d560b378fe6649e01b38faca16eb07c1b8506418e67dd3bc885efebcf36a2a7df416e9f5346054c493e179f593bfda2a32d15898b26718f56a931a87c187ac383c78197ad577673cdd86e30348d096225beeabdfc2153f1e6297d9de425428ae0ebe7e7cb3867d6db623a7553e67255a750414d0dc0bdd6232bfb18fb39173a220665a55ea26b91e95741ec3f36f6771072bbb69290df67c8e22e89b1fa0860938bf3bb819978d24df3296cc98b19f5be85ce66e293885d94a16949bd530ed6181edeb3769fe9bd5ee1b4dc405a14e3e6294f31279ff7e1556ae199c02552f6fa55f9ba49145ed23f4d8a6f82f01d72a18e9860356a463aad1d493c1ac1329526e042904a5288d4e18cf4b12a55d801f89f1e4eaba7eeae1740e1c80621a60f65db9dd5d3f652de2746d983d59e534daf7cb1a9dadbf1da49c31bb7de24027e0e5d20574b5947710e78e2766ea7483f89bb356aeac08bd4e54d43cc12add3d76d20c19f9fa1ac695b6ef967e4bf878ba41136856f84545573bf750f5e0e371e0dd910a52ddd630115699625664dd8d1c763a21da19aaaf4c1f6c7b0101000000010000000000000000000000000000000000000000000000000000000000000000ffffffff2601ca04637e135810000000000000000000000000000000000d2f6e6f64655374726174756d2f0000000002201d9a00000000002321028f26d5cd57e3817bf66095435d3996b608fc6e448af2bbd2361a0dac7879764eac488726000000000017a9147d46a730d31f97b1930d3368a967c309bd4d136a8700000000

zebra-test/src/vectors/block.rs

@@ -167,6 +167,12 @@ lazy_static! {
         <Vec<u8>>::from_hex(include_str!("block-main-0-000-202.txt").trim())
             .expect("Block bytes are in valid hex representation");
 
+    /// This contains an encoding of block 202 but with an improperly encoded
+    /// coinbase height.
+    pub static ref BAD_BLOCK_MAINNET_202_BYTES: Vec<u8> =
+        <Vec<u8>>::from_hex(include_str!("block-main-0-000-202-bad.txt").trim())
+            .expect("Block bytes are in valid hex representation");
+
     // Overwinter transition
     // for i in 347499 347500 347501; do
     //     zcash-cli getblock $i 0 > block-main-$[i/1000000]-$[i/1000%1000]-$[i%1000].txt