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

pull jrakibi wants to merge 1 commits into bitcoin:master from jrakibi:2024/03/assumeUTXO-tests changing 1 files +11 −8
  1. jrakibi commented at 3:00 am on March 11, 2024: contributor

    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:

    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)
  2. DrahtBot commented at 3:00 am on March 11, 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 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.

  3. DrahtBot commented at 4:05 am on March 11, 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/22494357167

  4. DrahtBot added the label CI failed on Mar 11, 2024
  5. jrakibi closed this on Mar 11, 2024
  6. jrakibi reopened this on Mar 11, 2024
  7. in test/functional/feature_assumeutxo.py:151 in 46f16a3c0f outdated 
    147@@ -148,6 +148,42 @@ def test_invalid_mempool_state(self, dump_output_path):
148 
149         self.restart_node(2, extra_args=self.extra_args[2])
150 
151+    def test_invalid_snapshot_wrong_coin_code(self, valid_snapshot_path):
    ismaelsadeeq commented at 3:55 pm on March 11, 2024: 
    0    def test_snapshot_with_invalid_coin_height(self, valid_snapshot_path):
    jrakibi commented at 7:54 pm on March 11, 2024:
    I was planning to add new methods to allow for more focused test cases, so each scenario is encapsulated within its relevant context. However, I guess the method name test_invalid_snapshot_scenarios assumes all snapshot scenarios are to be included in it. So I’ll adjust my approach
  9. in test/functional/feature_assumeutxo.py:153 in 46f16a3c0f outdated 
    147@@ -148,6 +148,42 @@ def test_invalid_mempool_state(self, dump_output_path):
148 
149         self.restart_node(2, extra_args=self.extra_args[2])
150 
151+    def test_invalid_snapshot_wrong_coin_code(self, valid_snapshot_path):
152+        self.log.info(
153+            "Testing snapshot file with an incorrect coin code: height 364 and coinbase 0"
    ismaelsadeeq commented at 3:50 pm on March 11, 2024: 
    0            "Testing snapshot file with an incorrect coin height 364 and coinbase 0"
    jrakibi commented at 8:00 pm on March 11, 2024:
    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
  10. in test/functional/feature_assumeutxo.py:185 in 46f16a3c0f outdated 
    180+                    "Unable to load UTXO snapshot",
181+                    self.nodes[1].loadtxoutset,
182+                    bad_snapshot_path,
183+                )
184+
185+        check_error()
    ismaelsadeeq commented at 3:54 pm on March 11, 2024:

    IMO If the check_error subroutine is not going to be used outside the scope of test_invalid_snapshot_wrong_coin_code function, receive args and used by future tests, this can just be simplified to be inline.

    0              with self.nodes[1].assert_debug_log(["[snapshot] bad snapshot data after deserializing 0 coins"]):
1            assert_raises_rpc_error(
2                -32603,
3                "Unable to load UTXO snapshot",
4                self.nodes[1].loadtxoutset,
5                bad_snapshot_path,
6            )
  11. ismaelsadeeq commented at 3:58 pm on March 11, 2024: member

    First pass Partial review.

    1. Please squash the two commits https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
    2. From the PR OP and description you mention base height, I am not familiar with the term “base height” did you mean chain tip?
  12. maflcko commented at 4:02 pm on March 11, 2024: member
    Missing test: prefix in pull title?
  13. jrakibi renamed this:
    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
    on Mar 11, 2024
  14. jrakibi force-pushed on Mar 11, 2024
  15. DrahtBot removed the label CI failed on Mar 12, 2024
  16. DrahtBot added the label Tests on Mar 12, 2024
  17. jrakibi force-pushed on Mar 13, 2024
  18. DrahtBot added the label CI failed on Mar 13, 2024
  19. DrahtBot commented at 0:01 am on March 13, 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/22587330142

  20. jrakibi force-pushed on Mar 13, 2024
  21. jrakibi force-pushed on Mar 13, 2024
  22. jrakibi commented at 0:47 am on March 13, 2024: contributor
    @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
  23. jrakibi closed this on Mar 13, 2024
  24. jrakibi reopened this on Mar 13, 2024
  25. jrakibi force-pushed on Mar 13, 2024
  26. DrahtBot removed the label CI failed on Mar 13, 2024
  27. jrakibi force-pushed on Mar 19, 2024
  28. jrakibi commented at 0:41 am on March 19, 2024: contributor
    I’ve just added another test case to invalidate snapshots containing outputs whose amounts exceed the MAX_MONEY supply limit
  29. jrakibi renamed this:
    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
    on Mar 19, 2024
  30. jrakibi renamed this:
    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
    on Mar 19, 2024
  31. DrahtBot added the label Needs rebase on Mar 20, 2024
  32. achow101 requested review from fjahr on Apr 9, 2024
  33. jrakibi force-pushed on Apr 10, 2024
  34. DrahtBot removed the label Needs rebase on Apr 10, 2024
  35. in test/functional/feature_assumeutxo.py:122 in e807838d26 outdated 
    125+            if is_custom_message:
