After #33259, getblockchaininfo reports the progress of the background chainstate relative to the snapshot base block, but getchainstates still computes verificationprogress against the network tip for every chainstate, so the same node reports two different numbers for the same chainstate.
This PR modifies verificationprogress value to report the correct value, relative to the block the chainstate actually stops at.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | nervana21 |
| Concept NACK | l0rinc |
| Concept ACK | sedited, vicjuma, musaHaruna |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
Concept ACK
would be interesting to add a new field "targetBlock" to show until which block must validate the background chainstate?
3610 | @@ -3641,7 +3611,8 @@ return RPCMethod{ 3611 | data.pushKV("bits", strprintf("%08x", tip->nBits)); 3612 | data.pushKV("target", GetTarget(*tip, chainman.GetConsensus().powLimit).GetHex()); 3613 | data.pushKV("difficulty", GetDifficulty(*tip)); 3614 | - data.pushKV("verificationprogress", chainman.GuessVerificationProgress(tip)); 3615 | + data.pushKV("verificationprogress", cs.TargetBlock() ? chainman.GetBackgroundVerificationProgress(*tip)
I feel like this change better aligns the assumeutxo.md documentation (accurate monitoring)
After the snapshot has loaded, the syncing process of both the snapshot chain and the background IBD chain can be monitored with the getchainstates RPC.
2e2deb8262f2297af61b152aa7646e3cabcc4f40: rpc: report background validation progress in getchainstates
data.pushKV("verificationprogress", cs.TargetBlock().has_value() ? chainman.GetBackgroundVerificationProgress(*tip)
nit
Thanks for the review! TargetBlock() returns a const CBlockIndex* rather than a std::optional, so I don't think .has_value() would work here.
ahh, you're right. my mistake!
Concept ACK
Concept ACK
tACK d9638e49f6a7ef70dbe31cb85e809d9f81e3ce55
Concept ACK
I’m currently running a node with AssumeUTXO, so I checked this before and after applying the change. My snapshot base is at height 935000.
Before the fix, at background height 474828, the background chainstate’s verificationprogress in getchainstates reported 16.73%, while backgroundvalidation.verificationprogress in getblockchaininfo reported 18.22% for the same block. After the fix, at height 475948, both reported exactly 0.1834620809971763. This is a good improvement: getchainstates now reports background progress toward the snapshot base, as getblockchaininfo already does.
Locally, the full functional test passed. Reverting only the RPC fix made the background-progress assertion fail; restoring it made the full test pass again.
564 | @@ -565,6 +565,10 @@ def check_dump_output(output): 565 | expected_verification_progress = background_tx_count / snapshot_tx_count 566 | assert_approx(bv_res["verificationprogress"], expected_verification_progress, vspan=0.01) 567 | 568 | + self.log.info("Check that getchainstates returns the background validation progress") 569 | + cs_res = n1.getchainstates()["chainstates"][0] 570 | + assert_approx(cs_res["verificationprogress"], expected_verification_progress, vspan=0.01) 571 | +
background, active = n1.getchainstates()["chainstates"]
assert_approx(background["verificationprogress"], expected_verification_progress)
self.log.info("Check that getchainstates returns the active chainstate verification progress")
assert_approx(active["verificationprogress"], res["verificationprogress"])
Optional suggestion: could we also check the active chainstate’s verificationprogress against the top-level value from getblockchaininfo? Both chainstates are already available in the test, so this would also check that active-chainstate progress is still reported correctly.
I understand this fixes an existing reporting inconsistency, but I’d prefer us to revisit the underlying synchronization design and costs discussed in #36476, rather than keep adding special cases around background validation. I think we should discuss that broader direction before extending this approach further. Concept NACK on the direction of endless patches for a broken feature.