test: refactor: various (de)serialization helpers cleanups/improvements by theStack · Pull Request #22257 · bitcoin/bitcoin

theStack · 2021-06-15T22:53:06Z

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 (comment))

klementtan

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.

fanquake · 2021-06-16T05:07:26Z

Concept ACK

practicalswift · 2021-06-16T05:28:35Z

Concept ACK: nice cleanup!

maflcko

Not sure about the long term goal of the ToHex replacement.

test/functional/feature_cltv.py

DrahtBot · 2021-06-16T07:20:50Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

test: Add functional test for replacement relay fee check #22310 (test: Add functional test for replacement relay fee check by ariard)
tests: Run both descriptor and legacy tests within a single test invocation #20892 (tests: Run both descriptor and legacy tests within a single test invocation by achow101)
test: Add gettransaction test for "coin-join" tx #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.

theStack · 2021-06-16T14:21:12Z

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.

test/functional/test_framework/messages.py

brunoerg

tACK 6665fec39e8e42e05a7b602a24f0f6d312cf3991

Nice improv!

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

theStack · 2021-06-17T15:57:50Z

Added a commit 18b780b to improve the documentation of FromHex by mentioning the recommended reverse operation (as suggested by practicalswift) and changing to docstring format.

practicalswift · 2021-06-17T19:59:22Z

cr ACK 18b780b6ce2b42245f067e922af5552bd8931be3

theStack · 2021-06-19T11:04:33Z

Rebased on master.

test/functional/mempool_accept.py

theStack · 2021-06-19T20:17:21Z

Added another commit to introduce a tx_from_hex helper, as suggested by MarcoFalke (#22257 (comment)).
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?

maflcko · 2021-06-20T09:31:47Z

I have a slight preference to squash to avoid repeatedly touching the same lines

theStack · 2021-06-20T12:19:26Z

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

maflcko · 2021-06-21T09:25:02Z

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

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

maflcko · 2021-06-21T09:25:39Z

review ACK 446fd1792ae3e7d0917df771bc0ccfa7c3c3b8f6 🏵

Show signature and timestamp

Signature:

Timestamp of file with hash a3b9802ea42e8e0ba75b10e75c2b16701bc96227df1740b76ceb4b2ee5a11dda -

`FromHex` is mostly used for transactions, so we introduce a shortcut `tx_from_hex` for `FromHex(CTransaction, hex_str)`.

-BEGIN VERIFY SCRIPT- sed -i 's/\<FromHex\>/from_hex/g' $(git grep -l FromHex) -END VERIFY SCRIPT- Co-authored-by: MarcoFalke <[email protected]>

…ative)

theStack · 2021-06-21T12:50:49Z

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

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

maflcko · 2021-06-23T07:04:58Z

review re-ACK bdb8b9a 😁

Show signature and timestamp

Signature:

Timestamp of file with hash 1cd7510533208f2380d21b0afee7343c273d929ac88f1e044e3c4c7724daab3b -

maflcko · 2021-06-23T07:05:06Z

