test: Fix utxo set hash serialisation signedness #29399

maflcko · 2024-02-07T10:51:44Z

It is unsigned in Bitcoin Core, so the tests should match it:

bitcoin/src/kernel/coinstats.cpp

Line 54 in 5b8990a

ss << static_cast<uint32_t>((coin.nHeight << 1) + coin.fCoinBase);

Large positive values for the block height are too difficult to hit in tests, but it still seems fine to fix this.

The bug was introduced when the code was written in 6ccc8fc.

(Lowercase i means signed, see https://docs.python.org/3/library/struct.html#format-characters)

DrahtBot · 2024-02-07T10:51:46Z

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	epiccurious, fjahr

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

l0rinc · 2024-02-07T11:13:36Z

test/functional/feature_utxo_set_hash.py

@@ -58,7 +56,7 @@ def test_muhash_implementation(self):
                        continue

                    data = COutPoint(int(tx.rehash(), 16), n).serialize()
-                    data += struct.pack("<i", height * 2 + coinbase)
+                    data += (height * 2 + coinbase).to_bytes(4, "little")


I'm just looking around, I'm not a Bitcoin core expert.
My understanding is that the unsigned little endian is <I, wouldn't it make sense to change as little as possible here:

Suggested change

data += (height * 2 + coinbase).to_bytes(4, "little")

data += struct.pack("<I", height * 2 + coinbase)

or if we want to mimic the C++ code:

Suggested change

data += (height * 2 + coinbase).to_bytes(4, "little")

data = struct.pack('<I', (height << 1) + coinbase)

wouldn't it make sense to change as little as possible here:

No. struct.pack is brittle and confusing. Just look at all the bugs: #29400, #29399, #29363, fa3886b, ...

Thus, struct.pack should be removed from all tests to avoid more bugs in the future.

Did that in #29401

l0rinc · 2024-02-07T11:16:33Z

test/functional/feature_utxo_set_hash.py

@@ -58,7 +56,7 @@ def test_muhash_implementation(self):
                        continue

                    data = COutPoint(int(tx.rehash(), 16), n).serialize()
-                    data += struct.pack("<i", height * 2 + coinbase)


if possible, can you cover it with a test that reproduces the error before this fix - and passes after it?

No, Large positive values for the block height are too difficult to hit in tests, but it still seems fine to fix this.

epiccurious · 2024-02-07T15:55:55Z

Tested ACK fa0ceae.

fjahr · 2024-02-09T16:59:17Z

utACK fa0ceae

maflcko · 2024-02-12T07:58:03Z

rfm? :)