126+                expected_error(log_msg=error_message_or_hash)
127+            else:
128+                expected_error(
129+                    log_msg=f"[snapshot] bad snapshot content hash: expected a4bf3407ccb2cc0145c49ebba8fa91199f8a3903daf0883875941497d2493c27, got {error_message_or_hash}"
130+                )
    rkrux commented at 7:46 am on April 15, 2024:

    Suggest the below refactoring to avoid passing wrong_hash and error_message in the same variable while relying on is_custom_message to differentiate between the two.

     0diff --git a/test/functional/feature_assumeutxo.py b/test/functional/feature_assumeutxo.py
 1index a70b511c91..d07f2db530 100755
 2--- a/test/functional/feature_assumeutxo.py
 3+++ b/test/functional/feature_assumeutxo.py
 4@@ -100,25 +100,24 @@ class AssumeutxoTest(BitcoinTestFramework):
 5         self.log.info("  - snapshot file with alternated UTXO data")
 6         cases = [
 7             # (content, offset, wrong_hash or message, is_custom_message)
 8-            [b"\xff" * 32, 0, "7d52155c9a9fdc4525b637ef6170568e5dad6fabd0b1fdbb9432010b8453095b", False],  # wrong outpoint hash
 9-            [(1).to_bytes(4, "little"), 32, "9f4d897031ab8547665b4153317ae2fdbf0130c7840b66427ebc48b881cb80ad", False],  # wrong outpoint index
10-            [b"\x81", 36, "3da966ba9826fb6d2604260e01607b55ba44e1a5de298606b08704bc62570ea8", False],  # wrong coin code VARINT
11-            [b"\x80", 36, "091e893b3ccb4334378709578025356c8bcb0a623f37c7c4e493133c988648e5", False],  # another wrong coin code
12-            [b"\x84\x58", 36, "[snapshot] bad snapshot data after deserializing 0 coins", True],  # wrong coin case with height 364 and coinbase 0
13-            [b"\xCA\xD2\x8F\x5A", 41, "[snapshot] bad snapshot data after deserializing 0 coins - bad tx out value", True],  # Amount exceed MAX_MONEY
14+            [b"\xff" * 32, 0, "7d52155c9a9fdc4525b637ef6170568e5dad6fabd0b1fdbb9432010b8453095b", None],  # wrong outpoint hash
15+            [(1).to_bytes(4, "little"), 32, "9f4d897031ab8547665b4153317ae2fdbf0130c7840b66427ebc48b881cb80ad", None],  # wrong outpoint index
16+            [b"\x81", 36, "3da966ba9826fb6d2604260e01607b55ba44e1a5de298606b08704bc62570ea8", None],  # wrong coin code VARINT
17+            [b"\x80", 36, "091e893b3ccb4334378709578025356c8bcb0a623f37c7c4e493133c988648e5", None],  # another wrong coin code
18+            [b"\x84\x58", 36, None, "[snapshot] bad snapshot data after deserializing 0 coins"],  # wrong coin case with height 364 and coinbase 0
19+            [b"\xCA\xD2\x8F\x5A", 41, None, "[snapshot] bad snapshot data after deserializing 0 coins - bad tx out value"],  # Amount exceed MAX_MONEY
20         ]
21
22-        for content, offset, error_message_or_hash, is_custom_message in cases:
23+        for content, offset, wrong_hash, custom_message in cases:
24             with open(bad_snapshot_path, "wb") as f:
25                 f.write(valid_snapshot_contents[: (32 + 8 + offset)])
26                 f.write(content)
27                 f.write(valid_snapshot_contents[(32 + 8 + offset + len(content)) :])
28-
29-            if is_custom_message:
30-                expected_error(log_msg=error_message_or_hash)
31+            if custom_message:
32+                expected_error(log_msg=custom_message)
33             else:
34                 expected_error(
35-                    log_msg=f"[snapshot] bad snapshot content hash: expected a4bf3407ccb2cc0145c49ebba8fa91199f8a3903daf0883875941497d2493c27, got {error_message_or_hash}"
36+                    log_msg=f"[snapshot] bad snapshot content hash: expected a4bf3407ccb2cc0145c49ebba8fa91199f8a3903daf0883875941497d2493c27, got {wrong_hash}"
37                 )
38
39     def test_headers_not_synced(self, valid_snapshot_path):
    jrakibi commented at 1:39 pm on April 22, 2024:
    Done 👍
  36. rkrux approved
  37. rkrux commented at 7:48 am on April 15, 2024: none

    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.

  38. in test/functional/feature_assumeutxo.py:107 in e807838d26 outdated 
    106+            # (content, offset, wrong_hash or message, is_custom_message)