(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

…s clean…

Just as mechanical and annoying as it looks.

theStack force-pushed the 202106-test-refactor-replace_manual_deser_with_fromtohex branch 2 times, most recently from 3749742 to 82449a5 Compare June 15, 2021 23:07

DrahtBot added the Tests label Jun 16, 2021

klementtan reviewed Jun 16, 2021

View reviewed changes

maflcko reviewed Jun 16, 2021

View reviewed changes

test/functional/feature_cltv.py Outdated Show resolved Hide resolved

This was referenced Jun 16, 2021

test: Use MiniWallet in test_no_inherited_signaling RBF test #22210

Merged

tests: Run both descriptor and legacy tests within a single test invocation #20892

Closed

test: Add gettransaction test for "coin-join" tx #18919

Merged

theStack force-pushed the 202106-test-refactor-replace_manual_deser_with_fromtohex branch from 82449a5 to 6665fec Compare June 16, 2021 14:14

theStack changed the title ~~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 Jun 16, 2021

practicalswift reviewed Jun 16, 2021

View reviewed changes

test/functional/test_framework/messages.py Outdated Show resolved Hide resolved

brunoerg approved these changes Jun 17, 2021

View reviewed changes

theStack force-pushed the 202106-test-refactor-replace_manual_deser_with_fromtohex branch 2 times, most recently from 02d2f96 to 18b780b Compare June 17, 2021 15:55

DrahtBot added the Needs rebase label Jun 19, 2021

theStack force-pushed the 202106-test-refactor-replace_manual_deser_with_fromtohex branch from 18b780b to c43d96e Compare June 19, 2021 11:02

DrahtBot removed the Needs rebase label Jun 19, 2021

maflcko reviewed Jun 19, 2021

View reviewed changes

test/functional/mempool_accept.py Outdated Show resolved Hide resolved

theStack force-pushed the 202106-test-refactor-replace_manual_deser_with_fromtohex branch 2 times, most recently from 8eb2389 to 65bf7b0 Compare June 19, 2021 20:33

theStack force-pushed the 202106-test-refactor-replace_manual_deser_with_fromtohex branch from 65bf7b0 to 446fd17 Compare June 20, 2021 12:05

theStack changed the title ~~test: refactor: use FromHex helper for msg serialization from hex, remove ToHex helper~~ test: refactor: various (de)serialization helpers cleanups/improvements Jun 20, 2021

theStack and others added 4 commits June 21, 2021 14:28

test: introduce tx_from_hex helper for tx deserialization

2ce7b47

`FromHex` is mostly used for transactions, so we introduce a shortcut `tx_from_hex` for `FromHex(CTransaction, hex_str)`.

test: remove ToHex helper, use .serialize().hex() instead

a79396f

scripted-diff: test: rename FromHex to from_hex

1914054

-BEGIN VERIFY SCRIPT- sed -i 's/\<FromHex\>/from_hex/g' $(git grep -l FromHex) -END VERIFY SCRIPT- Co-authored-by: MarcoFalke <[email protected]>

test: doc: improve doc for from_hex helper (mention to_hex altern…

bdb8b9a

…ative)

theStack force-pushed the 202106-test-refactor-replace_manual_deser_with_fromtohex branch from 446fd17 to bdb8b9a Compare June 21, 2021 12:50

DrahtBot mentioned this pull request Jun 23, 2021

test: Add functional test for replacement relay fee check #22310

Merged

maflcko merged commit d6a5916 into bitcoin:master Jun 24, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 24, 2021

Merge bitcoin#22257: test: refactor: various (de)serialization helper…

d8c5c3d

…s clean…

theStack deleted the 202106-test-refactor-replace_manual_deser_with_fromtohex branch July 31, 2021 20:06

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge d6a5916 into merged_master (Bitcoin PR bitcoin/bitcoin#22257)

083e08c

Just as mechanical and annoying as it looks.

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Conversation

theStack commented Jun 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

klementtan left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake commented Jun 16, 2021

Uh oh!

practicalswift commented Jun 16, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

theStack commented Jun 16, 2021

Uh oh!

Uh oh!

brunoerg left a comment

Choose a reason for hiding this comment

Uh oh!

theStack commented Jun 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Jun 17, 2021

Uh oh!

theStack commented Jun 19, 2021

Uh oh!

Uh oh!

theStack commented Jun 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jun 20, 2021

Uh oh!

theStack commented Jun 20, 2021

Uh oh!

maflcko commented Jun 21, 2021

Uh oh!

maflcko commented Jun 21, 2021

Uh oh!

theStack commented Jun 21, 2021

Uh oh!

maflcko commented Jun 23, 2021

Uh oh!

maflcko commented Jun 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

theStack commented Jun 15, 2021 •

edited

Loading

DrahtBot commented Jun 16, 2021 •

edited

Loading

theStack commented Jun 17, 2021 •

edited

Loading

theStack commented Jun 19, 2021 •

edited

Loading