test: assumeutxo: check that UTXO-querying RPCs operate on snapshot chainstate

theStack commented at 11:52 am on August 12, 2024: contributor

Inspired by some manual testing I did for #28553, this PR checks that RPCs which explicitly query the UTXO set database (i.e. gettxoutsetinfo, scantxoutset and gettxout) operate on the snapshot chainstate as expected.

DrahtBot commented at 11:52 am on August 12, 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, tdb3, achow101
Concept ACK	BrandonOdiwuor

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 Aug 12, 2024

fanquake commented at 12:56 pm on August 12, 2024: member

https://github.com/bitcoin/bitcoin/actions/runs/10351325972/job/28649600800?pr=30636#step:7:23340:

 0 test  2024-08-12T12:48:42.142000Z TestFramework (ERROR): JSONRPC error 
 1                                   Traceback (most recent call last):
 2                                     File "/home/runner/work/_temp/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 132, in main
 3                                       self.run_test()
 4                                     File "/home/runner/work/_temp/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/feature_assumeutxo.py", line 354, in run_test
 5                                       utxo_info = n1.gettxoutsetinfo()
 6                                                   ^^^^^^^^^^^^^^^^^^^^
 7                                     File "/home/runner/work/_temp/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/coverage.py", line 50, in __call__
 8                                       return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
 9                                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
10                                     File "/home/runner/work/_temp/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/authproxy.py", line 146, in __call__
11                                       raise JSONRPCException(response['error'], status)
12                                   test_framework.authproxy.JSONRPCException: Unable to get data because coinstatsindex is still syncing. Current height: 0 (-32603)
13 node1 2024-08-12T12:48:42.142251Z (mocktime: 2011-02-02T23:17:17Z) [scheduler] [validationinterface.cpp:246] [operator()] [validation] ChainStateFlushed: block hash=3bb7ce5eba0be48939b7a521ac1ba9316afee2c7bada3a0cca24188e6d7d96c0 
14 test  2024-08-12T12:48:42.149000Z TestFramework (DEBUG): Closing down network thread

fjahr commented at 1:27 pm on August 12, 2024: contributor

Concept ACK

DrahtBot added the label CI failed on Aug 12, 2024

theStack force-pushed on Aug 12, 2024

in test/functional/feature_assumeutxo.py:362 in 2e9072c137 outdated

357+        assert_equal(utxo_info['height'], loaded['base_height'])
358+        assert_equal(utxo_info['bestblock'], loaded['tip_hash'])
359+
360+        # find coinbase output at snapshot height on node0 and scan for it on node1,
361+        # where the block is not available, but the snapshot was loaded successfully
362+        snapshot_hash = loaded['tip_hash']

fjahr commented at 2:46 pm on August 12, 2024:

nit: could have moved this up and used it in the gettxoutsetinfo as well

fjahr commented at 2:48 pm on August 12, 2024:

On second thought: maybe just pull all three of these loaded keys out in the beginning since you use each of them at least twice and they are used again further below… or you could deduplicate the assert_equal triplet but it’s not a big deal either way…

theStack commented at 10:49 am on August 20, 2024:

Introduced snapshot_hash and snapshot_num_coins in the beginning and used the already existing SNAPSHOT_BASE_HEIGHT for the height.

in test/functional/feature_assumeutxo.py:370 in 2e9072c137 outdated

365+        coinbase_output_descriptor = coinbase_tx['vout'][0]['scriptPubKey']['desc']
366+        scan_result = n1.scantxoutset('start', [coinbase_output_descriptor])
367+        assert_equal(scan_result['success'], True)
368+        assert_equal(scan_result['txouts'], loaded['coins_loaded'])
369+        assert_equal(scan_result['height'], loaded['base_height'])
370+        assert_equal(scan_result['bestblock'], loaded['tip_hash'])

fjahr commented at 2:47 pm on August 12, 2024:

nit

0        assert_equal(scan_result['bestblock'], snapshot_hash)

in test/functional/feature_assumeutxo.py:371 in 2e9072c137 outdated

366+        scan_result = n1.scantxoutset('start', [coinbase_output_descriptor])
367+        assert_equal(scan_result['success'], True)
368+        assert_equal(scan_result['txouts'], loaded['coins_loaded'])
369+        assert_equal(scan_result['height'], loaded['base_height'])
370+        assert_equal(scan_result['bestblock'], loaded['tip_hash'])
371+        assert coinbase_tx['txid'] in [coin['txid'] for coin in scan_result['unspents']]

fjahr commented at 2:49 pm on August 12, 2024:

nit: Would be a bit more readable IMO if you would break it into two lines

fjahr commented at 2:57 pm on August 12, 2024: contributor

Code review ACK 2e9072c137e81c75c58d0c0788295c10fdafdc9b

This is a good regression test to have. I left a bunch of minor nits but this is fine to merge as-is so feel free to leave them unless you have to re-touch.

DrahtBot removed the label CI failed on Aug 12, 2024

tdb3 commented at 1:15 am on August 13, 2024: contributor

Concept ACK

Great test additions. Light code review and manual test. I concur with the nits from @fjahr.

BrandonOdiwuor commented at 12:50 pm on August 14, 2024: contributor

Concept ACK

test: assumeutxo: check that UTXO-querying RPCs operate on snapshot chainstate 917e70a620

theStack force-pushed on Aug 20, 2024

theStack commented at 10:49 am on August 20, 2024: contributor

Thanks for the reviews! Took the suggestions and refined the UTXO check against the scantxoutset result to also include the vout index (=0), rather than only verifying the txid.

fjahr commented at 11:09 am on August 20, 2024: contributor

utACK 917e70a6206c62c4c492fa922425fc8e00d3f328

Only changes since last review were addressing my minor review comments.

DrahtBot requested review from tdb3 on Aug 20, 2024

DrahtBot requested review from BrandonOdiwuor on Aug 20, 2024

tdb3 approved

tdb3 commented at 11:27 am on August 20, 2024: contributor

ACK 917e70a6206c62c4c492fa922425fc8e00d3f328

These tests are great additions. Changes incorporate comments nicely. Ran functionals locally (passed).

maflcko added this to the milestone 28.0 on Aug 20, 2024

achow101 commented at 5:20 pm on August 21, 2024: member

ACK 917e70a6206c62c4c492fa922425fc8e00d3f328

achow101 merged this on Aug 21, 2024

achow101 closed this on Aug 21, 2024

theStack deleted the branch on Aug 21, 2024

Theghost256 approved

test: assumeutxo: check that UTXO-querying RPCs operate on snapshot chainstate #30636

Code Coverage

Reviews