From 6033471853d12dd9df59d46075b023eb40805851 Mon Sep 17 00:00:00 2001 From: SomberNight Date: Sat, 13 Nov 2021 03:02:31 +0100 Subject: [PATCH 1/3] blockchain: clarify MAX_TARGET by referencing bitcoin core source change is no-op as the compact nBits form of both values are equal, that is: ``` >>> from electrum.blockchain import Blockchain >>> MAX_TARGET1 = 0x00000000FFFF0000000000000000000000000000000000000000000000000000 >>> MAX_TARGET2 = 0x00000000ffffffffffffffffffffffffffffffffffffffffffffffffffffffff >>> Blockchain.bits_to_target(Blockchain.target_to_bits(MAX_TARGET2)) == Blockchain.bits_to_target(Blockchain.target_to_bits(MAX_TARGET1)) True ``` --- electrum/blockchain.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/electrum/blockchain.py b/electrum/blockchain.py index f911d4b1e..9e9944902 100644 --- a/electrum/blockchain.py +++ b/electrum/blockchain.py @@ -37,7 +37,9 @@ from .logging import get_logger, Logger _logger = get_logger(__name__) HEADER_SIZE = 80 # bytes -MAX_TARGET = 0x00000000FFFF0000000000000000000000000000000000000000000000000000 + +# see https://github.com/bitcoin/bitcoin/blob/feedb9c84e72e4fff489810a2bbeec09bcda5763/src/chainparams.cpp#L76 +MAX_TARGET = 0x00000000ffffffffffffffffffffffffffffffffffffffffffffffffffffffff # compact: 0x1d00ffff class MissingHeader(Exception): From ee10e8e4d305a369ff8e3b87507dd6d6f344cb9b Mon Sep 17 00:00:00 2001 From: SomberNight Date: Sat, 13 Nov 2021 03:07:36 +0100 Subject: [PATCH 2/3] blockchain: bits_to_target: small clean-up note: why is the first byte cut unconditionally? what if it's non-zero? Maybe the intention of cutting the first two chars in the hex case intended to cut a "0x" prefix?? But there was no such prefix with the given format string... --- electrum/blockchain.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/electrum/blockchain.py b/electrum/blockchain.py index 9e9944902..b850b04b3 100644 --- a/electrum/blockchain.py +++ b/electrum/blockchain.py @@ -552,10 +552,11 @@ class Blockchain(Logger): @classmethod def target_to_bits(cls, target: int) -> int: - c = ("%064x" % target)[2:] - while c[:2] == '00' and len(c) > 6: - c = c[2:] - bitsN, bitsBase = len(c) // 2, int.from_bytes(bfh(c[:6]), byteorder='big') + 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: + c = c[1:] + bitsN, bitsBase = len(c), int.from_bytes(c[:3], byteorder='big') if bitsBase >= 0x800000: bitsN += 1 bitsBase >>= 8 From 7e2d9c48d12b99e4e4037c144661cd9d6576b031 Mon Sep 17 00:00:00 2001 From: SomberNight Date: Sat, 13 Nov 2021 04:31:08 +0100 Subject: [PATCH 3/3] 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 "", line 1, in 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 | 34 ++++++++++++++++++------- electrum/tests/test_blockchain.py | 41 +++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 9 deletions(-) diff --git a/electrum/blockchain.py b/electrum/blockchain.py index b850b04b3..43564e353 100644 --- a/electrum/blockchain.py +++ b/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 diff --git a/electrum/tests/test_blockchain.py b/electrum/tests/test_blockchain.py index 17d0d836f..a58c688fe 100644 --- a/electrum/tests/test_blockchain.py +++ b/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):