assumeutxo: change getchainstates RPC to return a list of chainstates

ryanofsky commented at 3:46 pm on October 4, 2023: contributor

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 (review)

DrahtBot commented at 3:46 pm on October 4, 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, 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 commented at 3:51 pm on October 4, 2023: contributor

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

jamesob commented at 3:56 pm on October 4, 2023: member

Concept ACK, seems like a fine simplification!

fanquake added this to the milestone 26.0 on Oct 4, 2023

DrahtBot added the label Needs rebase on Oct 4, 2023

ryanofsky force-pushed on Oct 4, 2023

DrahtBot added the label CI failed on Oct 4, 2023

ryanofsky commented at 6:54 pm on October 4, 2023: contributor

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

DrahtBot removed the label Needs rebase on Oct 4, 2023

DrahtBot removed the label CI failed on Oct 5, 2023

Sjors commented at 8:30 am on October 5, 2023: member

Concept ACK

tACK ff89dec6e65ac085ccfd000e147c801ca7a2138a

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

jamesob approved

jamesob commented at 12:57 pm on October 5, 2023: member

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

Good simplification, thanks. Built, ran tests locally.

jamesob commented at 12:58 pm on October 5, 2023: member

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

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

ryanofsky commented at 1:01 pm on October 5, 2023: contributor

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

in test/functional/feature_assumeutxo.py:170 in ff89dec6e6 outdated

175         self.sync_blocks(nodes=(n0, n1))
176 
177         self.log.info("Ensuring background validation completes")
178         # N.B.: the `snapshot` key disappears once the background validation is complete.
179-        wait_until_helper(lambda: not n1.getchainstates().get('snapshot'))
180+        wait_until_helper(lambda: len(n1.getchainstates()['chainstates']) != 1)

jamesob commented at 1:03 pm on October 5, 2023:

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/!=/==?

jamesob commented at 1:04 pm on October 5, 2023:

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.

ryanofsky commented at 1:06 pm on October 5, 2023:

re: #28590 (review)

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.

ryanofsky commented at 1:38 pm on October 5, 2023:

re: #28590 (review)

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 commented at 1:05 pm on October 5, 2023: member

I’m ACK again once this change is made: #28590 (review)

ryanofsky force-pushed on Oct 5, 2023

ryanofsky commented at 1:24 pm on October 5, 2023: contributor

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

assumeutxo: change getchainstates RPC to return a list of chainstates

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.

a9ef702a87

ryanofsky force-pushed on Oct 5, 2023

ryanofsky commented at 1:43 pm on October 5, 2023: contributor

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

Sjors commented at 3:15 pm on October 5, 2023: member

re-ACK a9ef702a877a964bac724a56e2c0b5bee4ea7586

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

DrahtBot requested review from jamesob on Oct 5, 2023

jamesob commented at 3:17 pm on October 5, 2023: member

re-ACK a9ef702

DrahtBot removed review request from jamesob on Oct 5, 2023

achow101 commented at 6:05 pm on October 5, 2023: member

ACK a9ef702a877a964bac724a56e2c0b5bee4ea7586

achow101 merged this on Oct 5, 2023

achow101 closed this on Oct 5, 2023

Frank-GER referenced this in commit 5c83247e86 on Oct 13, 2023

bitcoin locked this on Oct 4, 2024

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

Code Coverage

Reviews