validation: Send correct notification during snapshot completion

mzumsande commented at 8:16 pm on December 23, 2024: contributor

After AssumeUtxo background sync is completed in a ActivateBestChain() call, the GetRole() function called with BlockConnected() returns ChainstateRole::NORMAL instead of ChainstateRole::BACKGROUND for this chainstate. This would make the wallet (which ignores BlockConnected notifications for the background chainstate) process it, change m_last_block_processed_height to the (ancient) snapshot height, and display an incorrect balance.

Fix this by caching the chainstate role before calling ActivateBestChainStep(). Also contains a test for this situation that fails on master.

Fixes #31546

DrahtBot commented at 8:16 pm on December 23, 2024: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31556.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	fjahr, furszy, TheCharlatan, achow101

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30909 (wallet, assumeutxo: Don’t Assume m_chain_tx_count, Improve wallet RPC errors by fjahr)

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.

DrahtBot added the label Validation on Dec 23, 2024

in test/functional/wallet_assumeutxo.py:206 in ca0753160a outdated

197@@ -192,6 +198,18 @@ def run_test(self):
198         w = n1.get_wallet_rpc("w")
199         assert_equal(w.getbalance(), 34)
200 
201+        # Check balance of wallet on node 2 (for which wallet w is active during snapshot completion)
202+        n2.restorewallet("w", "backup_w.dat")
203+        self.log.info(
204+            f"Loading snapshot into third node from {dump_output['path']}")
205+        loaded = n2.loadtxoutset(dump_output['path'])
206+        self.log.info("Ensuring background validation completes")

fjahr commented at 12:04 pm on December 25, 2024:

nit: I prefer to not log the prep work of these tests, only what describes the actual point of the test either at the start or at the check. (It can still be a comment though)

I see that you follow roughly the same style of the logging in the rest of the file but the actually interesting part that sets this test apart from the other tests is in the comment. The logs are just printing rather generic stuff. So I basically revert the logging ad comments in this block. I.e. L201 becomes a log and L203, L206 and L210 become comments.

Not a blocker if you want to just keep the logging consistent.

mzumsande commented at 5:14 pm on December 26, 2024:

Makes sense to me! I think that the code is self-explanatory enough that it’s not necessary to keep the lines as comments, so I just removed them.

in src/validation.cpp:3532 in ca0753160a outdated

3525@@ -3526,6 +3526,10 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr<
3526 
3527                 bool fInvalidFound = false;
3528                 std::shared_ptr<const CBlock> nullBlockPtr;
3529+                // BlockConnected signals must be sent for the original role;
3530+                // in case snapshot validation is completed during ActivateBestChainStep, the
3531+                // result of GetRole() changes from BACKGROUND to NORMAL.
3532+               ChainstateRole chainstate_role{this->GetRole()};

fjahr commented at 1:20 pm on December 25, 2024:

nit: could have been const

mzumsande commented at 5:12 pm on December 26, 2024:

done

in test/functional/wallet_assumeutxo.py:63 in ca0753160a outdated

58@@ -57,6 +59,8 @@ def run_test(self):
59         """
60         n0 = self.nodes[0]
61         n1 = self.nodes[1]
62+        n2 = self.nodes[2]
63+

fjahr commented at 1:24 pm on December 25, 2024:

nit: newline not necessary imo

mzumsande commented at 5:12 pm on December 26, 2024:

removed, that was not on purpose

in test/functional/wallet_assumeutxo.py:205 in ca0753160a outdated

206+        self.log.info("Ensuring background validation completes")
207+        self.connect_nodes(0, 2)
208+        self.wait_until(lambda: len(n2.getchainstates()['chainstates']) == 1)
209+
210+        self.log.info("Check balance at third node")
211+        ensure_for(duration=1, f=lambda: (n2.getbalance() == 34))

fjahr commented at 1:32 pm on December 25, 2024:

I like to see ensure_for getting used but I think I would have opted for self.wait_until(lambda: (n2.getbalance() == 34), timeout=5) or so here. Is there a case I am missing where the balance might change after having been 34 that you are covering with this?

mzumsande commented at 4:46 pm on December 26, 2024:

I used ensure_for because otherwise the added test might not always have failed on master before (if the first check had been done immediately, before the incorrect notification was processed by the wallet, the balance would have still been 34).

fjahr commented at 1:33 pm on December 25, 2024: contributor

tACK ca0753160a8ebc113e08d62c7b6cbe8fa98455e6

Good catch, feel free to ignore my comments unless you have to retouch. I confirmed that the test fails on master and reviewed the code and surrounding behavior.

validation: Send correct notification during snapshot completion

If AssumeUtxo background sync is completed in this
ActivateBestChain() call, the GetRole() function
returns "normal" instead of "background" for this chainstate.
This would make the wallet (which ignores BlockConnected
notifcation for the background chainstate) process it, change
m_last_block_processed_height, and display an incorrect
balance.

226d03dd61

test: add functional test for balance after snapshot completion

Use a third node for this, which doesn't get restarted like the second
node.
This test would fail without the previous commit.

bc43ecaf6d

mzumsande force-pushed on Dec 26, 2024

mzumsande commented at 5:15 pm on December 26, 2024: contributor

ca07531 to bc43eca: Addressed feedback by @fjahr

fjahr commented at 10:16 am on December 27, 2024: contributor

re-ACK bc43ecaf6dc0830a27296d3a29428814fed07bb1

furszy commented at 8:56 pm on December 27, 2024: member

Code review ACK bc43ecaf6dc

in test/functional/wallet_assumeutxo.py:59 in bc43ecaf6d

58@@ -57,6 +59,7 @@ def run_test(self):
59         """

TheCharlatan commented at 2:56 pm on December 28, 2024:

Should the docs be updated to mention the added node?

mzumsande commented at 7:56 pm on December 30, 2024:

missed this, it could probably be done in a follow-up such as #30455

TheCharlatan approved

TheCharlatan commented at 2:58 pm on December 28, 2024: contributor

lgtm ACK bc43ecaf6dc0830a27296d3a29428814fed07bb1

achow101 commented at 7:26 pm on December 30, 2024: member

ACK bc43ecaf6dc0830a27296d3a29428814fed07bb1

achow101 merged this on Dec 30, 2024

achow101 closed this on Dec 30, 2024

mzumsande deleted the branch on Dec 30, 2024

validation: Send correct notification during snapshot completion #31556

Code Coverage & Benchmarks

Reviews

Conflicts