107+            [b"\xff" * 32, 0, "7d52155c9a9fdc4525b637ef6170568e5dad6fabd0b1fdbb9432010b8453095b", False],  # wrong outpoint hash
108+            [(1).to_bytes(4, "little"), 32, "9f4d897031ab8547665b4153317ae2fdbf0130c7840b66427ebc48b881cb80ad", False],  # wrong outpoint index
109+            [b"\x81", 36, "3da966ba9826fb6d2604260e01607b55ba44e1a5de298606b08704bc62570ea8", False],  # wrong coin code VARINT
110+            [b"\x80", 36, "091e893b3ccb4334378709578025356c8bcb0a623f37c7c4e493133c988648e5", False],  # another wrong coin code
111+            [b"\x84\x58", 36, "[snapshot] bad snapshot data after deserializing 0 coins", True],  # wrong coin case with height 364 and coinbase 0
    fjahr commented at 8:19 am on April 15, 2024:
    I think using one field for multiple things makes this more confusing than necessary. Please keep the hash field for the hash and where you have the boolean you can use that field for a custom message. Then you can check if the custom message is set instead of an explicit boolean.
  39. in test/functional/feature_assumeutxo.py:367 in e807838d26 outdated 
    373@@ -367,7 +374,6 @@ def check_tx_counts(final: bool) -> None:
374                 # Ensure indexes have synced for the assumeutxo node
375                 self.wait_until(lambda: n.getindexinfo() == completed_idx_state)
376 
377-
    fjahr commented at 8:19 am on April 15, 2024:
    Unnecessary blank space removal
  40. in test/functional/feature_assumeutxo.py:113 in e807838d26 outdated 
    114 
115-        for content, offset, wrong_hash in cases:
116+        for content, offset, error_message_or_hash, is_custom_message in cases:
117             with open(bad_snapshot_path, "wb") as f:
118-                f.write(valid_snapshot_contents[:(32 + 8 + offset)])
119+                f.write(valid_snapshot_contents[: (32 + 8 + offset)])
    fjahr commented at 8:25 am on April 15, 2024:
    I don’t think we put a space here anywhere else?
  41. fjahr commented at 8:33 am on April 15, 2024: contributor

    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".

  42. jrakibi force-pushed on Apr 22, 2024
  43. jrakibi force-pushed on Apr 22, 2024
  44. jrakibi force-pushed on Apr 22, 2024
  45. jrakibi commented at 1:48 pm on April 22, 2024: contributor
    @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
  46. in test/functional/feature_assumeutxo.py:108 in 6428b363e6 outdated 
    107+            [b"\xff" * 32, 0, "7d52155c9a9fdc4525b637ef6170568e5dad6fabd0b1fdbb9432010b8453095b", None],  # wrong outpoint hash
108+            [(1).to_bytes(4, "little"), 32, "9f4d897031ab8547665b4153317ae2fdbf0130c7840b66427ebc48b881cb80ad", None],  # wrong outpoint index
109+            [b"\x81", 36, "3da966ba9826fb6d2604260e01607b55ba44e1a5de298606b08704bc62570ea8", None],  # wrong coin code VARINT
110+            [b"\x80", 36, "091e893b3ccb4334378709578025356c8bcb0a623f37c7c4e493133c988648e5", None],  # another wrong coin code
111+            [b"\x84\x58", 36, None, "[snapshot] bad snapshot data after deserializing 0 coins"],  # wrong coin case with height 364 and coinbase 0
112+            [b"\xCA\xD2\x8F\x5A", 41, None, "[snapshot] bad snapshot data after deserializing 0 coins - bad tx out value"],  # Amount exceed MAX_MONEY
    fjahr commented at 3:45 pm on April 22, 2024:
    nit: exceeds
    jrakibi commented at 11:28 pm on April 22, 2024:
    Fixed and rebased.
  47. DrahtBot added the label CI failed on Apr 22, 2024
  48. fjahr commented at 4:53 pm on April 22, 2024: contributor

    Code review ACK 6428b363e68fee51a455ebd4f9bf7ed6d813caa8

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

  49. DrahtBot requested review from rkrux on Apr 22, 2024
  50. jrakibi force-pushed on Apr 22, 2024
  51. jrakibi force-pushed on Apr 22, 2024
  52. test:Validate UTXO snapshot with coin_height > base_height & amount > 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
    ec1f1abfef
  53. jrakibi force-pushed on Apr 22, 2024
  54. fjahr commented at 11:35 pm on April 22, 2024: contributor
    re-ACK ec1f1abfefa281e62bb876aa1c4738d576ef9a47
  55. DrahtBot removed the label CI failed on Apr 23, 2024
  56. maflcko commented at 7:33 am on April 23, 2024: member

    ACK ec1f1abfefa281e62bb876aa1c4738d576ef9a4 👑

    Signature:

    0untrusted 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}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: ACK ec1f1abfefa281e62bb876aa1c4738d576ef9a4 👑
3JmyXc4AA3GlfNbDIttEDoTvrUHdJtZaS+AtVKAl4yGV+WmcfcEFyZjxpaa5jgmWqix+7xHNhB/dd8/He/ZgGBg==
  57. achow101 commented at 8:38 pm on May 2, 2024: member
    ACK ec1f1abfefa281e62bb876aa1c4738d576ef9a47
  58. achow101 merged this on May 2, 2024
  59. achow101 closed this on May 2, 2024


jrakibi DrahtBot BrandonOdiwuor ismaelsadeeq maflcko rkrux fjahr achow101


rkrux

Labels
Tests

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-03 10:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me