test: Validate UTXO snapshot with coin height > base height & amount > MAX_MONEY supply #29617

jrakibi · 2024-03-11T03:00:51Z

Ensure snapshot loading fails for coins exceeding base height

Objective: This test verifies that snapshot loading is correctly rejected for coins with a height greater than the base height.

Update:

Added test_invalid_snapshot_wrong_coin_code to feature_assumeutxo.py.
The test artificially sets a coin's height above 299 in a snapshot and checks for load failure.
Edit: Added a test case for outputs whose amounts surpass the MAX_MONEY supply limit.

This implementation addresses the request for enhancing assumeutxo testing as outlined in issue #28648

Edit: This is an explanation on how I arrive at content values: b"\x84\x58" and b"\xCA\xD2\x8F\x5A"

You can use this tool to decode the utxo snapshot https://github.com/jrakibi/utxo-live
Here’s an overview of how it’s done:
The serialization format for a UTXO in the snapshot is as follows:

Transaction ID (txid) - 32 bytes
Output Index (outnum)- 4 bytes
VARINT (code) - A varible-length integer encoding the height and whether the transaction is a coinbase. The format of this VARINT is (height << 1) | coinbase_flag.
VARINT (amount_v) - A variable-length integer that represents a compressed format of the output amount (in satoshis).

For the test cases mentioned:

b"\x84\x58" - This value corresponds to a VARINT representing the height and coinbase flag. Once we decode this code, we can extract the height and coinbase using height = code_decoded >> 1 and coinbase = code_decoded & 0x01. In our case, with code_decoded = 728, it results in height = 364 and coinbase = 0.
b"\xCA\xD2\x8F\x5A" - This byte sequence represents a compressed amount value. The decompression function takes this value and translates it into a full amount in satoshis. In our case, the decompression of this amount translates to a number larger than the maximum allowed value of coins (21 million BTC)

DrahtBot · 2024-03-11T03:00:54Z

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	fjahr, maflcko, achow101
Stale ACK	rkrux

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:

#29612 (rpc: Optimize serialization and enhance metadata of dumptxoutset output by fjahr)
#27432 (contrib: add tool to convert compact-serialized UTXO set to SQLite database by theStack)

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 · 2024-03-11T04:05:48Z

🚧 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/22494357167}

BrandonOdiwuor

You should remove the TODO:

bitcoin/test/functional/feature_assumeutxo.py

Lines 19 to 20 in a945f09

- TODO: Valid hash but invalid snapshot file (bad coin height or

bad other serialization)

test/functional/feature_assumeutxo.py

ismaelsadeeq

First pass Partial review.

Please squash the two commits https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
From the PR OP and description you mention base height, I am not familiar with the term "base height" did you mean chain tip?

ismaelsadeeq · 2024-03-11T15:50:14Z

test/functional/feature_assumeutxo.py

@@ -148,6 +148,42 @@ def test_invalid_mempool_state(self, dump_output_path):

        self.restart_node(2, extra_args=self.extra_args[2])

+    def test_invalid_snapshot_wrong_coin_code(self, valid_snapshot_path):
+        self.log.info(
+            "Testing snapshot file with an incorrect coin code: height 364 and coinbase 0"


Suggested change

"Testing snapshot file with an incorrect coin code: height 364 and coinbase 0"

"Testing snapshot file with an incorrect coin height 364 and coinbase 0"

Thanks @ismaelsadeeq, I've squashed them.
Base height is related to AssumeUTXO snapshot, refers to the block height at which the UTXOset snapshot was taken.
Also coin code in this context encodes the block height and whether the tx is Coinbase or not

test/functional/feature_assumeutxo.py

maflcko · 2024-03-11T16:02:17Z

Missing test: prefix in pull title?

DrahtBot · 2024-03-13T00:01:40Z

🚧 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/22587330142}

jrakibi · 2024-03-13T00:47:12Z

@ismaelsadeeq @BrandonOdiwuor I've updated the PR with your feedbacks. Also included my test case in test_invalid_snapshot_scenarios method to keep all invalid snapshot scenarios cases in one place

jrakibi · 2024-03-19T00:41:17Z

I've just added another test case to invalidate snapshots containing outputs whose amounts exceed the MAX_MONEY supply limit

rkrux

tACK e807838

I was able to successfully build and run all the functional tests.
Provided a refactoring suggestion in favour of clear separation of variables.

test/functional/feature_assumeutxo.py

fjahr

Concept ACK

I think it would be helpful for reviewers if you could add information in the commit message how you arrived at the content values b"\x84\x58" and b"\xCA\xD2\x8F\x5A".

test/functional/feature_assumeutxo.py

jrakibi · 2024-04-22T13:48:22Z

@fjahr I've updated the code based on your feedback. In the commit & PR description, you can find the steps followed to arrive at the content values, along with a tool to decode a UTXO snapshot

