test: refactor: various (de)serialization helpers cleanups/improvements #22257

pull theStack wants to merge 4 commits into bitcoin:master from theStack:202106-test-refactor-replace_manual_deser_with_fromtohex changing 33 files +396 −261
  1. theStack commented at 10:53 pm on June 15, 2021: member

    There are still many functional tests that perform conversions from a hex-string to a message object (deserialization) manually. This PR identifies all those instances and replaces them with a newly introduced helper tx_from_hex.

    Instances were found via

    • git grep "deserialize.*BytesIO"

    and some of them manually, when it were not one-liners.

    Further, the helper ToHex was removed and simply replaced by .serialize().hex(), since now both variants are in use (sometimes even within the same test) and using the helper doesn’t really have an advantage in readability. (see discussion #22257 (review))

  2. theStack force-pushed on Jun 15, 2021
  3. theStack force-pushed on Jun 15, 2021
  4. DrahtBot added the label Tests on Jun 16, 2021
  5. klementtan commented at 1:21 am on June 16, 2021: contributor

    Code review ACK 82449a55f16b7a2897ccef999c3f807ec1a1e591

    Verified that .serialize().hex() to ToHex(obj) and obj.deserialize(BytesIO(hex_str_to_bytes(hex_string))) to FromHex(obj, hex_string) are the only changes made.

    Also manually checked with rg '.deserialize' -g '*.py' and rg '\.hex\(\)' -g '*.py that there are no other places to refactor.

  6. fanquake commented at 5:07 am on June 16, 2021: member
    Concept ACK
  7. practicalswift commented at 5:28 am on June 16, 2021: contributor
    Concept ACK: nice cleanup!
  8. in test/functional/feature_cltv.py:170 in 82449a55f1 outdated 
    166@@ -166,7 +167,7 @@ def run_test(self):
167                     'allowed': False,
168                     'reject-reason': expected_cltv_reject_reason,
169                 }],
170-                self.nodes[0].testmempoolaccept(rawtxs=[spendtx.serialize().hex()], maxfeerate=0),
171+                self.nodes[0].testmempoolaccept(rawtxs=[ToHex(spendtx)], maxfeerate=0),
    MarcoFalke commented at 7:00 am on June 16, 2021:

    Is there a reason to do this replacement? Now that python supports bin->hex by just appending .hex(), the helper isn’t really needed anymore.

    Personally I think both versions are fine and there isn’t really a way to “protect” new code from using one or the other.

    theStack commented at 7:41 am on June 16, 2021:

    Agree that the gain can be questioned – I was also hesitating about this replacement, but then thought symmetry (i.e. why get rid of most deserialize() calls, but let the serialize() calls in) and the fact that it’s shorter may be worth it.

    Some tests already imported and used ToHex, but at some places still called serialize().hex(). At least mixing within the same test should be avoided IMHO, as it only creates confusion.

    Maybe in the long-term, removing ToHex completely would be an alternative?

    MarcoFalke commented at 8:29 am on June 16, 2021:
    Then my preference would be to remove ToHex to avoid future pull requests changing it back (or forth)
  9. MarcoFalke commented at 7:02 am on June 16, 2021: member
    Not sure about the long term goal of the ToHex replacement.
  10. DrahtBot commented at 7:20 am on June 16, 2021: 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:

    • #22310 (test: Add functional test for replacement relay fee check by ariard)
    • #20892 (tests: Run both descriptor and legacy tests within a single test invocation by achow101)
    • #18919 (test: Add gettransaction test for “coin-join” tx by MarcoFalke)

    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.

  11. theStack force-pushed on Jun 16, 2021
  12. theStack renamed this:
    test: refactor: use `{From,To}Hex` helpers for msg (de)serialization from/to hex
    test: refactor: use `FromHex` helper for msg serialization from hex, remove `ToHex` helper
    on Jun 16, 2021
  13. theStack commented at 2:21 pm on June 16, 2021: member
    Force-pushed with a change in the second commit: ToHex is removed and all instances are replaced with .serialize().hex(), as suggested in this discussion by MarcoFalke. Also adapted PR title and description accordingly.
  14. in test/functional/test_framework/messages.py:198 in 6665fec39e outdated 
    194@@ -195,10 +195,6 @@ def FromHex(obj, hex_string):
