It is unsigned in Bitcoin Core, so the tests should match it:
Large positive values for the block height are too difficult to hit in tests, but it still seems fine to fix this.
The bug was introduced when the code was written in 6ccc8fc067bf516cda7bc5d7d721945be5ac2003.
(Lowercase i means signed, see https://docs.python.org/3/library/struct.html#format-characters)
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | epiccurious, fjahr |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
55@@ -58,7 +56,7 @@ def test_muhash_implementation(self):
56 continue
57
58 data = COutPoint(int(tx.rehash(), 16), n).serialize()
59- data += struct.pack("<i", height * 2 + coinbase)
60+ data += (height * 2 + coinbase).to_bytes(4, "little")
I’m just looking around, I’m not a Bitcoin core expert.
My understanding is that the unsigned little endian is <I, wouldn’t it make sense to change as little as possible here:
0 data += struct.pack("<I", height * 2 + coinbase)
or if we want to mimic the C++ code:
0 data = struct.pack('<I', (height << 1) + coinbase)
55@@ -58,7 +56,7 @@ def test_muhash_implementation(self):
56 continue
57
58 data = COutPoint(int(tx.rehash(), 16), n).serialize()
59- data += struct.pack("<i", height * 2 + coinbase)
