test: Remove struct.pack from almost all places

maflcko commented at 12:07 pm on February 7, 2024: member

struct.pack has many issues:

The format string consists of characters that may be confusing and may need to be looked up in the documentation, as opposed to using easy to understand self-documenting code.

This lead to many test bugs, which weren’t hit, which is fine, but still confusing. Ref: #29400, #29399, #29363, fa3886b7c69cbbe564478f30bb2c35e9e6b1cffa, …

Fix all issues by using the built-in int helpers to_bytes via a scripted diff.

Review notes:

For struct.pack and int.to_bytes the error behavior is the same, although the error messages are not identical.
Two struct.pack remain. One for float serialization in a C++ code comment, and one for native serialization.

DrahtBot commented at 12:07 pm on February 7, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	rkrux, theStack, achow101
Concept ACK	epiccurious

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29325 (consensus: Store transaction nVersion as uint32_t by achow101)
#27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
#27286 (wallet: Keep track of the wallet’s own transaction outputs in memory by achow101)

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.

DrahtBot renamed this:
~~test: Remove struct.pack from almost all places~~
test: Remove struct.pack from almost all places
on Feb 7, 2024

DrahtBot added the label Tests on Feb 7, 2024

maflcko marked this as a draft on Feb 7, 2024

DrahtBot added the label CI failed on Feb 7, 2024

DrahtBot commented at 1:13 pm on February 7, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/21315900160

epiccurious commented at 2:32 pm on February 9, 2024: contributor

Concept ACK. Is this ready for testing?

maflcko force-pushed on Feb 12, 2024

DrahtBot removed the label CI failed on Feb 12, 2024

DrahtBot added the label Needs rebase on Feb 21, 2024

maflcko marked this as ready for review on Feb 22, 2024

maflcko force-pushed on Feb 22, 2024

DrahtBot removed the label Needs rebase on Feb 22, 2024

test: Normalize struct.pack format

* Add () around some int values
* Remove b-prefix from strings

This is needed for the scripted diff to work.

faf3cd659a

test: Use int.to_bytes over struct packing

This is done in prepration for the scripted diff, which can not deal
with those lines.

faf2a975ad

scripted-diff: test: Use int.to_bytes over struct packing

-BEGIN VERIFY SCRIPT-
 sed -i --regexp-extended 's!struct.pack\(.<?B., (.*)\)!\1.to_bytes(1, "little")!g'             $( git grep -l struct.pack )
 sed -i --regexp-extended 's!struct.pack\(.<I., (.*)\)!\1.to_bytes(4, "little")!g'              $( git grep -l struct.pack )
 sed -i --regexp-extended 's!struct.pack\(.<H., (.*)\)!\1.to_bytes(2, "little")!g'              $( git grep -l struct.pack )
 sed -i --regexp-extended 's!struct.pack\(.<i., (.*)\)!\1.to_bytes(4, "little", signed=True)!g' $( git grep -l struct.pack )
 sed -i --regexp-extended 's!struct.pack\(.<q., (.*)\)!\1.to_bytes(8, "little", signed=True)!g' $( git grep -l struct.pack )
 sed -i --regexp-extended 's!struct.pack\(.>H., (.*)\)!\1.to_bytes(2, "big")!g'                 $( git grep -l struct.pack )
-END VERIFY SCRIPT-

fa826db477

test: Remove struct.pack from almost all places fa52e13ee8

maflcko force-pushed on May 7, 2024

rkrux approved

rkrux commented at 9:52 am on May 18, 2024: none

tACK fa52e13

Make successful, so are all the functional tests.

Overall I agree with the intent to get rid of struct.pack usage, which requires looking up the documentation most of the time and might be prone to errors. Using the alternate approach of to_bytes is more self-documenting and preferable to me.

theStack commented at 12:14 pm on June 2, 2024: contributor

Concept ACK

in contrib/seeds/generate-seeds.py:119 in fa52e13ee8

115@@ -117,11 +116,11 @@ def ser_compact_size(l):
116     if l < 253:
117         r = l.to_bytes(1, "little")
118     elif l < 0x10000:
119-        r = struct.pack("<BH", 253, l)
120+        r = (253).to_bytes(1, "little") + l.to_bytes(2, "little")

theStack commented at 1:01 pm on June 6, 2024:

nit: for serializing single bytes I personally prefer bytes([value]) over the .to_bytes method, e.g.

0        r = bytes([253]) + l.to_bytes(2, "little")

(but no blocker, feel free to ignore)

maflcko commented at 3:08 pm on June 6, 2024:

I agree, but for consistency with the other def ser_compact_size and the other code, I’ll leave as-is for now.

theStack approved

theStack commented at 1:02 pm on June 6, 2024: contributor

Code-review ACK fa52e13ee81fcc7543890dbd6986fcb55168583f

achow101 commented at 11:09 pm on June 6, 2024: member

ACK fa52e13ee81fcc7543890dbd6986fcb55168583f

achow101 merged this on Jun 6, 2024

achow101 closed this on Jun 6, 2024

maflcko deleted the branch on Jun 7, 2024

test: Remove struct.pack from almost all places #29401

Code Coverage

Reviews

Conflicts