fjahr

Code review ACK 6428b36

Ignore the nit unless there is other feedback to address or you need to rebase.

fjahr · 2024-04-22T15:45:39Z

test/functional/feature_assumeutxo.py

+            [b"\x81", 36, "3da966ba9826fb6d2604260e01607b55ba44e1a5de298606b08704bc62570ea8", None],  # wrong coin code VARINT
+            [b"\x80", 36, "091e893b3ccb4334378709578025356c8bcb0a623f37c7c4e493133c988648e5", None],  # another wrong coin code
+            [b"\x84\x58", 36, None, "[snapshot] bad snapshot data after deserializing 0 coins"],  # wrong coin case with height 364 and coinbase 0
+            [b"\xCA\xD2\x8F\x5A", 41, None, "[snapshot] bad snapshot data after deserializing 0 coins - bad tx out value"],  # Amount exceed MAX_MONEY


nit: exceeds

Fixed and rebased.

… money_supply You can use this tool to decode the utxo snapshot https://github.com/jrakibi/utxo-live Here’s an overview of how it’s done: The serialization forma for a UTXO in the snapshot is as follows: 1. Transaction ID (txid) - 32 bytes 2. Output Index (outnum)- 4 bytes 3. VARINT (code) - A varible-length integer encoding the height and whether the transaction is a coinbase. The format of this VARINT is (height << 1) | coinbase_flag. 4. VARINT (amount_v) - A variable-length integer that represents a compressed format of the output amount (in satoshis). For the test cases mentioned: * b"\x84\x58" - This value corresponds to a VARINT representing the height and coinbase flag. Once we decode this code, we can extract the height and coinbase using height = code_decoded >> 1 and coinbase = code_decoded & 0x01. In our case, with code_decoded = 728, it results in height = 364 and coinbase = 0. * b"\xCA\xD2\x8F\x5A" - This byte sequence represents a compressed amount value. The decompression function takes this value and translates it into a full amount in satoshis. In our case, the decompression of this amount translates to a number larger than the maximum allowed value of coins (21 million BTC) test:Validate UTXO snapshot with coin_height > base_height & amount > money_supply test:Validate UTXO snapshot with coin_height > base_height & amount > money_supply

fjahr · 2024-04-22T23:35:37Z

re-ACK ec1f1ab

maflcko · 2024-04-23T07:33:45Z

ACK ec1f1ab 👑

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK ec1f1abfefa281e62bb876aa1c4738d576ef9a4 👑
JmyXc4AA3GlfNbDIttEDoTvrUHdJtZaS+AtVKAl4yGV+WmcfcEFyZjxpaa5jgmWqix+7xHNhB/dd8/He/ZgGBg==

achow101 · 2024-05-02T20:38:55Z

ACK ec1f1ab

DrahtBot added the CI failed label Mar 11, 2024

jrakibi closed this Mar 11, 2024

jrakibi reopened this Mar 11, 2024

BrandonOdiwuor reviewed Mar 11, 2024

View reviewed changes

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

ismaelsadeeq reviewed Mar 11, 2024

View reviewed changes

jrakibi changed the title ~~Add test for invalid UTXO snapshot with coin height above base height~~ test: Add test for invalid UTXO snapshot with coin height above base height Mar 11, 2024

jrakibi force-pushed the 2024/03/assumeUTXO-tests branch from 46f16a3 to 4104d9f Compare March 11, 2024 17:35

DrahtBot mentioned this pull request Mar 11, 2024

test: Assumeutxo: snapshots with less work should not be loaded #29428

Open

DrahtBot added Tests and removed CI failed labels Mar 12, 2024

jrakibi force-pushed the 2024/03/assumeUTXO-tests branch from 885be24 to 0ca552c Compare March 13, 2024 00:00

DrahtBot added the CI failed label Mar 13, 2024

jrakibi force-pushed the 2024/03/assumeUTXO-tests branch 2 times, most recently from 21825ad to 28ab34d Compare March 13, 2024 00:36

jrakibi closed this Mar 13, 2024

jrakibi reopened this Mar 13, 2024

jrakibi force-pushed the 2024/03/assumeUTXO-tests branch from 28ab34d to a94cfca Compare March 13, 2024 00:50

This was referenced Mar 13, 2024

rpc: Optimize serialization and enhance metadata of dumptxoutset output #29612

Merged

assumeutxo: Get rid of faked nTx and nChainTx values #29370

Merged

DrahtBot removed the CI failed label Mar 13, 2024

fjahr mentioned this pull request Mar 14, 2024

AssumeUTXO Mainnet Readiness Tracking #29616

Open

22 tasks

jrakibi force-pushed the 2024/03/assumeUTXO-tests branch from a94cfca to 1349ea9 Compare March 19, 2024 00:32

