assumeutxo: change getchainstates RPC to return a list of chainstates #28590

ryanofsky · 2023-10-04T15:46:36Z

Current getchainstates RPC returns "normal" and "snapshot" fields which are not ideal because it requires new "normal" and "snapshot" terms to be defined, and the definitions are not really consistent with internal code. (In the RPC interface, the "snapshot" chainstate becomes the "normal" chainstate after it is validated, while in internal code there is no "normal chainstate" and the "snapshot chainstate" is still called that temporarily after it is validated).

The current getchainstates RPC is also awkward to use if you to want information about the most-work chainstate, because you have to look at the "snapshot" field if it exists, and otherwise fall back to the "normal" field.

Fix these issues by having getchainstates just return a flat list of chainstates ordered by work, and adding a new chainstate "validated" field alongside the existing "snapshot_blockhash" field so it is explicit if a chainstate was originally loaded from a snapshot, and whether the snapshot has been validated.

This change was motivated by comment thread in #28562 (comment)

DrahtBot · 2023-10-04T15:46:38Z

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, jamesob, achow101
Concept ACK	fjahr

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

fjahr · 2023-10-04T15:51:29Z

Concept ACK, would be good to have this in 26.0 so we don't change the RPC interface afterwards

jamesob · 2023-10-04T15:56:41Z

Concept ACK, seems like a fine simplification!

ryanofsky · 2023-10-04T18:54:43Z

Rebased 3306714 -> ff89dec (pr/getchain.1 -> pr/getchain.2, compare) due to conflict with #28589

Sjors · 2023-10-05T08:30:48Z

Concept ACK

tACK ff89dec

Tested with a signet IBD, loading the snapshot and waiting for background sync to finish.

jamesob

ACK ff89dec (jamesob/ackr/28590.1.ryanofsky.assumeutxo_change_getcha)

Good simplification, thanks. Built, ran tests locally.

jamesob · 2023-10-05T12:58:58Z

Ah, just had a functional test fail while running in a loop. Investigating...

./test/functional/feature_assumeutxo.py  6.94s user 0.41s system 99% cpu 7.380 total
2023-10-05T12:58:59.161000Z TestFramework (INFO): PRNG seed is: 6200617450151020946
2023-10-05T12:58:59.161000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_smviasai
2023-10-05T12:58:59.605000Z TestFramework (INFO): -- Testing assumeutxo + some indexes + pruning
2023-10-05T12:58:59.605000Z TestFramework (INFO): Creating a UTXO snapshot at height 299
2023-10-05T12:58:59.655000Z TestFramework (INFO): Loading snapshot into second node from /tmp/bitcoin_func_test_smviasai/node0/regtest/utxos.dat
2023-10-05T12:58:59.658000Z TestFramework (INFO): Restarting node to stop at height 359
2023-10-05T12:59:00.111000Z TestFramework (INFO): Checking that blocks are segmented on disk
2023-10-05T12:59:00.111000Z TestFramework (INFO): Restarted node before snapshot validation completed, reloading...
2023-10-05T12:59:00.418000Z TestFramework (INFO): Ensuring snapshot chain syncs to tip. (399)
2023-10-05T12:59:00.470000Z TestFramework (INFO): Ensuring background validation completes
2023-10-05T13:00:00.480000Z TestFramework.utils (ERROR): wait_until() failed. Predicate: ''''
        wait_until_helper(lambda: len(n1.getchainstates()['chainstates']) != 1)
'''
2023-10-05T13:00:00.480000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/home/james/src/bitcoin/test/functional/test_framework/test_framework.py", line 131, in main
    self.run_test()
  File "/home/james/src/bitcoin/./test/functional/feature_assumeutxo.py", line 170, in run_test
    wait_until_helper(lambda: len(n1.getchainstates()['chainstates']) != 1)
  File "/home/james/src/bitcoin/test/functional/test_framework/util.py", line 276, in wait_until_helper
    raise AssertionError("Predicate {} not true after {} seconds".format(predicate_source, timeout))
AssertionError: Predicate ''''
        wait_until_helper(lambda: len(n1.getchainstates()['chainstates']) != 1)
''' not true after 60.0 seconds
2023-10-05T13:00:00.531000Z TestFramework (INFO): Stopping nodes
2023-10-05T13:00:00.633000Z TestFramework (WARNING): Not cleaning up dir /tmp/bitcoin_func_test_smviasai

ryanofsky · 2023-10-05T13:01:01Z

Tangent, but @DrahtBot does not seem to pick up Sjors ACK with "Concept ACK" on the first line and "tACK" on the second line. I guess the ACK detector is overloaded

jamesob · 2023-10-05T13:03:27Z

test/functional/feature_assumeutxo.py

        self.sync_blocks(nodes=(n0, n1))

        self.log.info("Ensuring background validation completes")
        # N.B.: the `snapshot` key disappears once the background validation is complete.