195     obj.deserialize(BytesIO(hex_str_to_bytes(hex_string)))
196     return obj
197 
198-# Convert a binary-serializable object to hex (eg for submission via RPC)
    practicalswift commented at 8:45 pm on June 16, 2021:
    Non-blocking suggestion: Could add a comment to FromHex describing that the recommended inverse is obj.serialize().hex() (instead of a hypothetical ToHex) :)
    theStack commented at 3:52 pm on June 17, 2021:
    Good idea, I added a commit with your suggestion!
  15. brunoerg approved
  16. brunoerg commented at 12:05 pm on June 17, 2021: member

    tACK 6665fec39e8e42e05a7b602a24f0f6d312cf3991

    Nice improv!

    Ran test runner and every changed test several times on MacOS 11.3.

  17. theStack force-pushed on Jun 17, 2021
  18. theStack force-pushed on Jun 17, 2021
  19. theStack commented at 3:57 pm on June 17, 2021: member
    Added a commit https://github.com/bitcoin/bitcoin/commit/18b780b6ce2b42245f067e922af5552bd8931be3 to improve the documentation of FromHex by mentioning the recommended reverse operation (as suggested by practicalswift) and changing to docstring format.
  20. practicalswift commented at 7:59 pm on June 17, 2021: contributor
    cr ACK 18b780b6ce2b42245f067e922af5552bd8931be3
  21. DrahtBot added the label Needs rebase on Jun 19, 2021
  22. theStack force-pushed on Jun 19, 2021
  23. theStack commented at 11:04 am on June 19, 2021: member
    Rebased on master.
  24. DrahtBot removed the label Needs rebase on Jun 19, 2021
  25. in test/functional/mempool_accept.py:93 in cd4e0e18df outdated 
    89@@ -91,8 +90,7 @@ def run_test(self):
