use consistent bytes <-> hex-string conversions in functional test framework #22605

1. 
   theStack commented at 2:53 pm on August 2, 2021: member

   Right now in the functional test framework, conversions from hex-string to bytes and vice-versa are done in different ways:

   • convert bytes_obj to hex-string: binascii.hexlify(bytes_obj), bytes_obj.hex()
   • convert hex_str to bytes object: binascii.unhexlify(hex_str), binascii.a2b_hex(hex_str), bytes.fromhex(hex_str)

   For consistency reasons, only the built-in way of using bytes (class) methods should be used (see also MarcoFalke’s comment #22593 (comment)):

   • bytes.fromhex() (https://docs.python.org/3.7/library/stdtypes.html#bytes.fromhex)
   • bytes.hex() (https://docs.python.org/3.7/library/stdtypes.html#bytes.hex)

   This seems to be the most natural and modern way to do it, without a need to import any libraries, and the other ones using binascii should be replaced by those. As an example, see PR #22593 which replaced a helper hex_str_to_bytes (which in turn used binascii.unhexlify) with bytes.fromhex(). For quickly identifying instances the command git grep binascii can be used, though one would need to check that there is maybe uses of binascii that are still legit.

   Useful skills:

   • Python 3 basic skills, knowledge of fundamental data types string and bytes

   Want to work on this issue?

   For guidance on contributing, please read CONTRIBUTING.md before opening your pull request.

2. 
   theStack commented at 2:54 pm on August 2, 2021: member

   Would be nice if a maintainer could tag this as “good first issue”
3. MarcoFalke added the label good first issue on Aug 2, 2021
4. 
   Zero-1729 commented at 5:35 pm on August 2, 2021: contributor

   @theStack I’d like to take a shot at this.
5. 
   MarcoFalke commented at 12:30 pm on August 3, 2021: member

   @Zero-1729 Looks like this was already picked up in #22617
6. 
   Zero-1729 commented at 1:56 pm on August 3, 2021: contributor

   Wow, didn’t expect to see this 😂 (I assumed making my intentions public here was a good safeguard).

   I am not sure what to do with this branch now. I might as well breakdown the changes here:

   Removed all instances of binascii methods in the test framework. I also ran all the affected functional tests to ensure nothing broke.

   This commit 91b5350119a160df3eaa4e6eaff4e9ede97a51e6 takes out all binascii calls and replaces them with bytes.hex and bytes.fromhex where appropriate. Certain changes to some files are based on the following realisation/assumption:

   0bytes.hex(data) == binascii.hexlify(data).decode()
   1bytes.hex(data).encode() == binascii.hexlify(data)
   

   If the above is incorrect, the changes based on it can simply be discarded.

   P.S.: For anyone interested, there is also a parent branch with all these changes and more, intended to get rid of all instances of binascii in the repo, and making git grep binascii yield nothing.

7. 
   Saviour1001 commented at 6:57 pm on August 3, 2021: none

   @Zero-1729 I tested your branch on my end and it works correctly. I would suggest to close #22617 and just consider https://github.com/bitcoin/bitcoin/commit/91b5350119a160df3eaa4e6eaff4e9ede97a51e6 for fixing this issue.
8. 
   Zero-1729 commented at 8:11 pm on August 3, 2021: contributor

   @Saviour1001 thanks for testing! I just opened the PR.
9. MarcoFalke closed this on Aug 5, 2021

   ---

10. 
    MarcoFalke commented at 10:19 am on August 5, 2021: member

    Not fixed completely: #22619 (comment)
11. MarcoFalke reopened this on Aug 5, 2021

    ---

12. 
    Zero-1729 commented at 11:34 am on August 5, 2021: contributor

    @MarcoFalke Thanks for the edit! @theStack The last usage of binascii is in the following Python code comment in src/test/serfloat_tests.cpp. Do I also tackle that in the follow-up, so git grep binascii no longer yields anything?
13. fanquake deleted a comment on Aug 5, 2021
14. fanquake deleted a comment on Aug 5, 2021
15. 
    theStack commented at 10:15 pm on August 6, 2021: member

    @theStack The last usage of binascii is in the following Python code comment in src/test/serfloat_tests.cpp. Do I also tackle that in the follow-up, so git grep binascii no longer yields anything?

    Yes, I’d suggest to also tackle the Python code comment. Happy to review within the next days :)

16. 
    Zero-1729 commented at 1:12 pm on August 8, 2021: contributor

    Just pushed the changes.
17. MarcoFalke closed this on Aug 21, 2021

    ---

18. DrahtBot locked this on Aug 21, 2022

Contributors
theStack Zero-1729 MarcoFalke Saviour1001

Labels
good first issue