From 4fa119dd1f93ec6c6e8e7efb07b4bb974a6a6d92 Mon Sep 17 00:00:00 2001 From: Henry de Valence Date: Mon, 30 Nov 2020 15:10:22 -0800 Subject: [PATCH] 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 | 19 ++++++++-- zebra-chain/src/transparent/serialize.rs | 36 +++++++++++-------- zebra-state/src/constants.rs | 2 +- .../src/vectors/block-main-0-000-202-bad.txt | 1 + zebra-test/src/vectors/block.rs | 6 ++++ 5 files changed, 46 insertions(+), 18 deletions(-) create mode 100644 zebra-test/src/vectors/block-main-0-000-202-bad.txt diff --git a/zebra-chain/src/block/tests/vectors.rs b/zebra-chain/src/block/tests/vectors.rs index 15a47e771..63799aa52 100644 --- a/zebra-chain/src/block/tests/vectors.rs +++ b/zebra-chain/src/block/tests/vectors.rs @@ -80,13 +80,28 @@ fn deserialize_block() { .zcash_deserialize_into::() .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::() .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::() + .expect_err("parsing fails"); +} + #[test] fn block_test_vectors_unique() { zebra_test::init(); diff --git a/zebra-chain/src/transparent/serialize.rs b/zebra-chain/src/transparent/serialize.rs index 68937ba67..9b93d46cb 100644 --- a/zebra-chain/src/transparent/serialize.rs +++ b/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(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::(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::(h)?; } else { diff --git a/zebra-state/src/constants.rs b/zebra-state/src/constants.rs index 888ca8b40..d99c14205 100644 --- a/zebra-state/src/constants.rs +++ b/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; diff --git a/zebra-test/src/vectors/block-main-0-000-202-bad.txt b/zebra-test/src/vectors/block-main-0-000-202-bad.txt new file mode 100644 index 000000000..6ad1fbd79 --- /dev/null +++ b/zebra-test/src/vectors/block-main-0-000-202-bad.txt @@ -0,0 +1 @@ +04000000efaac18270e32193ed370edc5248fc82c2f10631a41bac03c250eca374360000c04ad19e73aa798345431eb39c14094cc1f3a12298a5a942e6a9aba7c2bc6ae00000000000000000000000000000000000000000000000000000000000000000637e1358f636391e980c0038000000000000000038000c990a000000000000000000000000000003fd4005018016942ceb483b769826e7b73d025677fc97e9cc27e3c42e720f087fe35ab55d8bdde092bbed994b9e03cc1aa721c81529abdaa715ac6d9caa0ae69f3a814a64ccf1002bae0bd9a937af745a7d471bbc1b368707de40405e84f50de73be22ad07d1d8636e833e1d5223edeb0699e6f0507f304aecaf4b91f04d4bbd8a32af52c262792edc5cb3016ee267412b2113a94234030cc1abad860c2ef674346c2a861220e5af07742b705b503afd9d5d77f2a9443d70772814e1424d4ab7d0d4d2ba04db67711f0b8f0de0de179708fdcf541301419ff080bed8b47fbe5f1431bee9c7d5c99f480541c7bcabbdea2a16f19eb9400dfcf6191d62598da800a2634e3fc75411fc9b2e104a442fa34df6e36712d119b21cc1cb0cd5bc0ea614c194f6b554176d9e8190f120ecb53904996acfdd6ce467ac5a710a75fc4b811b60a34f588e644524311cef3b282a97ea373472c05056a8c8264d56f905c065495ce29320b22bd0f6f140d4fee47d98cc4d018136c3faed8f133de56c272080dc135756f452bb249f640a9f02c1240c019a88409295b9f78ed8c6d94d172dd52b03e2a10be7776aa0add3fd049d27049e91df2c5aa48268a29c8dc06fd0bef5d7ea8c5900a64552243a1cec338d8607d64ea0e6f66d5cac9a719ff8f25fa40ce84367b6f5483bb1cb12dfa62d0453360da72fdaf201ab93979d7780005384997726e29afcbdfd0d233a46786d98e3edd210c3fdd23b0c57d41c235d5459347514343ffde132f2febf54fc918943d0a53a35ad6f31bd4f6aebe954b832776e75ef80fffca98c90405d5f29a4f71d3c4e00e6c6c722a0f6ba1214793ac03e88b4682a31a6508289e233065a6013b965e72ba48e5af9936c5ce783919242aece8c6f38cbb8e638a5a372e66021615abb9321e4a02f4cf9e07e4eb65ca8b6d3de9ef42d50eaf01ec9a3b039bcdd3403cb273f5b6f57595073109210df040bbf1cceee39d7f493828df317f0c6e5dc00025b173191c8b0986d437b385294ad013cdc83f8ffb267c0d9fa9e7d4bfb320a547e433654deffdb079700246ec9de9225b3b7a28d26691e2645dc718736a740a64afd48a287a79da4900a917daddfa730437efbd0b0bd3f8c44cfba53da5221e459b604b37b7bc9ea21315e220fad31772ac4754198eb6374dbdfdf9f70301f1bbfe05d0ebb48a1d108df04cd7dd430cb40de913ab0db41095b02ceada44a90e4bab2980102c85961bf4565bb469718370fd6ba461604fab289cdfd7f258741b32b3699dab5d06b85d7e6478d21efbf38425025c502024116d5729d560b378fe6649e01b38faca16eb07c1b8506418e67dd3bc885efebcf36a2a7df416e9f5346054c493e179f593bfda2a32d15898b26718f56a931a87c187ac383c78197ad577673cdd86e30348d096225beeabdfc2153f1e6297d9de425428ae0ebe7e7cb3867d6db623a7553e67255a750414d0dc0bdd6232bfb18fb39173a220665a55ea26b91e95741ec3f36f6771072bbb69290df67c8e22e89b1fa0860938bf3bb819978d24df3296cc98b19f5be85ce66e293885d94a16949bd530ed6181edeb3769fe9bd5ee1b4dc405a14e3e6294f31279ff7e1556ae199c02552f6fa55f9ba49145ed23f4d8a6f82f01d72a18e9860356a463aad1d493c1ac1329526e042904a5288d4e18cf4b12a55d801f89f1e4eaba7eeae1740e1c80621a60f65db9dd5d3f652de2746d983d59e534daf7cb1a9dadbf1da49c31bb7de24027e0e5d20574b5947710e78e2766ea7483f89bb356aeac08bd4e54d43cc12add3d76d20c19f9fa1ac695b6ef967e4bf878ba41136856f84545573bf750f5e0e371e0dd910a52ddd630115699625664dd8d1c763a21da19aaaf4c1f6c7b0101000000010000000000000000000000000000000000000000000000000000000000000000ffffffff2601ca04637e135810000000000000000000000000000000000d2f6e6f64655374726174756d2f0000000002201d9a00000000002321028f26d5cd57e3817bf66095435d3996b608fc6e448af2bbd2361a0dac7879764eac488726000000000017a9147d46a730d31f97b1930d3368a967c309bd4d136a8700000000 diff --git a/zebra-test/src/vectors/block.rs b/zebra-test/src/vectors/block.rs index fae1624be..d69d885d4 100644 --- a/zebra-test/src/vectors/block.rs +++ b/zebra-test/src/vectors/block.rs @@ -167,6 +167,12 @@ lazy_static! { >::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 = + >::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