test: Remove confusing hash256 function in util #16598

pull elichai wants to merge 1 commits into bitcoin:master from elichai:2019-08-rename-hash256 changing 2 files +7 −14

elichai commented at 7:01 PM on August 13, 2019: contributor

Right now there are two hash256(bytes) in the test framework: first: https://github.com/bitcoin/bitcoin/blob/master/test/functional/test_framework/util.py#L186 second: https://github.com/bitcoin/bitcoin/blob/master/test/functional/test_framework/messages.py#L60

While they have the same name they're actually doing different things, one just does a sha256d and the other sha256d and reverses the bytes. so I renamed the second one to be hash256_reversed to signify that it's hash256 reversed.
MarcoFalke renamed this:
~~Renaming confusing hash256 function name in the tests~~
test: Renaming confusing hash256 function name
on Aug 13, 2019
in test/functional/test_framework/util.py:186 in 2aafb5bc9e outdated
```
 182 | @@ -183,7 +183,7 @@ def check_json_precision():
 183 |  def count_bytes(hex_string):
 184 |      return len(bytearray.fromhex(hex_string))
 185 |  
 186 | -def hash256(byte_str):
 187 | +def hash256r(byte_str):
```
MarcoFalke commented at 7:05 PM on August 13, 2019:

wouldn't mind being extra verbose here, since it is only used twice in the code

def hash256_reversed(byte_str):
MarcoFalke commented at 7:07 PM on August 13, 2019:

Or maybe just move it to the zmq test, as that is the only place where it is used
in test/functional/test_framework/util.py:189 in 2aafb5bc9e outdated
```
 182 | @@ -183,7 +183,7 @@ def check_json_precision():
 183 |  def count_bytes(hex_string):
 184 |      return len(bytearray.fromhex(hex_string))
 185 |  
 186 | -def hash256(byte_str):
 187 | +def hash256r(byte_str):
 188 |      sha256 = hashlib.sha256()
 189 |      sha256.update(byte_str)
 190 |      sha256d = hashlib.sha256()
```
MarcoFalke commented at 7:09 PM on August 13, 2019:

Looks like you can replace all of this code with hash256()[::-1]

elichai commented at 7:10 PM on August 13, 2019:

yeah but then there will be a circular dependencies. (messages already imports util, this will require util to also import messages)

But if I'll move this function to zmq tests then that's possible(or to messages?)
elichai force-pushed on Aug 13, 2019
in test/functional/test_framework/util.py:184 in 9b091b4d47 outdated
```
 182 | @@ -183,12 +183,6 @@ def check_json_precision():
 183 |  def count_bytes(hex_string):
 184 |      return len(bytearray.fromhex(hex_string))
 185 |  
```
MarcoFalke commented at 7:30 PM on August 13, 2019:

test/functional/test_framework/util.py:10:1: F401 'hashlib' imported but unused

elichai commented at 7:39 PM on August 13, 2019:

Thanks!
MarcoFalke renamed this:
~~test: Renaming confusing hash256 function name~~
test: Remove confusing hash256 function in util
on Aug 13, 2019
elichai force-pushed on Aug 13, 2019
Moved and renamed hash256 from util.py to zmq_interface.py afc0966d72
DrahtBot added the label Tests on Aug 13, 2019
MarcoFalke commented at 7:59 PM on August 13, 2019: member

unsigned ACK afc0966d725aeeb8842dc264bd48f0e9c41f6a34
mzumsande commented at 8:24 PM on August 13, 2019: member

This is silly, but the new name hash256_reversed scared me for a split second - my first association was hash inversion.
DrahtBot commented at 10:10 PM on August 13, 2019: member


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #16404 (qa: Test ZMQ notification after chain reorg by promag)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
fanquake approved
fanquake commented at 6:45 AM on August 14, 2019: member

ACK afc0966d725aeeb8842dc264bd48f0e9c41f6a34

Checked that interface_zmq.py is still passing. Also updated the PR text with the correct function name hash256r -> hash256_reversed
fanquake merged this on Aug 14, 2019
fanquake closed this on Aug 14, 2019
fanquake referenced this in commit 05ccbe9a29 on Aug 14, 2019
sidhujag referenced this in commit 38e973a087 on Aug 17, 2019

in test/functional/interface_zmq.py:106 in afc0966d72

 102 | @@ -103,7 +103,7 @@ def _zmq_test(self):
 103 |  
 104 |              # Should receive the generated raw block.
 105 |              block = self.rawblock.receive()
 106 | -            assert_equal(genhashes[x], hash256(block[:80]).hex())
 107 | +            assert_equal(genhashes[x], hash256_reversed(block[:80]).hex())

promag commented at 12:16 AM on August 19, 2019:

nit, could just inline.

promag commented at 12:16 AM on August 19, 2019: member

ACK
elichai deleted the branch on Aug 22, 2019
jasonbcox referenced this in commit e516d58d92 on Sep 3, 2020
vijaydasmp referenced this in commit 4e9648e3fe on Nov 21, 2021
MarcoFalke locked this on Dec 16, 2021

Contributors

Labels

test: Remove confusing hash256 function in util #16598

Conflicts