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
```
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.
The few other cases that used SimpleConfig.get_instance() now
either get passed a config instance, or they try to get a reference
to something else that has a reference to a config.
(see lnsweep, qt/qrcodewidget, qt/qrtextedit)
The function Blockchain.verify_header was previously not covered by tests
at all. Even removing all the tests in it would still make the unit tests
pass. This change adds tests for this important (!) function.
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
Sander van Grieken
SomberNight
Daniel Kraft