blockchain: fix bugs in bits_to_target and target_to_bits

This fixes three bugs:
- too large targets: the fixme in target_to_bits, which meant that we could
  not handle targets where the first bit was non-zero. This however cannot
  happen due to these being over MAX_TARGET. (difficulty 1)
- too small targets: in bits_to_target, very small targets were not handled well:
  ```
  >>> Blockchain.bits_to_target(0x03008000)
  32768
  ```
  We could not process headers with targets smaller than the above value.
  (note that these small targets would only occur at astronomically high mining difficulty)
- non-canonically encoded targets:
  we would not accept headers that had targets encoded in compact form (nBits) in a non-canonical way.
  I think this bug could actually be triggered by mining such a header.
  E.g. consider the target "1167130406913723784571467005534932607254396928"
  ```
  Blockchain.target_to_bits(1167130406913723784571467005534932607254396928).to_bytes(4, "big").hex()
  '13345600'
  ```
  Bitcoin Core when used to e.g. mine a block would encode this target as 0x13345600 in compact form.
  However, AFAICT, when validating Bitcoin Core would equally accept 0x14003456 which decodes to the
  same target. We were however rejecting such non-canonical compact nBits.
  ```
  >>> from electrum.blockchain import Blockchain
  >>> Blockchain.bits_to_target(0x14003456)
  Traceback (most recent call last):
    File "<stdin>", line 1, in <module>
    File "/home/user/wspace/electrum/electrum/blockchain.py", line 548, in bits_to_target
      raise Exception("Second part of bits should be in [0x8000, 0x7fffff]")
  Exception: Second part of bits should be in [0x8000, 0x7fffff]
  >>> Blockchain.bits_to_target(0x13345600)
  1167130406913723784571467005534932607254396928
  ```

electrum/blockchain.py

@@ -542,21 +542,37 @@ class Blockchain(Logger):
 
     @classmethod
     def bits_to_target(cls, bits: int) -> int:
+        # arith_uint256::SetCompact in Bitcoin Core
+        if not (0 <= bits < (1 << 32)):
+            raise Exception(f"bits should be uint32. got {bits!r}")
         bitsN = (bits >> 24) & 0xff
-        if not (0x03 <= bitsN <= 0x1d):
-            raise Exception("First part of bits should be in [0x03, 0x1d]")
-        bitsBase = bits & 0xffffff
-        if not (0x8000 <= bitsBase <= 0x7fffff):
-            raise Exception("Second part of bits should be in [0x8000, 0x7fffff]")
-        return bitsBase << (8 * (bitsN-3))
+        bitsBase = bits & 0x7fffff
+        if bitsN <= 3:
+            target = bitsBase >> (8 * (3-bitsN))
+        else:
+            target = bitsBase << (8 * (bitsN-3))
+        if target != 0 and bits & 0x800000 != 0:
+            # Bit number 24 (0x800000) represents the sign of N
+            raise Exception("target cannot be negative")
+        if (target != 0 and
+                (bitsN > 34 or
+                 (bitsN > 33 and bitsBase > 0xff) or
+                 (bitsN > 32 and bitsBase > 0xffff))):
+            raise Exception("target has overflown")
+        return target
 
     @classmethod
     def target_to_bits(cls, target: int) -> int:
+        # arith_uint256::GetCompact in Bitcoin Core
+        # see https://github.com/bitcoin/bitcoin/blob/7fcf53f7b4524572d1d0c9a5fdc388e87eb02416/src/arith_uint256.cpp#L223
         c = target.to_bytes(length=32, byteorder='big')
-        c = c[1:]  # FIXME why is this done unconditionally?
-        while c[0] == 0 and len(c) > 3:
+        bitsN = len(c)
+        while bitsN > 0 and c[0] == 0:
             c = c[1:]
