assumeutxo: Don't load a snapshot if it's not in the best header chain #30320
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
055af4d to
29ab88d
Compare
|
After merge of #30267: Rebased, addressed nit by fjahr, and marked as ready for review. |
| self.log.info("Test bitcoind should fail when the node knows the headers of another chain with more work.") | ||
| node0 = self.nodes[0] | ||
| node1 = self.nodes[1] | ||
| # create two blocks that form a longer chain |
There was a problem hiding this comment.
nit: Comment could be a little more detailed
| # create two blocks that form a longer chain | |
| # 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. |
There was a problem hiding this comment.
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.
29ab88d to
ab478c5
Compare
ab478c5 to
b715a13
Compare
|
re-ACK b715a13 Just addressed my latest re-nit. |
There was a problem hiding this comment.
tACK b715a13
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
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.
b715a13 to
55b6d7b
Compare
|
re-ACK 55b6d7b |
|
ACK 55b6d7b |
|
Post merge concept ACK |
Summary: 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. This is a backport of [[bitcoin/bitcoin#30320 | core#30320]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D18059
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 tom_best_headershould 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_headerwill 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.