refactor: Remove unused nchaintx from SnapshotMetadata constructor, fix test, add test #28639

1. 
   maflcko commented at 9:16 am on October 12, 2023: member

   See commit messages
2. refactor: Remove unused nchaintx from SnapshotMetadata constructor

   Also, remove wrong nChainTx comment and cast.

   faa90f6e7b
3. test: Check snapshot file with wrong number of coins

   Also, fix a bug in an assert_debug_log call.

   fafde92f84
4. 
   DrahtBot commented at 9:16 am on October 12, 2023: 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	Sjors, theStack

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

5. DrahtBot renamed this:
   refactor: Remove unused nchaintx from SnapshotMetadata constructor, fix test, add test
   refactor: Remove unused nchaintx from SnapshotMetadata constructor, fix test, add test
   on Oct 12, 2023
6. DrahtBot added the label Refactoring on Oct 12, 2023
7. in src/rpc/blockchain.cpp:2697 in faa90f6e7b outdated

   2693@@ -2694,9 +2694,7 @@ UniValue CreateUTXOSnapshot(
   2694     result.pushKV("base_height", tip->nHeight);
   2695     result.pushKV("path", path.u8string());
   2696     result.pushKV("txoutset_hash", maybe_stats->hashSerialized.ToString());
   2697-    // Cast required because univalue doesn't have serialization specified for
   2698-    // `unsigned int`, nChainTx's type.
   2699-    result.pushKV("nchaintx", uint64_t{tip->nChainTx});
   2700+    result.pushKV("nchaintx", tip->nChainTx);
   

   ---

   ---

   

   Sjors commented at 9:55 am on October 12, 2023:

   faa90f6e7b6b8c531e1ae142a5c2f568b48502a9: was this always wrong or is there some recent change to UniValue?

   ---

   

   maflcko commented at 9:58 am on October 12, 2023:

   https://github.com/bitcoin/bitcoin/commit/fa23c197509f692a815193acc1b50bad2fcbedfe
8. in test/functional/feature_assumeutxo.py:85 in fafde92f84

   83             # block hash of the snapshot base is stored right at the start (first 32 bytes)
   84             f.write(bytes.fromhex(bad_snapshot_block_hash)[::-1] + valid_snapshot_contents[32:])
   85 
   86         expected_log = f"assumeutxo height in snapshot metadata not recognized ({bad_snapshot_height}) - refusing to load snapshot"
   87-        with self.nodes[1].assert_debug_log(expected_log):
   88+        with self.nodes[1].assert_debug_log([expected_log]):
   

   ---

   ---

   

   Sjors commented at 10:05 am on October 12, 2023:

   fafde92f84fb7c245bc3c1cd946a32c891861e5e: I verified this test indeed would pass regardless of expected_log. Wen type safety?
9. 
   Sjors commented at 10:07 am on October 12, 2023: member

   utACK fafde92f84fb7c245bc3c1cd946a32c891861e5e
10. in test/functional/feature_assumeutxo.py:89 in fafde92f84

    87-        with self.nodes[1].assert_debug_log(expected_log):
    88+        with self.nodes[1].assert_debug_log([expected_log]):
    89             assert_raises_rpc_error(-32603, "Unable to load UTXO snapshot", self.nodes[1].loadtxoutset, bad_snapshot_path)
    90 
    91+        self.log.info("  - snapshot file with wrong number of coins")
    92+        valid_num_coins = struct.unpack("<I", valid_snapshot_contents[32:32 + 4])[0]
    

    ---

    ---

    

    theStack commented at 1:19 pm on October 12, 2023:

    nit: could do the same without the struct module by using int.from_bytes (and int.to_bytes below):

    0        valid_num_coins = int.from_bytes(valid_snapshot_contents[32:32 + 4], 'little')
    

    (mostly a matter of taste I guess though)

    ---

    

    maflcko commented at 1:28 pm on October 12, 2023:

    Thanks, will do in a follow-up

    ---

    

    maflcko commented at 10:18 am on October 13, 2023:

    Also, 4 bytes is wrong. The count is u64, which is closer to 8 bytes.

    ---

    

    maflcko commented at 10:19 am on October 13, 2023:

    Fixed both in https://github.com/bitcoin/bitcoin/pull/28647
11. theStack approved
12. 
    theStack commented at 1:21 pm on October 12, 2023: contributor

    ACK fafde92f84fb7c245bc3c1cd946a32c891861e5e

    Thanks for fixing the sneaky assert_debug_log bug.

13. achow101 added this to the milestone 26.0 on Oct 12, 2023
14. fanquake requested review from jamesob on Oct 12, 2023
15. maflcko removed this from the milestone 26.0 on Oct 12, 2023
16. maflcko added this to the milestone 26.0 on Oct 12, 2023
17. fanquake merged this on Oct 13, 2023
18. fanquake closed this on Oct 13, 2023

    ---

19. maflcko deleted the branch on Oct 13, 2023

Contributors
maflcko DrahtBot Sjors theStack

Review Requested
jamesob

Labels
Refactoring

Milestone
26.0