90             inputs=[{"txid": txid_in_block, "vout": 0, "sequence": BIP125_SEQUENCE_NUMBER}],  # RBF is used later
91             outputs=[{node.getnewaddress(): Decimal('0.3') - fee}],
92         ))['hex']
93-        tx = CTransaction()
94-        tx.deserialize(BytesIO(hex_str_to_bytes(raw_tx_0)))
95+        tx = FromHex(CTransaction(), raw_tx_0)
    MarcoFalke commented at 4:58 pm on June 19, 2021:
    cd4e0e18dfcda92b0271607a81cf452d7a6bb559: I am wondering if it makes sense to introduce a tx_from_hex(str) helper, as CTransction is the most common type to be deserialized from hex. This would also solve the ambiguity and verbosity around passing the first parameter to FromHex.
    theStack commented at 8:14 pm on June 19, 2021:
    Good idea, done.
  26. theStack commented at 8:17 pm on June 19, 2021: member
    Added another commit to introduce a tx_from_hex helper, as suggested by MarcoFalke (https://github.com/bitcoin/bitcoin/pull/22257#discussion_r654817481). Tempted to squash this with the first commit to minimize reviewing burden, OTOH both replacing code with an existing helper and introducing a new one seems a bit much for a single commit. Opinions?
  27. theStack force-pushed on Jun 19, 2021
  28. theStack force-pushed on Jun 19, 2021
  29. MarcoFalke commented at 9:31 am on June 20, 2021: member
    I have a slight preference to squash to avoid repeatedly touching the same lines
  30. theStack force-pushed on Jun 20, 2021
  31. theStack renamed this:
    test: refactor: use `FromHex` helper for msg serialization from hex, remove `ToHex` helper
    test: refactor: various (de)serialization helpers cleanups/improvements
    on Jun 20, 2021
  32. theStack commented at 12:19 pm on June 20, 2021: member

    Restructured everything a bit (sorry for confusing to all who already reviewed!):

    • the first commit immediately introduces a helper tx_from_hex, which is used for instances where deserialization was done manually, and also for instances which used FromHex already
    • second commit gets rid of ToHex
    • third commit renames FromHex to from_hex via scripted diff (to follow coding guidelines / PEP8 naming)
    • fourth commit adds from_hex documentation
  33. MarcoFalke commented at 9:25 am on June 21, 2021: member

    I’ve rewritten the scripted diff and it seems there is a silent merge conflict?

     0$ grd 8ed7dbd386562368e864fce23722676cd3637854 HEAD
 11:  8c127a86be = 1:  8c127a86be test: introduce `tx_from_hex` helper for tx deserialization
 22:  4223f18a8c = 2:  4223f18a8c test: remove `ToHex` helper, use .serialize().hex() instead
 33:  8ed7dbd386 ! 3:  662223f4ba scripted-diff: test: rename `FromHex` to `from_hex`
 4    @@ Commit message
 5         scripted-diff: test: rename `FromHex` to `from_hex`
 6     
 7         -BEGIN VERIFY SCRIPT-
 8    -    git ls-files -- test/functional | xargs sed -i 's/FromHex/from_hex/g'
 9    +    sed -i 's/\<FromHex\>/from_hex/g' $(git grep -l FromHex)
10         -END VERIFY SCRIPT-
11     
12      ## test/functional/feature_utxo_set_hash.py ##
  34. MarcoFalke commented at 9:25 am on June 21, 2021: member

    review ACK 446fd1792ae3e7d0917df771bc0ccfa7c3c3b8f6 🏵

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK 446fd1792ae3e7d0917df771bc0ccfa7c3c3b8f6 🏵
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUiqPAv8DOhtbQZoGRJyrizrhOHso5pM2dbArc/E6N3UEPT/FpHLBHuZiEvgbKeA
 8CmPU4f9QQKrW4pLMIdF7i/R94Lwo7pNulA0697LyZMSypRSMZiLIq6WBSaY7U6+o
 99Ao7AOf9gzBBhqs9qWeFrk0dSK8uaEcPMhe+poDOvzr+8BGyf49amhySCUFo9CRQ
10gsjhPVt2/cEfX7Chg+iUVotW35t5lLid8NUBpelumapeeflZ0943cguv6QSy6IZ8
11shtdjptvn/VFMi8nhqeBPTspKrQfY29cF3h0DBjPxFqWdERgdBhdNetG3wIGHTzQ
12Hg/yf1FfsBs7Sk9MFDE+HW+u1i8LhmzCFkP3rRXI8qhY7RGCWKe4NFSJUJLi+4JB
13CpPtsPSjJkomj6rqec34GqWDuGzkxOqpWM5j++xpCp46pzA4zkdtICecnp3VCcYv
148vGpeU2K6x2tKX7ZiTMSKZ3m7XqoF2x9RcP/NP2kWDVzdA4f184VXoJVT0DHRF5Z
15bpeDtV39
16=U71S
17-----END PGP SIGNATURE-----

    Timestamp of file with hash a3b9802ea42e8e0ba75b10e75c2b16701bc96227df1740b76ceb4b2ee5a11dda -

  35. test: introduce `tx_from_hex` helper for tx deserialization 
    `FromHex` is mostly used for transactions, so we introduce a
shortcut `tx_from_hex` for `FromHex(CTransaction, hex_str)`.
    2ce7b47958
  36. test: remove `ToHex` helper, use .serialize().hex() instead a79396fe5f
  37. scripted-diff: test: rename `FromHex` to `from_hex` 
    -BEGIN VERIFY SCRIPT-
sed -i 's/\<FromHex\>/from_hex/g' $(git grep -l FromHex)
-END VERIFY SCRIPT-

Co-authored-by: MarcoFalke <falke.marco@gmail.com>
    1914054208
  38. test: doc: improve doc for `from_hex` helper (mention `to_hex` alternative) bdb8b9a347
  39. theStack force-pushed on Jun 21, 2021
  40. theStack commented at 12:50 pm on June 21, 2021: member

    I’ve rewritten the scripted diff and it seems there is a silent merge conflict?

     0$ grd 8ed7dbd386562368e864fce23722676cd3637854 HEAD
 11:  8c127a86be = 1:  8c127a86be test: introduce `tx_from_hex` helper for tx deserialization
 22:  4223f18a8c = 2:  4223f18a8c test: remove `ToHex` helper, use .serialize().hex() instead
 33:  8ed7dbd386 ! 3:  662223f4ba scripted-diff: test: rename `FromHex` to `from_hex`
 4    @@ Commit message
 5         scripted-diff: test: rename `FromHex` to `from_hex`
 6     
 7         -BEGIN VERIFY SCRIPT-
 8    -    git ls-files -- test/functional | xargs sed -i 's/FromHex/from_hex/g'
 9    +    sed -i 's/\<FromHex\>/from_hex/g' $(git grep -l FromHex)
10         -END VERIFY SCRIPT-
11     
12      ## test/functional/feature_utxo_set_hash.py ##

    Oops, I completely forgot that we also have Python scripts outside of test/functional that use helpers from the test framework and are thus affected by the changes (namely contrib/signet/miner).

    Force-pushed with changes including it (affects all but the last commit), used your scripted-diff and add you as co-author.

    (Not directly connected to this PR, but it would probably make sense to call the signet mining tool in one of the functional tests, to let it run through CI?)

  41. MarcoFalke commented at 7:04 am on June 23, 2021: member

    review re-ACK bdb8b9a347e68f80a2e8d44ce5590a2e8214b6bb 😁

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review re-ACK bdb8b9a347e68f80a2e8d44ce5590a2e8214b6bb 😁
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjrswv/bCHcA6Zpk9Gv9fkRzBtmSfbuLE2ifDg+RGN2prKClfrlim2Kx9LhjTK+
 8BoeGGWwMAUdg+SHdNi5RMhmiRrSB+4JMtxw3TY6COBjP/QYdmzyySle/xNGwAoov
 90CGZsEZaqiC5QPFJe6pLZJMRJET8iJR9gLaNNkWbEkUllqkjdvaIuGCJjKC4k1ld
10epdUxh+yxg5U/FPMgSR5jPew1GBMpb6XPk0YSJ2yPimCBlSeaIH9X1XlYNix1WPv
11tn6rCN4ZUkufPqb8x0R//y/x3T0lMOgjOsnODR5BZtUdV0P/GK7ZTRa9zBtz/Xa4
12D7somqqh5TKOBvNJounyLS1FO3zXBoE6Kbr5xZQinob7fpTlEf9nvS9yWNtE+tyZ
13em+74lyfdHMiBBWmlEIIiHaOrrG6pPOXQ6zn/GSEkrlHjQLtLJ2a+YoAzO6hc6Af
14RUA/NKPHz8NPHcOzpbtPTs+sYloLrtbp8O5dUsgyZ7JWTT0hAEwIpahQsEoi9j4b
15lnHvly/x
16=rKMS
17-----END PGP SIGNATURE-----

    Timestamp of file with hash 1cd7510533208f2380d21b0afee7343c273d929ac88f1e044e3c4c7724daab3b -

  42. MarcoFalke commented at 7:05 am on June 23, 2021: member

    (Not directly connected to this PR, but it would probably make sense to call the signet mining tool in one of the functional tests, to let it run through CI?)

    Agree

  43. MarcoFalke merged this on Jun 24, 2021
  44. MarcoFalke closed this on Jun 24, 2021
  45. sidhujag referenced this in commit d8c5c3da56 on Jun 24, 2021
  46. theStack deleted the branch on Jul 31, 2021
  47. gwillen referenced this in commit 083e08cf82 on Jun 1, 2022
  48. DrahtBot locked this on Aug 16, 2022


theStack klementtan fanquake practicalswift MarcoFalke DrahtBot brunoerg

Labels
Tests

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-30 00:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me