assumeutxo: Don't load a snapshot if it's not in the best header chain

mzumsande commented at 3:34 pm on June 21, 2024: contributor

This was suggested by me in the discussion of #30288, which has more context.

If the snapshot is not an ancestor of the most-work header (m_best_header), syncing from that alternative chain leading to m_best_header should be prioritised. Therefore it’s not useful loading the snapshot in this situation. If the other chain turns out to be invalid or the chain with the snapshot retrieves additional headers so that it’s the most-work one again (see functional test), m_best_header will change and loading the snapshot will be possible again.

Because of the work required to generate a conflicting headers chain, a situation with two conflicting chains should only be possible under extreme circumstances, such as major forks.

DrahtBot commented at 3:34 pm on June 21, 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, alfonsoromanz, 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:

#30403 (test, assumeutxo: Remove resolved todo comments and add new test 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.

in src/validation.cpp:5685 in 055af4d137 outdated

5675@@ -5676,6 +5676,9 @@ util::Result<void> ChainstateManager::ActivateSnapshot(
5676             return util::Error{strprintf(_("The base block header (%s) must appear in the headers chain. Make sure all headers are syncing, and call loadtxoutset again."),
5677                           base_blockhash.ToString())};
5678         }
5679+        if (!m_best_header || m_best_header->GetAncestor(base_blockheight) != snapshot_start_block) {
5680+            return util::Error{_("A forked headers-chain with more work than the chain with the base block header exists. Please proceed to sync without AssumeUtxo.")};

fjahr commented at 3:39 pm on June 22, 2024:

nit: Not sure if users understand what “base block” is. Maybe make it “snapshot base block”.

mzumsande commented at 5:44 pm on June 24, 2024:

will change to that when taking it out of draft.

fjahr commented at 3:42 pm on June 22, 2024: contributor

Concept ACK

DrahtBot added the label Needs rebase on Jul 2, 2024

mzumsande force-pushed on Jul 3, 2024

mzumsande marked this as ready for review on Jul 3, 2024

mzumsande commented at 8:11 pm on July 3, 2024: contributor

After merge of #30267: Rebased, addressed nit by fjahr, and marked as ready for review.

DrahtBot removed the label Needs rebase on Jul 3, 2024

in test/functional/feature_assumeutxo.py:232 in 29ab88dfe1 outdated

227+        node0 = self.nodes[0]
228+        node1 = self.nodes[1]
229+        # create two blocks that form a longer chain
230+        parent = node0.getblockhash(SNAPSHOT_BASE_HEIGHT - 1)
231+        block_time = node0.getblock(node0.getbestblockhash())['time'] + 1
232+        fork1 = create_block(int(parent, 16), create_coinbase(SNAPSHOT_BASE_HEIGHT - 1), block_time)

fjahr commented at 12:46 pm on July 4, 2024:

Huh, either I am confused or something isn’t right here: The parent above is at height SNAPSHOT_BASE_HEIGHT - 1 and now the child of this parent has a coinbase also at height SNAPSHOT_BASE_HEIGHT - 1? It seems this isn’t validated so it’s not a huge issue though.

mzumsande commented at 5:20 pm on July 8, 2024:

Good catch! Yes, since the full blocks are never being submitted to any node (just the headers to node 1), nothing failed. Fixed now.

in test/functional/feature_assumeutxo.py:229 in 29ab88dfe1 outdated

221@@ -218,6 +222,27 @@ def test_snapshot_block_invalidated(self, dump_output_path):
222             assert_raises_rpc_error(-32603, msg, node.loadtxoutset, dump_output_path)
223             node.reconsiderblock(block_hash)
224 
225+    def test_snapshot_not_on_most_work_chain(self, dump_output_path):
226+        self.log.info("Test bitcoind should fail when the node knows the headers of another chain with more work.")
227+        node0 = self.nodes[0]
228+        node1 = self.nodes[1]
229+        # create two blocks that form a longer chain

fjahr commented at 12:48 pm on July 4, 2024:

nit: Comment could be a little more detailed

0# On node 0, create an alternative chain containing 2 new blocks, forking off the main chain at the block before the snapshot block. This simulates a longer chain than the main chain when submitting these two block headers to node 1 because it is only aware of the main chain headers up to the snapshot height.

mzumsande commented at 5:22 pm on July 8, 2024:

Done, in a slightly modified way. The alternative chain is just created in python, node 0 is only used to fetch the previous block it builds on.

in test/functional/feature_assumeutxo.py:241 in 29ab88dfe1 outdated

236+        node1.submitheader(fork1.serialize().hex())
237+        node1.submitheader(fork2.serialize().hex())
238+        msg = "A forked headers-chain with more work than the chain with the snapshot base block header exists. Please proceed to sync without AssumeUtxo."
239+        assert_raises_rpc_error(-32603, msg, node1.loadtxoutset, dump_output_path)
240+        # Cleanup: submit two more headers of the snapshot chain, so that loading the snapshot in future subtests succeeds
241+        main1 = node0.getblock(node0.getblockhash(SNAPSHOT_BASE_HEIGHT + 1), 0)

fjahr commented at 12:55 pm on July 4, 2024:

nit: making the variables a bit more detailed could make the test a bit easier to parse

parent = > parent_block_hash fork1 => fork_block1 main1 = main_block1

mzumsande commented at 5:22 pm on July 8, 2024:

took those

fjahr commented at 12:57 pm on July 4, 2024: contributor

tACK 29ab88dfe1c43af3620480c99cba844aa414023c

I reviewed the code and confirmed that the test fails as expected when the behavior change is not applied.

DrahtBot added the label Needs rebase on Jul 4, 2024

in test/functional/feature_assumeutxo.py:226 in 29ab88dfe1 outdated

221@@ -218,6 +222,27 @@ def test_snapshot_block_invalidated(self, dump_output_path):
222             assert_raises_rpc_error(-32603, msg, node.loadtxoutset, dump_output_path)
223             node.reconsiderblock(block_hash)
224 
225+    def test_snapshot_not_on_most_work_chain(self, dump_output_path):
226+        self.log.info("Test bitcoind should fail when the node knows the headers of another chain with more work.")

fjahr commented at 2:48 pm on July 4, 2024:

I think this test also actually resolves the TODOs that https://github.com/bitcoin/bitcoin/pull/29996/commits/556ff68cea1dfa6c444ef9c920dd0ce52ecc9668 aimed to resolve. I will elaborate more there.

fjahr commented at 7:25 pm on July 5, 2024:

Can remove the TODO Valid snapshot file and snapshot block, but the block is not on the most-work chain here now as well.

fjahr commented at 11:22 am on July 8, 2024:

Actually also this TODO, I think these basically mean the same thing: - TODO: Not an ancestor or a descendant of the snapshot block and has more work

fjahr commented at 5:22 pm on July 8, 2024:

Could also mention #28648 as related in description because of this.

mzumsande commented at 5:23 pm on July 8, 2024:

Removed both TODOs.

mzumsande force-pushed on Jul 8, 2024

mzumsande commented at 5:25 pm on July 8, 2024: contributor

29ab88d to ab478c5: Addressed feedback by @fjahr (thanks!) and rebased.

DrahtBot removed the label Needs rebase on Jul 8, 2024

in src/validation.cpp:5685 in ab478c5fa1 outdated

5680@@ -5681,6 +5681,10 @@ util::Result<void> ChainstateManager::ActivateSnapshot(
5681             return util::Error{strprintf(_("The base block header (%s) is part of an invalid chain."), base_blockhash.ToString())};
5682         }
5683 
5684+        if (!m_best_header || m_best_header->GetAncestor(base_blockheight) != snapshot_start_block) {
5685+            return util::Error{_("A forked headers-chain with more work than the chain with the base block header exists. Please proceed to sync without AssumeUtxo.")};

fjahr commented at 8:09 pm on July 8, 2024:

nit: seems the “snapshot” added before is back out, still not a blocker for me though

mzumsande commented at 8:31 pm on July 8, 2024:

ah, sorry, am on a different computer now and forgot to update the branch… Fixed now.

fjahr commented at 8:13 pm on July 8, 2024: contributor

re-ACK ab478c5fa16427b496e8a93e4780770d4c270214

Confirmed only changes since last review were the rebase and addressing comments (per git range-diff master 29ab88dfe1c43af3620480c99cba844aa414023c ab478c5fa16427b496e8a93e4780770d4c270214).

mzumsande force-pushed on Jul 8, 2024

fjahr commented at 8:36 pm on July 8, 2024: contributor

re-ACK b715a13d11c6ca7e928b2dee66edcc8c6c30a8d4

Just addressed my latest re-nit.

alfonsoromanz approved

alfonsoromanz commented at 9:11 am on July 9, 2024: contributor

tACK b715a13d11c6ca7e928b2dee66edcc8c6c30a8d4

The test passes successfully. I also verified that the test fails with no exception raised if the patch in src/validation.cpp is not applied, i.e

File “/Users/alfonso/dev/bitcoin/test/functional/feature_assumeutxo.py”, line 238, in test_snapshot_not_on_most_work_chain assert_raises_rpc_error(-32603, msg, node1.loadtxoutset, dump_output_path) File “/Users/alfonso/dev/bitcoin/test/functional/test_framework/util.py”, line 148, in assert_raises_rpc_error assert try_rpc(code, message, fun, *args, **kwds), “No exception raised” AssertionError: No exception raised

DrahtBot added the label Needs rebase on Jul 10, 2024

validation: Don't load a snapshot if it's not in the best header chain.

If the snapshot is not an ancestor of the most-work header (m_best_header),
syncing from that alternative chain should be prioritised.
Therefore don't accept loading a snapshot in this situation.

If that other chain turns out to be invalid, m_best_header
would be reset and loading the snapshot should be possible again.

Because of the work required to generate a conflicting headers chain,
this should only be possible under extreme circumstances, such as major forks.

55b6d7be68

mzumsande force-pushed on Jul 11, 2024

mzumsande commented at 5:14 pm on July 11, 2024: contributor

b715a13 to 55b6d7b: rebased

DrahtBot removed the label Needs rebase on Jul 11, 2024

fjahr commented at 11:27 pm on July 11, 2024: contributor

re-ACK 55b6d7be68a6f6c3882588ffd5b9349d885ed953

DrahtBot requested review from alfonsoromanz on Jul 11, 2024

alfonsoromanz approved

alfonsoromanz commented at 8:40 am on July 12, 2024: contributor

Re ACK 55b6d7be68a6f6c3882588ffd5b9349d885ed953

achow101 commented at 9:21 pm on July 18, 2024: member

ACK 55b6d7be68a6f6c3882588ffd5b9349d885ed953

achow101 merged this on Jul 18, 2024

achow101 closed this on Jul 18, 2024

mzumsande deleted the branch on Jul 18, 2024

Sjors commented at 4:48 pm on July 23, 2024: member

Post merge concept ACK

assumeutxo: Don’t load a snapshot if it’s not in the best header chain #30320

Code Coverage

Reviews

Conflicts