test: Test improvements for UTXO set hash tests #21390

fjahr commented at 8:17 PM on March 8, 2021: member

Follow-ups to #19145:

Small improvement on the help text of RPC gettxoutsetinfo
Using deterministic blockchain in the test functional/feature_utxo_set_hash.py
Removing wallet dependency in the test functional/feature_utxo_set_hash.py

Split out of #19521.

fjahr force-pushed on Mar 8, 2021

DrahtBot added the label RPC/REST/ZMQ on Mar 8, 2021

DrahtBot commented at 4:27 AM on March 9, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21144 (test: convert feature_bip68_sequence.py to use MiniWallet by danben)

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.

brunoerg commented at 11:45 PM on March 9, 2021: member

tACK 7a1af9036d1579bae7f70512de37d842372f4419 MacOS 11.1

Screen Shot 2021-03-09 at 10 07 18

leonardojobim approved

leonardojobim commented at 4:41 AM on March 10, 2021: none

Tested ACK https://github.com/bitcoin/bitcoin/pull/21390/commits/f33a9f7a7648c7af6fdc9a1549ab648c7498e45b on Ubuntu 20.04.

jonatack commented at 11:41 AM on March 23, 2021: member

Concept ACK. Revewing and testing #19521, then reviewing this patch.

in src/node/coinstats.cpp:18 in f33a9f7a76 outdated

  14 | @@ -15,6 +15,7 @@
  15 |  
  16 |  #include <map>
  17 |  
  18 | +// Database-independent, meaningless metric indicating the UTXO set size

MarcoFalke commented at 2:23 PM on March 23, 2021:

// Database-independent metric indicating the UTXO set size

nit: It means to represent the size, so not meaningless

fjahr commented at 7:50 PM on March 23, 2021:

done

in test/functional/feature_utxo_set_hash.py:35 in f33a9f7a76 outdated

  33 | +        blocks = node.generatetoaddress(10, node.get_deterministic_priv_key().address)
  34 | +
  35 | +        assert_equal(node.gettxoutsetinfo()['hash_serialized_2'], "9411e505b12619bc0d464ead3c2a14823f53d288380adf3742c7f86b64e3d6f1")
  36 | +        assert_equal(node.gettxoutsetinfo("muhash")['muhash'], "75d363bf768ebe52102613792ceeb345364874b86f4579eb8030ff835799ecc9")
  37 | +
  38 | +        node.invalidateblock(blocks[0])

MarcoFalke commented at 2:41 PM on March 23, 2021:

Any reason to test determinism separate from consistency? It should be ok to just move the assert_equal down a bit:

diff --git a/test/functional/feature_utxo_set_hash.py b/test/functional/feature_utxo_set_hash.py
index 233a0ccec7..d25803d883 100755
--- a/test/functional/feature_utxo_set_hash.py
+++ b/test/functional/feature_utxo_set_hash.py
@@ -25,20 +25,9 @@ class UTXOSetHashTest(BitcoinTestFramework):
         self.log.info("Test deterministic UTXO set hash results")
 
         node = self.nodes[0]
+        wallet = MiniWallet(node)
         mocktime = node.getblockheader(node.getblockhash(0))['time'] + 1
         node.setmocktime(mocktime)
-        blocks = node.generatetoaddress(10, node.get_deterministic_priv_key().address)
-
-        assert_equal(node.gettxoutsetinfo()['hash_serialized_2'], "9411e505b12619bc0d464ead3c2a14823f53d288380adf3742c7f86b64e3d6f1")
-        assert_equal(node.gettxoutsetinfo("muhash")['muhash'], "75d363bf768ebe52102613792ceeb345364874b86f4579eb8030ff835799ecc9")
-
-        node.invalidateblock(blocks[0])
-
-    def test_muhash_implementation(self):
-        self.log.info("Test MuHash implementation consistency")
-
-        node = self.nodes[0]
-        wallet = MiniWallet(node)
 
         # Generate 100 blocks and remove the first since we plan to spend its
         # coinbase
@@ -78,10 +67,11 @@ class UTXOSetHashTest(BitcoinTestFramework):
         node_muhash = node.gettxoutsetinfo("muhash")['muhash']
 
         assert_equal(finalized[::-1].hex(), node_muhash)
+        assert_equal(node.gettxoutsetinfo()['hash_serialized_2'], "5b1b44097406226c0eb8e1362cd17a1f346522cf9390a8175a57a5262cb1963f")
+        assert_equal(node.gettxoutsetinfo("muhash")['muhash'], "4b8803075d7151d06fad3e88b68ba726886794873fbfa841d12aefb2cc2b881b")
 
     def run_test(self):
         self.test_deterministic_hash_results()
-        self.test_muhash_implementation()
 
 
 if __name__ == '__main__':

fjahr commented at 7:50 PM on March 23, 2021:

makes sense, done

MarcoFalke approved

MarcoFalke commented at 2:41 PM on March 23, 2021: member

review ACK d7a679302401395501840cfdf8645e41c59189ec 🚸

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash 77191ae7c24a373f7b4fa14a63f9a8762f9c8c844c6f84b706a595b934dc11c2 -

</details>

MarcoFalke commented at 2:43 PM on March 23, 2021: member

fun fact: This has three "ACKs", but only one is mentioning the correct commit hash.

MarcoFalke commented at 2:43 PM on March 23, 2021: member

review ACK f33a9f7a7648c7af6fdc9a1549ab648c7498e45b 🔰

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash 4a2e7bb742ec362fb28376a4b87efe766247e29a659fb64940354a2212c8b214 -

</details>

rpc: Improve gettxoutsetinfo help 1a27af1d7b

test: Remove wallet dependency of utxo set hash test 4973c5175c

test: Use deterministic chain in utxo set hash test

Also melts the previously separated test cases into one.

4f2653a890

fjahr force-pushed on Mar 23, 2021

fjahr commented at 7:51 PM on March 23, 2021: member

Took @MarcoFalke 's suggestions

MarcoFalke commented at 6:39 AM on March 24, 2021: member

review ACK 4f2653a89018fa4d24bd2a551832a7410b682600 👲

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash ce3e75ebeace1e189987faeb3cfec5408339b7c7faa321bac4f4435d1dd5e06c -

</details>

MarcoFalke merged this on Mar 26, 2021

MarcoFalke closed this on Mar 26, 2021

sidhujag referenced this in commit a571a3e6a8 on Mar 26, 2021

DrahtBot locked this on Aug 16, 2022