B017 `assertRaises(Exception)` and `pytest.raises(Exception)` should be considered evil. They can lead to your test passing even if the code being tested is never executed due to a typo. Assert for a more specific exception (builtin or custom), or use `assertRaisesRegex` (if using `assertRaises`), or add the `match` keyword argument (if using `pytest.raises`), or use the context manager form with a target.
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
```
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...
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
```
Saw a deadlock near a swap_with_parent(), could not reproduce.
get_branch_size() and get_parent_heights() could have been the culprits as
they take locks in differing orders and are called from gui/network threads.
Previously this function would not switch to a different chain if the
current chain contained the preferred block. This was not the intended
behaviour: if there is a *stronger* chain that *also* contains the
preferred block, we should jump to that.
Note that with this commit there will now always be a preferred block
(defaults to genesis). Previously, it might seem that often there was none,
but actually in practice if the user used the GUI context menu to switch
servers even once, there was one (usually genesis).
Hence, with the old code, if an attacker mined a single header which
then got reorged, auto_connect clients which were connected to the
attacker's server would never switch servers (jump chains) even
without the user explicitly configuring preference for the stale branch.
after a reorg, in a many fork/orphan chains scenario,
we would sometimes not undo SPV for enough blocks
functions in blockchain.py somewhat based on kyuupichan/bitcoinX@5126bd15ef0c9ba36e17a455513452ebed7b2328
"target" is a 256 bit int, but the "bits" field in the block headers
that is used to represent target is only 32 bits.
We were checking PoW against the untruncated target value, which is a
slightly larger value than the one that can actually be represented,
and hence we would have accepted a slightly lower difficulty chain
than what the consensus requires.
If auto_connect is enabled, allow jumping between forks too.
(Previously auto_connect was only switching servers on a given fork,
not across forks)
If there is a preferred fork set, jump to that (and stay);
if there isn't, always jump to the longest fork.
Sander van Grieken
SomberNight
bitromortac
Janus
Francisco J. Hernandez Heras
ThomasV