Skip to content

Conversation

Zero-1729
Copy link
Contributor

@Zero-1729 Zero-1729 commented Aug 5, 2021

This PR removes the remaining binascii method calls outside test/functional and test_framework, as pointed out here #22619 (review).

Follow-up to #22593 and #22619
Closes #22605

@Zero-1729
Copy link
Contributor Author

Diff from 3a2ad3b to 33b8a31: Updated Python2 code sample to Python3 and removed binascii.hexlify call.

Now git grep binascii no longer yields any result.

Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code-review ACK 33b8a31

It's nice to see that with this PR, there is now only one method left for hex-string/bytes conversions in the functional tests and contrib scripts. Special kudos for also replacing the Python code comment in serfloat_tests.cpp. I tested this directly in the Python interpreter and it works as expected, i.e. the statement yields True.

@Zero-1729 Zero-1729 force-pushed the follow-ups-to-22605 branch from 33b8a31 to b95d5ad Compare August 9, 2021 01:28
@Zero-1729
Copy link
Contributor Author

Zero-1729 commented Aug 9, 2021

Diff from 33b8a31 to b95d5ad: addressed comment above.

Thanks @theStack for the background reference regarding contrib/zmq/zmq_sub.py!

Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-ACK b95d5ad 💯

@josibake
Copy link
Member

ACK b95d5ad

+1 for consistency! did a code review and everything looks good. also ran the standalone scripts where possible and tested a few individual functions (e.g python3 -c 'from rpcauth import generate_salt; print(generate_salt(10))'). copied the python snippet from the comment and ran it, everything working as expected

@laanwj laanwj changed the title refactor: follow-ups to 22619 refactor: Replace remaining binascii method calls Aug 16, 2021
@laanwj
Copy link
Member

laanwj commented Aug 16, 2021

Concept ACK. Let's be sure that this covers all and is the last PR in the series, it's been dragging on for a while.

@Zero-1729
Copy link
Contributor Author

Zero-1729 commented Aug 16, 2021

I think this is the last PR in this series. No other instances of binascii remain. Unless @theStack has any other suggestions that need to be added?

@theStack
Copy link
Contributor

Let's be sure that this covers all and is the last PR in the series, it's been dragging on for a while.

Agreed!

I think this is the last PR in this series. No other instances of binascii remain. Unless @theStack has any other suggestions that need to be added?

Doing a search in test/function via "git grep -i hex", I unfortunately found yet another variant of converting bin-to-hex that should be fixed (I verified via git grep hex_codec that this is the only place it is used):

diff --git a/test/functional/test_framework/messages.py b/test/functional/test_framework/messages.py
index 6e57107f8..65d90f844 100755
--- a/test/functional/test_framework/messages.py
+++ b/test/functional/test_framework/messages.py
@@ -19,7 +19,6 @@ Classes use __slots__ to ensure extraneous attributes aren't accidentally added
 by tests, compromising their intended effect.
 """
 from base64 import b32decode, b32encode
-from codecs import encode
 import copy
 import hashlib
 from io import BytesIO
@@ -681,7 +680,7 @@ class CBlockHeader:
             r += struct.pack("<I", self.nBits)
             r += struct.pack("<I", self.nNonce)
             self.sha256 = uint256_from_str(hash256(r))
-            self.hash = encode(hash256(r)[::-1], 'hex_codec').decode('ascii')
+            self.hash = hash256(r)[::-1].hex()

     def rehash(self):
         self.sha256 = None

This one is not using binascii (thus not matching the PR title), but should still be tackled in this PR for the reasons laanwj mentioned. After this I'm pretty sure we have them all and #22605 can be closed.

@Zero-1729 Zero-1729 force-pushed the follow-ups-to-22605 branch from b95d5ad to 021daed Compare August 16, 2021 18:38
@Zero-1729
Copy link
Contributor Author

Zero-1729 commented Aug 16, 2021

Nice catch! Updated.

Diff b95d5ad -> 021daed:

a/test/functional/test_framework/messages.py

    - from codecs import encode
    ...

    -            self.hash = encode(hash256(r)[::-1], 'hex_codec').decode('ascii')
    +            self.hash = hash256(r)[::-1].hex()

    ...

Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-ACK 021daed

@fanquake
Copy link
Member

cc @practicalswift @tryphe given you both reviewed the parent PR.

@josibake
Copy link
Member

re-ACK 021daed

@maflcko maflcko merged commit f5a406f into bitcoin:master Aug 21, 2021
Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Posthumous ACK, modulo could have updated the Python example in src/test/serfloat_tests.cpp -- git grep binascii takes you there.

Edit: oops, I ran grep from /src instead of root, there are some other ones, idem for git grep hexlify. Maybe a follow-up if it matters.

@Zero-1729
Copy link
Contributor Author

Posthumous ACK, modulo could have updated the Python example in src/test/serfloat_tests.cpp -- git grep binascii takes you there.

This PR also tackled the Python code sample in src/test/serfloat_tests.cpp.

Edit: oops, I ran grep from /src instead of root, there are some other ones, idem for git grep hexlify. Maybe a follow-up if it matters.

Just pulled master locally, running both git grep binascii and git grep hexlify in the root shows nothing for me. Also tried it in src and got no results. @jonatack are you still getting any results after f5a406f?

@jonatack
Copy link
Member

jonatack commented Aug 21, 2021

@Zero-1729 You're right! I grepped from the PR branch (edit: nope, another PR branch 😅) sorry for the noise.

@Zero-1729
Copy link
Contributor Author

@jonatack No worries, happy to help! 🙏🏾

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 22, 2021
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

8 participants