Merge rust-bitcoin/rust-bitcoin#1244: BIP 34: Check last minimal encoding accoriding of heights

840520042a BIP 34: Check last minimal encoding of encoded heights in coinbase tx (sanket1729)
fd3e01d6e3 Fix minimality of reading/writing CScriptNums (sanket1729)

Pull request description:

  See: https://github.com/bitcoin/bips/blob/master/bip-0034.mediawiki

  The BIP was updated with commit: 08844fd6ef

  Found while reviewing #1240

  Also cleanly deals with negative heights as errors.

ACKs for top commit:
  apoelstra:
    ACK 840520042a
  tcharding:
    ACK 840520042a

Tree-SHA512: 85eb1fbb428fb600f864390c3ac2e8e62efc1fc4b03b842e384ddb4ed9273c66d2274d0de219f1c7a6bad7b02bd8723f8f257c864577614df674b44bfc02010a

src/blockdata/block.rs

@@ -379,7 +379,7 @@ impl Block {
         // Citing the spec:
         // Add height as the first item in the coinbase transaction's scriptSig,
         // and increase block version to 2. The format of the height is
-        // "serialized CScript" -- first byte is number of bytes in the number
+        // "minimally encoded serialized CScript"" -- first byte is number of bytes in the number
         // (will be 0x03 on main net for the next 150 or so years with 2^23-1
         // blocks), following bytes are little-endian representation of the
         // number (including a sign bit). Height is the height of the mined
@@ -393,14 +393,14 @@ impl Block {
         let input = cb.input.first().ok_or(Bip34Error::NotPresent)?;
         let push = input.script_sig.instructions_minimal().next().ok_or(Bip34Error::NotPresent)?;
         match push.map_err(|_| Bip34Error::NotPresent)? {
-            script::Instruction::PushBytes(b) if b.len() <= 8 => {
-                // Expand the push to exactly 8 bytes (LE).
-                let mut full = [0; 8];
-                full[0..b.len()].copy_from_slice(b);
-                Ok(util::endian::slice_to_u64_le(&full))
-            }
-            script::Instruction::PushBytes(b) if b.len() > 8 => {
-                Err(Bip34Error::UnexpectedPush(b.to_vec()))
+            script::Instruction::PushBytes(b) => {
+                // Check that the number is encoded in the minimal way.
+                let h = script::read_scriptint(b).map_err(|_e| Bip34Error::UnexpectedPush(b.to_vec()))?;
+                if h < 0 {
+                    Err(Bip34Error::NegativeHeight)
+                } else {
+                    Ok(h as u64)
+                }
             }
             _ => Err(Bip34Error::NotPresent),
         }
@@ -417,6 +417,8 @@ pub enum Bip34Error {
     NotPresent,
     /// The BIP34 push was larger than 8 bytes.
     UnexpectedPush(Vec<u8>),
+    /// The BIP34 push was negative.
+    NegativeHeight,
 }
 
 impl fmt::Display for Bip34Error {
@@ -427,6 +429,7 @@ impl fmt::Display for Bip34Error {
             Bip34Error::UnexpectedPush(ref p) => {
                 write!(f, "unexpected byte push of > 8 bytes: {:?}", p)
             }
+            Bip34Error::NegativeHeight => write!(f, "negative BIP34 height"),
         }
     }
 }
@@ -438,7 +441,10 @@ impl std::error::Error for Bip34Error {
         use self::Bip34Error::*;
 
         match self {
-            Unsupported | NotPresent | UnexpectedPush(_) => None,
+            Unsupported |
+            NotPresent |
+            UnexpectedPush(_) |
+            NegativeHeight => None,
         }
     }
 }

src/blockdata/script.rs

@@ -225,9 +225,9 @@ impl From<bitcoinconsensus::Error> for Error {
     }
 }
 
-/// Helper to encode an integer in script format.
+/// Helper to encode an integer in script(minimal CScriptNum) format.
 /// Writes bytes into the buffer and returns the number of bytes written.
-fn write_scriptint(out: &mut [u8; 8], n: i64) -> usize {
+pub fn write_scriptint(out: &mut [u8; 8], n: i64) -> usize {
     let mut len = 0;
     if n == 0 { return len; }
 
@@ -256,7 +256,7 @@ fn write_scriptint(out: &mut [u8; 8], n: i64) -> usize {
     len
 }
 
-/// Helper to decode an integer in script format
+/// Helper to decode an integer in script(minimal CScriptNum) format
 /// Notice that this fails on overflow: the result is the same as in
 /// bitcoind, that only 4-byte signed-magnitude values may be read as
 /// numbers. They can be added or subtracted (and a long time ago,
@@ -272,8 +272,26 @@ fn write_scriptint(out: &mut [u8; 8], n: i64) -> usize {
 /// This is basically a ranged type implementation.
 pub fn read_scriptint(v: &[u8]) -> Result<i64, Error> {
     let len = v.len();
-    if len == 0 { return Ok(0); }
     if len > 4 { return Err(Error::NumericOverflow); }
+    let last = match v.last() {
+        Some(last) => last,
+        None => return Ok(0),
+    };
+    // Comment and code copied from bitcoin core:
+    // https://github.com/bitcoin/bitcoin/blob/447f50e4aed9a8b1d80e1891cda85801aeb80b4e/src/script/script.h#L247-L262
+    // If the most-significant-byte - excluding the sign bit - is zero
+    // then we're not minimal. Note how this test also rejects the
+    // negative-zero encoding, 0x80.
+    if (last & 0x7f) == 0 {
+        // One exception: if there's more than one byte and the most
+        // significant bit of the second-most-significant-byte is set
+        // it would conflict with the sign bit. An example of this case
+        // is +-255, which encode to 0xff00 and 0xff80 respectively.
+        // (big-endian).
+        if v.len() <= 1 || (v[v.len() - 2] & 0x80) == 0 {
+            return Err(Error::NonMinimalPush);
+        }
+    }
 
     let (mut ret, sh) = v.iter()
                          .fold((0, 0), |(acc, sh), n| (acc + ((*n as i64) << sh), sh + 8));
@@ -1300,6 +1318,14 @@ mod test {
         assert!(read_scriptint(&build_scriptint(-(1 << 31))).is_err());
     }
 
+    #[test]
+    fn non_minimal_scriptints() {
+        assert_eq!(read_scriptint(&[0x80, 0x00]), Ok(0x80));
+        assert_eq!(read_scriptint(&[0xff, 0x00]), Ok(0xff));
+        assert_eq!(read_scriptint(&[0x8f, 0x00, 0x00]), Err(Error::NonMinimalPush));
+        assert_eq!(read_scriptint(&[0x7f, 0x00]), Err(Error::NonMinimalPush));
+    }
+
     #[test]
     fn script_hashes() {
         let script = hex_script!("410446ef0102d1ec5240f0d061a4246c1bdef63fc3dbab7733052fbbf0ecd8f41fc26bf049ebb4f9527f374280259e7cfa99c48b0e3f39c51347a19a5819651503a5ac");