-        bitsN, bitsBase = len(c), int.from_bytes(c[:3], byteorder='big')
+            bitsN -= 1
+            if len(c) < 3:
+                c += b'\x00'
+        bitsBase = int.from_bytes(c[:3], byteorder='big')
         if bitsBase >= 0x800000:
             bitsN += 1
             bitsBase >>= 8

electrum/tests/test_blockchain.py

@@ -384,6 +384,47 @@ class TestBlockchain(ElectrumTestCase):
         self.assertEqual([chain_u], self.get_chains_that_contain_header_helper(self.HEADERS['O']))
         self.assertEqual([chain_z, chain_l], self.get_chains_that_contain_header_helper(self.HEADERS['I']))
 
+    def test_target_to_bits(self):
+        # https://github.com/bitcoin/bitcoin/blob/7fcf53f7b4524572d1d0c9a5fdc388e87eb02416/src/arith_uint256.h#L269
+        self.assertEqual(0x05123456, Blockchain.target_to_bits(0x1234560000))
+        self.assertEqual(0x0600c0de, Blockchain.target_to_bits(0xc0de000000))
+
+        # tests from https://github.com/bitcoin/bitcoin/blob/a7d17daa5cd8bf6398d5f8d7e77290009407d6ea/src/test/arith_uint256_tests.cpp#L411
+        tuples = (
+            (0, 0x0000000000000000000000000000000000000000000000000000000000000000, 0),
+            (0x00123456, 0x0000000000000000000000000000000000000000000000000000000000000000, 0),
+            (0x01003456, 0x0000000000000000000000000000000000000000000000000000000000000000, 0),
+            (0x02000056, 0x0000000000000000000000000000000000000000000000000000000000000000, 0),
+            (0x03000000, 0x0000000000000000000000000000000000000000000000000000000000000000, 0),
+            (0x04000000, 0x0000000000000000000000000000000000000000000000000000000000000000, 0),
+            (0x00923456, 0x0000000000000000000000000000000000000000000000000000000000000000, 0),
+            (0x01803456, 0x0000000000000000000000000000000000000000000000000000000000000000, 0),
+            (0x02800056, 0x0000000000000000000000000000000000000000000000000000000000000000, 0),
+            (0x03800000, 0x0000000000000000000000000000000000000000000000000000000000000000, 0),
+            (0x04800000, 0x0000000000000000000000000000000000000000000000000000000000000000, 0),
+            (0x01123456, 0x0000000000000000000000000000000000000000000000000000000000000012, 0x01120000),
+            (0x02123456, 0x0000000000000000000000000000000000000000000000000000000000001234, 0x02123400),
+            (0x03123456, 0x0000000000000000000000000000000000000000000000000000000000123456, 0x03123456),
+            (0x04123456, 0x0000000000000000000000000000000000000000000000000000000012345600, 0x04123456),
+            (0x05009234, 0x0000000000000000000000000000000000000000000000000000000092340000, 0x05009234),
+            (0x20123456, 0x1234560000000000000000000000000000000000000000000000000000000000, 0x20123456),
+        )
+        for nbits1, target, nbits2 in tuples:
+            with self.subTest(original_compact_nbits=nbits1.to_bytes(length=4, byteorder="big").hex()):
+                num = Blockchain.bits_to_target(nbits1)
+                self.assertEqual(target, num)
+                self.assertEqual(nbits2, Blockchain.target_to_bits(num))
+
+        # Make sure that we don't generate compacts with the 0x00800000 bit set
+        self.assertEqual(0x02008000, Blockchain.target_to_bits(0x80))
+
+        with self.assertRaises(Exception):  # target cannot be negative
+            Blockchain.bits_to_target(0x01fedcba)
+        with self.assertRaises(Exception):  # target cannot be negative
+            Blockchain.bits_to_target(0x04923456)
+        with self.assertRaises(Exception):  # overflow
+            Blockchain.bits_to_target(0xff123456)
+
 
 class TestVerifyHeader(ElectrumTestCase):