qa: clarify and document one assumeutxo test case with malleated snapshot

darosior commented at 3:11 pm on February 19, 2025: member

The feature_assumeutxo.py functional test checks various errors with malleated snapshots. Some of these cases are brittle or use confusing and undocumented values. Fix one of those by using a clear, documented and forward-compatible value.

I ran across those when working on an unrelated changeset which affected the snapshot. It took me a while to understand where the seemingly magic byte string was coming from, so i figured it was worth proposing this patch on its own for the sake of making the test more maintainable.

See commit messages for details.

DrahtBot commented at 3:11 pm on February 19, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31907.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	janb84, theStack, fjahr, i-am-yuvi
Concept ACK	rkrux
Stale ACK	Prabhat1308

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

DrahtBot added the label Tests on Feb 19, 2025

TheCharlatan commented at 9:15 pm on February 19, 2025: contributor

The third paragraph in the first commit messages mentions commits that don’t exist here. I’m guessing that is a reference to the original patchset you developed this with?

darosior commented at 9:33 pm on February 19, 2025: member

Ugh yes it is indeed. Will fix.

in test/functional/feature_assumeutxo.py:144 in f43b6ba276 outdated

138@@ -139,7 +139,14 @@ def expected_error(msg):
139             [b"\x81", 34, "3da966ba9826fb6d2604260e01607b55ba44e1a5de298606b08704bc62570ea8", None],  # wrong coin code VARINT
140             [b"\x80", 34, "091e893b3ccb4334378709578025356c8bcb0a623f37c7c4e493133c988648e5", None],  # another wrong coin code
141             [b"\x84\x58", 34, None, "Bad snapshot data after deserializing 0 coins"],  # wrong coin case with height 364 and coinbase 0
142-            [b"\xCA\xD2\x8F\x5A", 39, None, "Bad snapshot data after deserializing 0 coins - bad tx out value"],  # Amount exceeds MAX_MONEY
143+            [
144+                # VARINT(Compressed(MAX_MONEY + 1)) + VARINT(0)
145+                b"\xa0\xc8\xad\xb1\xd1\x83\xff\x01" + b"\x00",

theStack commented at 0:45 am on February 20, 2025:

Could even go one step further and eliminate the magic bytes by implementing VARINT serialization and amount compression routines in the test framework. If you want to pull in, feel free: https://github.com/theStack/bitcoin/commits/202502-test-introduce_varint_and_amount_compression_routines/

darosior commented at 2:25 pm on February 20, 2025:

I’m on the fence. I agree it’s nicer but also it would 10x the size of the change with harder to review code. Thanks for the commit but i’d rather keep it this way for now, unless/until more reviewers believe it’s worth pulling in a full varint implementation.

theStack commented at 2:49 pm on February 20, 2025:

Fair enough, doesn’t have to happen in this PR (if at all), happy to ACK without (once #31907 (comment) is fixed). Still, I think it would be nice long-term if varints would be easy to change/create, without having to fiddle around in the C++ codebase to get out the serialization.

rkrux commented at 10:45 am on February 21, 2025:

The current implementation certainly makes it easier to parse but for someone new to this piece of code it still feels like magic value as the amount compression logic and the varint serialisation logic are quite custom. To eliminate the magical-ness completely, I would support pulling in the said routines in the test framework. Some of them are already present in the utxo-to-sqlite.py tool.

fjahr commented at 8:28 pm on February 21, 2025:

I’m in favor of adding varint and compression implementation here since it’s a more meaningful improvement and the code is already there. Also not sure why we need to add VARINT(0)?

darosior commented at 10:51 pm on March 3, 2025:

Also not sure why we need to add VARINT(0)?

You mean in the comment? I guess it doesn’t matter either way but it’s clearer this way…

If like @Prabhat1308 you ask why it’s necessary to have an empty varint in the serialization itself, this is simply because otherwise you would get a serialization error instead of a MoneyRange error.

To give more details:

this test is checking the error returned there https://github.com/bitcoin/bitcoin/blob/79bbb381a1fd13ad456fde736b3c195a791d4e58/src/validation.cpp#L5958
which requires a coin was successfully deserialized there https://github.com/bitcoin/bitcoin/blob/79bbb381a1fd13ad456fde736b3c195a791d4e58/src/validation.cpp#L5951
which in turn requires to successfully deserialize a compressed txout https://github.com/bitcoin/bitcoin/blob/79bbb381a1fd13ad456fde736b3c195a791d4e58/src/compressor.h#L112-L115
therefore my serialization uses an amount of MAX_MONEY + 1 (to test the bound) and an empty scriptpubkey (because we don’t care). The latter is serialized as an empty VarInt.

theStack commented at 0:50 am on February 20, 2025: contributor

Concept ACK, I agree that the snapshot modification tests are in a bit of a messy state currently.

in test/functional/feature_assumeutxo.py:136 in f43b6ba276 outdated

132@@ -133,13 +133,20 @@ def expected_error(msg):
133         cases = [
134             # (content, offset, wrong_hash, custom_message)
135             [b"\xff" * 32, 0, "7d52155c9a9fdc4525b637ef6170568e5dad6fabd0b1fdbb9432010b8453095b", None],  # wrong outpoint hash
136-            [(2).to_bytes(1, "little"), 32, None, "Bad snapshot data after deserializing 1 coins."],  # wrong txid coins count
137+            [(2).to_bytes(1, "little"), 32, None, "Bad snapshot data after deserializing 1 coins"],  # wrong txid coins count

yancyribbens commented at 1:31 am on February 20, 2025:

nit 1 coin?

rkrux commented at 3:05 pm on February 20, 2025:

This is an expected error from the underlying RPC call. Although using coin here in the assertion won’t fail the functional test but I don’t think this makes any meaningful difference.

yancyribbens commented at 1:45 am on February 21, 2025:

Ah, I see thanks. I guess the underlying call would be more clear to return coin(s) although that’s besides the point.

janb84 commented at 12:04 pm on February 20, 2025: contributor

tACK f43b6ba

Ran unit and functional tests related to assumeutxo (test/functional/feature_assumeutxo.py).
Ran all the functional tests
Validated the code change.

I also wrote a review note on the PR

DrahtBot requested review from theStack on Feb 20, 2025

darosior force-pushed on Feb 20, 2025

darosior commented at 5:18 pm on February 20, 2025: member

Updated the commit message to remove the reference to later modification i had in my branch.

janb84 commented at 10:08 am on February 21, 2025: contributor

reACK dedc2d9

in test/functional/feature_assumeutxo.py:1 in 0a0c803785 outdated

rkrux commented at 10:47 am on February 21, 2025:

Comment about the commit message: Curious to know the reason behind using UTx0 instead of UTXO.

fjahr commented at 7:04 pm on February 21, 2025:

In 0a0c803785782e40f4fe8055383565e3d24b59cc : That is weird but also I seem to be unable to parse this commit message (and the next one). Can this be made more understandable?

What I can gather is this seems to make the test less tight around the actually expected error (?) and I don’t understand how that would make the test better.

“as the snapshot data may one day be so that a coin is correctly deserialized and none of these errors is hit”. I don’t understand how we are supposed to write tests for hypothetical future changes? If there are changes and these errors are not hit anymore then the test fails and it will need to be updated. I don’t see what’s the problem with that.

darosior commented at 10:40 pm on March 3, 2025:

Curious to know the reason behind using UTx0 instead of UTXO.

:shrug: doesn’t matter, but as it is an acronym it makes more sense to me to capitalize the first letter of each word an not the others.

Can this be made more understandable?

That’s a pretty vague request.. I tried to be as detailed as possible in the commit message since the change is not obvious. Could you point to what you don’t understand exactly?

What I can gather is this seems to make the test less tight around the actually expected error (?) and I don’t understand how that would make the test better.

Indeed this makes the test less tight because the test is currently unnecessarily specific. It’s supposed to be testing that a wrong txid count will cause a deserialization error, but it’s currently tightened to one of the many deserialization errors that can happen. The error that happens will depend on the snapshot data itself, which may change, therefore this check is brittle. This commit changes the check to match on two of the many deserialization errors that can happen. This makes it slightly less brittle.

I don’t see what’s the problem with that.

The issue is that the test does not check what is documented but something stricter and dependent on the snapshot data itself which may vary.

I think i’ll just drop this commit from this PR and keep it with my other PR.

rkrux commented at 2:06 pm on March 4, 2025:

doesn’t matter,

What immediately caught my eye was the unusual looking 0, which at first glance made me think it was zero. Nevertheless, I am not suggesting any changes related to this, it just made me curious.

darosior commented at 3:51 pm on March 4, 2025:

I’ve now dropped this commit from this PR. I’ll keep it with my followup branch for now.

rkrux commented at 10:48 am on February 21, 2025: contributor

Concept ACK dedc2d9cecccb866230c07112d00288237d12eaa

fanquake commented at 5:50 pm on February 21, 2025: member

@fjahr you might want to have a look here?

in test/functional/feature_assumeutxo.py:144 in dedc2d9cec outdated

141             [b"\x80", 34, "091e893b3ccb4334378709578025356c8bcb0a623f37c7c4e493133c988648e5", None],  # another wrong coin code
142             [b"\x84\x58", 34, None, "Bad snapshot data after deserializing 0 coins"],  # wrong coin case with height 364 and coinbase 0
143-            [b"\xCA\xD2\x8F\x5A", 39, None, "Bad snapshot data after deserializing 0 coins - bad tx out value"],  # Amount exceeds MAX_MONEY
144+            [
145+                # VARINT(Compressed(MAX_MONEY + 1)) + VARINT(0)
146+                b"\xa0\xc8\xad\xb1\xd1\x83\xff\x01" + b"\x00",

Prabhat1308 commented at 2:27 pm on March 2, 2025:

as the above comment has already asked , I can’t seem to understand why VARINT(0) was added considering we have already exceeded the max amount in the first amount written itself.

darosior commented at 10:52 pm on March 3, 2025:

See #31907 (review)

Prabhat1308 commented at 2:38 pm on March 2, 2025: none

tACK dedc2d9

tested the amount with compression and varint encoding to match .
On the case of implementing varint serialization and compression logic , it would be a great addition to bring in the logic to test_framework (or in any other PR , no preference to bring it in this one).

DrahtBot requested review from rkrux on Mar 2, 2025

test: introduce VARINT (de)serialization routines a7911ed101

darosior force-pushed on Mar 4, 2025

darosior commented at 3:50 pm on March 4, 2025: member

Since it was tiny, pretty specifc to my branch and confused some reviewers, i dropped the first commit of this PR.

Since several reviewers were in favor, i included in this PR @theStack’s commits to introduce varint and compression serialization primitives to the functional test framework. I adapted the commit to use these primitives instead of the documented bytestring i was using before.

I have also reworked the commit message a bit. Please let me know if it’s still too hard to understand.

DrahtBot commented at 5:38 pm on March 4, 2025: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/38180547289

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly 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.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

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

DrahtBot added the label CI failed on Mar 4, 2025

test: introduce output amount (de)compression routines b34fdb5ade

qa: use a clearer and documented amount error in malleated snapshot

In the assumeutxo functional tests, the final test case with alternated UTxO data tests the error
raised when deserializing a snapshot that contains a coin with an amount not in range (<0 or
>MAX_MONEY).

The current malleation uses an undocumented byte string and offset which makes it hard to maintain.
In addition, the undocumented offset is set surprisingly high (39 bytes is well into the
serialization of the amount which starts at offset 36). Similarly the value is surprisingly small,
presumably one was adjusted for the other. But there is no comment explaining how they were chosen,
why not in a clearer manner and what they are supposed to represent.

Instead replace this seemingly magic value with a clear one, MAX_MONEY + 1, serialize the whole
value for the amount field at the correct offset, and document the whole thing for the next person
around.

e5ff4e416e

darosior force-pushed on Mar 4, 2025

DrahtBot removed the label CI failed on Mar 4, 2025

janb84 commented at 9:22 am on March 5, 2025: contributor

re ACK e5ff4e4

DrahtBot requested review from Prabhat1308 on Mar 5, 2025

theStack approved

theStack commented at 2:20 pm on March 5, 2025: contributor

ACK e5ff4e416ecc8a51367022eb8a7a291f8cbc0c65

in test/functional/test_framework/messages.py:123 in a7911ed101 outdated

119@@ -120,6 +120,26 @@ def deser_compact_size(f):
120     return nit
121 
122 
123+def ser_varint(l):

fjahr commented at 5:22 pm on March 5, 2025:

This could probably use a hint about VarIntMode? Could be left for a follow-up though since maybe in the future we may want both.

in test/functional/test_framework/messages.py:1943 in a7911ed101 outdated

1938+        def check_varint(num, expected_encoding_hex):
1939+            expected_encoding = bytes.fromhex(expected_encoding_hex)
1940+            self.assertEqual(ser_varint(num), expected_encoding)
1941+            self.assertEqual(deser_varint(BytesIO(expected_encoding)), num)
1942+
1943+        # test cases from serialize_tests.cpp:varint_bitpatterns

fjahr commented at 5:22 pm on March 5, 2025:

nit-ish: Reference is not fully correct

0        # test cases from serialize_tests.cpp:varints_bitpatterns

in test/functional/feature_assumeutxo.py:150 in e5ff4e416e

143@@ -139,7 +144,14 @@ def expected_error(msg):
144             [b"\x81", 34, "3da966ba9826fb6d2604260e01607b55ba44e1a5de298606b08704bc62570ea8", None],  # wrong coin code VARINT
145             [b"\x80", 34, "091e893b3ccb4334378709578025356c8bcb0a623f37c7c4e493133c988648e5", None],  # another wrong coin code
146             [b"\x84\x58", 34, None, "Bad snapshot data after deserializing 0 coins"],  # wrong coin case with height 364 and coinbase 0
147-            [b"\xCA\xD2\x8F\x5A", 39, None, "Bad snapshot data after deserializing 0 coins - bad tx out value"],  # Amount exceeds MAX_MONEY
148+            [
149+                # compressed txout value + scriptpubkey
150+                ser_varint(compress_amount(MAX_MONEY + 1)) + ser_varint(0),
151+                # txid + coins per txid + vout + coin height

fjahr commented at 6:13 pm on March 5, 2025:

0                # txid + coins per txid + vout + coin code (height*2 + coinbase)

fjahr commented at 6:20 pm on March 5, 2025: contributor

Code review ACK e5ff4e416ecc8a51367022eb8a7a291f8cbc0c65

Would be fine for me to leave comments for when these files are retouched. The title and description of the PR should still be updated to reflect the new content though.

darosior renamed this:
~~qa: clarify and document assumeutxo tests~~
qa: clarify and document one assumeutxo test case with malleated snapshot
on Mar 5, 2025

darosior commented at 6:28 pm on March 5, 2025: member

Thanks for the reviews. Updated title and description to only mention one of the test cases since i dropped the other commit. I don’t think it’s worth addressing the nits unless people feel too strongly.