-        wait_until_helper(lambda: not n1.getchainstates().get('snapshot'))
+        wait_until_helper(lambda: len(n1.getchainstates()['chainstates']) != 1)


I'm not sure I follow this substitution... once background validation completes, chainman.GetAll() should (at least eventually) return only one chainstate: the fully-validated snapshot chainstate. Maybe s/!=/==?

Funny, I think as-written this succeeds most of the time because the check is racily performed before validation actually completes, which essentially makes the check exit early.

re: #28590 (comment)

Funny, I think as-written this succeeds most of the time because the check is racily performed before validation actually completes, which essentially makes the check exit early.

Right, this is the opposite of the condition you'd expect. Probably I just wrote the check one way, then inverted when it failed assuming I'd made a mistake. But either of these checks are racy, so it makes more sense to be direct and check the new "validated" field. Updated in new push.

re: #28590 (comment)

Actually thinking about it more, the change you suggested is not racy, and is a little better than just checking the "validated" field because it also ensures that the background chainstate disappears from the list after it is disabled. Also check you suggested is more consistent with the check done for the n2 node below on line 218

jamesob · 2023-10-05T13:05:10Z

I'm ACK again once this change is made: #28590 (comment)

ryanofsky

Updated ff89dec -> 35f6d90 (pr/getchain.2 -> pr/getchain.3, compare) fixing race condition in test

ryanofsky · 2023-10-05T13:06:27Z

test/functional/feature_assumeutxo.py

        self.sync_blocks(nodes=(n0, n1))

        self.log.info("Ensuring background validation completes")
        # N.B.: the `snapshot` key disappears once the background validation is complete.
-        wait_until_helper(lambda: not n1.getchainstates().get('snapshot'))
+        wait_until_helper(lambda: len(n1.getchainstates()['chainstates']) != 1)


re: #28590 (comment)

Funny, I think as-written this succeeds most of the time because the check is racily performed before validation actually completes, which essentially makes the check exit early.

Right, this is the opposite of the condition you'd expect. Probably I just wrote the check one way, then inverted when it failed assuming I'd made a mistake. But either of these checks are racy, so it makes more sense to be direct and check the new "validated" field. Updated in new push.

Current getchainstates RPC returns "normal" and "snapshot" fields which are not ideal because it requires new "normal" and "snapshot" terms to be defined, and the definitions are not really consistent with internal code. (In the RPC interface, the "snapshot" chainstate becomes the "normal" chainstate after it is validated, while in internal code there is no "normal chainstate" and the "snapshot chainstate" is still called that temporarily after it is validated). The current getchainstatees RPC is also awkward to use if you to want information about the most-work chainstate because you have to look at the "snapshot" field if it exists, and otherwise fall back to the "normal" field. Fix these issues by having getchainstates just return a flat list of chainstates ordered by work, and adding new chainstate "validated" field alongside the existing "snapshot_blockhash" so it is explicit if a chainstate was originally loaded from a snapshot, and whether the snapshot has been validated.

ryanofsky

Updated 35f6d90 -> a9ef702 (pr/getchain.3 -> pr/getchain.4, compare) taking James suggestion and making test a little stricter and more internally consistent

ryanofsky · 2023-10-05T13:38:36Z

test/functional/feature_assumeutxo.py

        self.sync_blocks(nodes=(n0, n1))

        self.log.info("Ensuring background validation completes")
        # N.B.: the `snapshot` key disappears once the background validation is complete.
-        wait_until_helper(lambda: not n1.getchainstates().get('snapshot'))
+        wait_until_helper(lambda: len(n1.getchainstates()['chainstates']) != 1)


re: #28590 (comment)

Actually thinking about it more, the change you suggested is not racy, and is a little better than just checking the "validated" field because it also ensures that the background chainstate disappears from the list after it is disabled. Also check you suggested is more consistent with the check done for the n2 node below on line 218

Sjors · 2023-10-05T15:15:10Z

re-ACK a9ef702

Only a test change since I last reviewed, which I didn't study closely.

jamesob · 2023-10-05T15:17:49Z

re-ACK a9ef702

achow101 · 2023-10-05T18:05:49Z

ACK a9ef702