jrakibi changed the title ~~test: Add test for invalid UTXO snapshot with coin height above base height~~ test: Validate UTXO snapshot with coin height > base height & amount > money supply Mar 19, 2024

jrakibi changed the title ~~test: Validate UTXO snapshot with coin height > base height & amount > money supply~~ test: Validate UTXO snapshot with coin height > base height & amount > MAX_MONEY supply Mar 19, 2024

DrahtBot added the Needs rebase label Mar 20, 2024

achow101 requested a review from fjahr April 9, 2024 14:27

jrakibi force-pushed the 2024/03/assumeUTXO-tests branch from 1349ea9 to e807838 Compare April 10, 2024 13:54

DrahtBot removed the Needs rebase label Apr 10, 2024

DrahtBot mentioned this pull request Apr 11, 2024

contrib: add tool to convert compact-serialized UTXO set to SQLite database #27432

Open

rkrux approved these changes Apr 15, 2024

View reviewed changes

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

fjahr reviewed Apr 15, 2024

View reviewed changes

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

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

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

jrakibi force-pushed the 2024/03/assumeUTXO-tests branch 3 times, most recently from f614e0e to 6428b36 Compare April 22, 2024 13:34

DrahtBot added the CI failed label Apr 22, 2024

fjahr reviewed Apr 22, 2024

View reviewed changes

DrahtBot requested a review from rkrux April 22, 2024 16:53

jrakibi force-pushed the 2024/03/assumeUTXO-tests branch 2 times, most recently from 8766cbb to ef27f36 Compare April 22, 2024 22:20

jrakibi force-pushed the 2024/03/assumeUTXO-tests branch from ef27f36 to ec1f1ab Compare April 22, 2024 22:37

DrahtBot removed the CI failed label Apr 23, 2024

achow101 merged commit 62ef33a into bitcoin:master May 2, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: Validate UTXO snapshot with coin height > base height & amount > MAX_MONEY supply #29617

test: Validate UTXO snapshot with coin height > base height & amount > MAX_MONEY supply #29617

jrakibi commented Mar 11, 2024 •

edited

DrahtBot commented Mar 11, 2024 •

edited

DrahtBot commented Mar 11, 2024

BrandonOdiwuor left a comment

ismaelsadeeq left a comment

ismaelsadeeq Mar 11, 2024

jrakibi Mar 11, 2024 •

edited

maflcko commented Mar 11, 2024

DrahtBot commented Mar 13, 2024

jrakibi commented Mar 13, 2024 •

edited

jrakibi commented Mar 19, 2024

rkrux left a comment

fjahr left a comment

jrakibi commented Apr 22, 2024 •

edited

fjahr left a comment

fjahr Apr 22, 2024

jrakibi Apr 22, 2024

fjahr commented Apr 22, 2024

maflcko commented Apr 23, 2024

achow101 commented May 2, 2024

	- TODO: Valid hash but invalid snapshot file (bad coin height or
	bad other serialization)

	"Testing snapshot file with an incorrect coin code: height 364 and coinbase 0"
	"Testing snapshot file with an incorrect coin height 364 and coinbase 0"

test: Validate UTXO snapshot with coin height > base height & amount > MAX_MONEY supply #29617

test: Validate UTXO snapshot with coin height > base height & amount > MAX_MONEY supply #29617

Conversation

jrakibi commented Mar 11, 2024 • edited

Ensure snapshot loading fails for coins exceeding base height

DrahtBot commented Mar 11, 2024 • edited

Code Coverage

Reviews

Conflicts

DrahtBot commented Mar 11, 2024

BrandonOdiwuor left a comment

Choose a reason for hiding this comment

ismaelsadeeq left a comment

Choose a reason for hiding this comment

ismaelsadeeq Mar 11, 2024

Choose a reason for hiding this comment

jrakibi Mar 11, 2024 • edited

Choose a reason for hiding this comment

maflcko commented Mar 11, 2024

DrahtBot commented Mar 13, 2024

jrakibi commented Mar 13, 2024 • edited

jrakibi commented Mar 19, 2024

rkrux left a comment

Choose a reason for hiding this comment

fjahr left a comment

Choose a reason for hiding this comment

jrakibi commented Apr 22, 2024 • edited

fjahr left a comment

Choose a reason for hiding this comment

fjahr Apr 22, 2024

Choose a reason for hiding this comment

jrakibi Apr 22, 2024

Choose a reason for hiding this comment

fjahr commented Apr 22, 2024

maflcko commented Apr 23, 2024

achow101 commented May 2, 2024

jrakibi commented Mar 11, 2024 •

edited

DrahtBot commented Mar 11, 2024 •

edited

jrakibi Mar 11, 2024 •

edited

jrakibi commented Mar 13, 2024 •

edited

jrakibi commented Apr 22, 2024 •

edited