fa52e13 test: Remove struct.pack from almost all places (MarcoFalke) fa826db scripted-diff: test: Use int.to_bytes over struct packing (MarcoFalke) faf2a97 test: Use int.to_bytes over struct packing (MarcoFalke) faf3cd6 test: Normalize struct.pack format (MarcoFalke) Pull request description: `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, fa3886b, ... 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. ACKs for top commit: achow101: ACK fa52e13 rkrux: tACK [fa52e13](fa52e13) theStack: Code-review ACK fa52e13 Tree-SHA512: ee80d935b68ae43d1654b047e84ceb80abbd20306df35cca848b3f7874634b518560ddcbc7e714e2e7a19241e153dee64556dc4701287ae811e26e4f8c57fe3e

fa0ceae test: Fix utxo set hash serialisation signedness (MarcoFalke) Pull request description: It is unsigned in Bitcoin Core, so the tests should match it: https://github.com/bitcoin/bitcoin/blob/5b8990a1f3c49b0b02b7383c69e95320acbda13e/src/kernel/coinstats.cpp#L54 Large positive values for the block height are too difficult to hit in tests, but it still seems fine to fix this. The bug was introduced when the code was written in 6ccc8fc. (Lowercase `i` means signed, see https://docs.python.org/3/library/struct.html#format-characters) ACKs for top commit: epiccurious: Tested ACK fa0ceae. fjahr: utACK fa0ceae Tree-SHA512: ab4405c74fb191fff8520b456d3a800cd084d616bb9ddca27d56b8e5c8969bd537490f6e204c1870dbb09a3e130b03b22a27b6644252a024059c200bbd9004e7

b70e091 Merge bitcoin#29667: fuzz: actually test garbage >64b in p2p transport test (fanquake) 6d7aa3d Merge bitcoin#29497: test: simplify test_runner.py (fanquake) d0e15d5 Merge bitcoin#29606: refactor: Reserve memory for ToLower/ToUpper conversions (Ava Chow) 045fa5f Merge bitcoin#29514: tests: Provide more helpful assert_equal errors (Ava Chow) bd607f0 Merge bitcoin#29393: i2p: log connection was refused due to arbitrary port (Ava Chow) c961755 Merge bitcoin#29595: doc: Wrap flags with code in developer-notes.md (fanquake) 8d6e5e7 Merge bitcoin#29583: fuzz: Apply fuzz env (suppressions, etc.) when fetching harness list (fanquake) 4dce690 Merge bitcoin#29576: Update functional test runner to return error code when no tests are found to run (fanquake) 910a7d6 Merge bitcoin#29529: fuzz: restrict fopencookie usage to Linux & FreeBSD (fanquake) fdac2b3 Merge bitcoin#29493: subtree: update crc32c subtree (fanquake) a23b342 Merge bitcoin#29475: doc: Fix Broken Links (fanquake) 92bad90 Merge bitcoin#28178: fuzz: Generate with random libFuzzer settings (fanquake) 9b6a05d Merge bitcoin#29443: depends: fix BDB compilation on OpenBSD (fanquake) 9963e6b Merge bitcoin#29413: fuzz: increase length of string used for `NetWhitelist{bind}Permissions::TryParse` (fanquake) 3914745 Merge bitcoin#29425: test: fix intermittent failure in wallet_reorgrestore.py (fanquake) b719883 Merge bitcoin#29399: test: Fix utxo set hash serialisation signedness (fanquake) f096880 Merge bitcoin#29377: test: Add makefile target for running unit tests (Ava Chow) 03e0bd3 Merge bitcoin#27319: addrman, refactor: improve stochastic test in `AddSingle` (Ava Chow) Pull request description: ## Issue being fixed or feature implemented Batch of trivial backports ## What was done? See commits ## How Has This Been Tested? built locally; large combined merge passed tests locally ## Breaking Changes Should be none ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK b70e091 knst: utACK b70e091 Tree-SHA512: 659a931f812c8a92cf3854abb873d92885219a392d6aa8e49ac4b27fe41e8e163ef9a135050e7c2e1bd33cecd2f7dae215e05a9c29f62e837e0057d3c16746d6

test: Fix utxo set hash serialisation signedness

fa0ceae

DrahtBot added the Tests label Feb 7, 2024

maflcko requested a review from fjahr February 7, 2024 11:03

l0rinc reviewed Feb 7, 2024

View reviewed changes

maflcko mentioned this pull request Feb 7, 2024

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

Merged

epiccurious approved these changes Feb 7, 2024

View reviewed changes

DrahtBot removed the request for review from fjahr February 9, 2024 16:59

fanquake merged commit 7d837b5 into bitcoin:master Feb 12, 2024

maflcko deleted the 2402-test-fix-sign-2- branch February 12, 2024 12:17

bitcoin locked and limited conversation to collaborators Feb 11, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

test: Fix utxo set hash serialisation signedness #29399

test: Fix utxo set hash serialisation signedness #29399

Uh oh!

maflcko commented Feb 7, 2024 •

edited

Loading

Uh oh!

DrahtBot commented Feb 7, 2024 •

edited

Loading

Uh oh!

l0rinc Feb 7, 2024 •

edited

Loading

Uh oh!

maflcko Feb 7, 2024

Uh oh!

maflcko Feb 9, 2024

Uh oh!

l0rinc Feb 7, 2024

Uh oh!

maflcko Feb 7, 2024 •

edited

Loading

Uh oh!

epiccurious commented Feb 7, 2024

Uh oh!

fjahr commented Feb 9, 2024

Uh oh!

maflcko commented Feb 12, 2024

Uh oh!

Uh oh!

	data += (height * 2 + coinbase).to_bytes(4, "little")
	data += struct.pack("<I", height * 2 + coinbase)

	data += (height * 2 + coinbase).to_bytes(4, "little")
	data = struct.pack('<I', (height << 1) + coinbase)

test: Fix utxo set hash serialisation signedness #29399

test: Fix utxo set hash serialisation signedness #29399

Uh oh!

Conversation

maflcko commented Feb 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Feb 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Uh oh!

l0rinc Feb 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Feb 7, 2024

Choose a reason for hiding this comment

Uh oh!

maflcko Feb 9, 2024

Choose a reason for hiding this comment

Uh oh!

l0rinc Feb 7, 2024

Choose a reason for hiding this comment

Uh oh!

maflcko Feb 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

epiccurious commented Feb 7, 2024

Uh oh!

fjahr commented Feb 9, 2024

Uh oh!

maflcko commented Feb 12, 2024

Uh oh!

Uh oh!

maflcko commented Feb 7, 2024 •

edited

Loading

DrahtBot commented Feb 7, 2024 •

edited

Loading

l0rinc Feb 7, 2024 •

edited

Loading

maflcko Feb 7, 2024 •

edited

Loading