…a list of chainstates a9ef702 assumeutxo: change getchainstates RPC to return a list of chainstates (Ryan Ofsky) Pull request description: Current `getchainstates` RPC returns "normal" and "snapshot" fields which are not ideal because it requires new "normal" and "snapshot" terms to be defined, and the definitions are not really consistent with internal code. (In the RPC interface, the "snapshot" chainstate becomes the "normal" chainstate after it is validated, while in internal code there is no "normal chainstate" and the "snapshot chainstate" is still called that temporarily after it is validated). The current `getchainstates` RPC is also awkward to use if you to want information about the most-work chainstate, because you have to look at the "snapshot" field if it exists, and otherwise fall back to the "normal" field. Fix these issues by having `getchainstates` just return a flat list of chainstates ordered by work, and adding a new chainstate "validated" field alongside the existing "snapshot_blockhash" field so it is explicit if a chainstate was originally loaded from a snapshot, and whether the snapshot has been validated. This change was motivated by comment thread in bitcoin#28562 (comment) ACKs for top commit: Sjors: re-ACK a9ef702 jamesob: re-ACK a9ef702 achow101: ACK a9ef702 Tree-SHA512: b364e2e96675fb7beaaee60c4dff4b69e6bc2d8a30dea1ba094265633d1cddf9dbf1c5ce20c07d6e23222cf1e92a195acf6227e4901f3962e81a1e53a43490aa

ryanofsky mentioned this pull request Oct 4, 2023

test: assumeutxo func test race fixes #28589

Merged

fanquake added this to the 26.0 milestone Oct 4, 2023

ryanofsky mentioned this pull request Oct 4, 2023

AssumeUTXO follow-ups #28562

Merged

DrahtBot added the Needs rebase label Oct 4, 2023

ryanofsky force-pushed the pr/getchain branch from 3306714 to ff89dec Compare October 4, 2023 18:53

DrahtBot added the CI failed label Oct 4, 2023

DrahtBot removed Needs rebase CI failed labels Oct 4, 2023

jamesob approved these changes Oct 5, 2023

View reviewed changes

jamesob reviewed Oct 5, 2023

View reviewed changes

ryanofsky force-pushed the pr/getchain branch from ff89dec to 35f6d90 Compare October 5, 2023 13:23

ryanofsky commented Oct 5, 2023

View reviewed changes

ryanofsky force-pushed the pr/getchain branch from 35f6d90 to a9ef702 Compare October 5, 2023 13:42

ryanofsky commented Oct 5, 2023

View reviewed changes

DrahtBot requested a review from jamesob October 5, 2023 15:15

DrahtBot removed the request for review from jamesob October 5, 2023 15:17

achow101 merged commit 0b2c93b into bitcoin:master Oct 5, 2023
13 of 16 checks passed

fjahr mentioned this pull request Oct 6, 2023

assumeutxo (2) #27596

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

assumeutxo: change getchainstates RPC to return a list of chainstates #28590

assumeutxo: change getchainstates RPC to return a list of chainstates #28590

ryanofsky commented Oct 4, 2023

DrahtBot commented Oct 4, 2023 •

edited

fjahr commented Oct 4, 2023

jamesob commented Oct 4, 2023

ryanofsky commented Oct 4, 2023

Sjors commented Oct 5, 2023 •

edited

jamesob left a comment

jamesob commented Oct 5, 2023

ryanofsky commented Oct 5, 2023

jamesob Oct 5, 2023

jamesob Oct 5, 2023

ryanofsky Oct 5, 2023 •

edited

ryanofsky Oct 5, 2023

jamesob commented Oct 5, 2023

ryanofsky left a comment

ryanofsky Oct 5, 2023 •

edited

ryanofsky left a comment

ryanofsky Oct 5, 2023

Sjors commented Oct 5, 2023

jamesob commented Oct 5, 2023

achow101 commented Oct 5, 2023

assumeutxo: change getchainstates RPC to return a list of chainstates #28590

assumeutxo: change getchainstates RPC to return a list of chainstates #28590

Conversation

ryanofsky commented Oct 4, 2023

DrahtBot commented Oct 4, 2023 • edited

Code Coverage

Reviews

fjahr commented Oct 4, 2023

jamesob commented Oct 4, 2023

ryanofsky commented Oct 4, 2023

Sjors commented Oct 5, 2023 • edited

jamesob left a comment

Choose a reason for hiding this comment

jamesob commented Oct 5, 2023

ryanofsky commented Oct 5, 2023

jamesob Oct 5, 2023

Choose a reason for hiding this comment

jamesob Oct 5, 2023

Choose a reason for hiding this comment

ryanofsky Oct 5, 2023 • edited

Choose a reason for hiding this comment

ryanofsky Oct 5, 2023

Choose a reason for hiding this comment

jamesob commented Oct 5, 2023

ryanofsky left a comment

Choose a reason for hiding this comment

ryanofsky Oct 5, 2023 • edited

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

ryanofsky Oct 5, 2023

Choose a reason for hiding this comment

Sjors commented Oct 5, 2023

jamesob commented Oct 5, 2023

achow101 commented Oct 5, 2023

DrahtBot commented Oct 4, 2023 •

edited

Sjors commented Oct 5, 2023 •

edited

ryanofsky Oct 5, 2023 •

edited

ryanofsky Oct 5, 2023 •

edited