test: Assumeutxo: snapshots with less work should not be loaded #29428
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. |
|
@aureleoules Looks like https://corecheck.dev/bitcoin/bitcoin/pulls/29428 is dead? |
@maflcko thanks for the ping. The code I added to filter out flaky coverage was too slow and isn't very reliable so I removed it. Unfortunately, the UI will now display flaky coverage. |
|
You'll have to remove the TODO on the top of the file as well, when the test was added? |
Done. It'll be updated with my next push |
There was a problem hiding this comment.
Code Review ACK 8603741
Should remove the TODO fixed by this commit
|
Are you still working on this? |
I haven´t been able to think of / work on other tests to add, but the current commit test is ready for review, so i am taking the PR out of Draft status. |
|
I was asking, because you said you'd update the branch in February: #29428 (comment) |
8603741 to
4922f56
Compare
|
Updated to remove a (no lonnger valid) TODO comment as requested in #29428 (comment). Also updated the PR title and description. |
|
Please delete the hidden comment in the pull description, because this will end up in the merge commit. |
Oh, sorry for that. Deleted. |
@aureleoules @adamjonas Looks like this is down again. |
4922f56 to
2f1b1ee
Compare
|
Updated to re add an incorrectly removed TODO comment. |
|
lgtm ACK 2f1b1ee |
There was a problem hiding this comment.
crACK, utACK 2f1b1eee
Thanks for adding the test, it's short and to the point. However, I've not been able to build the branch because it fails on my system (MAC i7) with the following error. I understand it is because this branch doesn't contain the latest changes of the master branch, notably this one: #29081.
util/time.cpp:54:9: error: use of undeclared identifier 'gmtime_s'; did you mean 'gmtime_r'?
if (gmtime_s(&epoch, &time_val) != 0) {
^~~~~~~~
gmtime_r
|
Needs rebase (for some reason Drahtbot isn't detecting this) |
2f1b1ee to
3e11746
Compare
3e11746 to
df6dc2a
Compare
Rebased. Also deleted a couple of redundant comment as suggested by @maflcko to retrigger the false negative CI |
| assert_equal(node.getblockcount(), FINAL_HEIGHT) | ||
| with node.assert_debug_log(expected_msgs=["[snapshot] activation failed - work does not exceed active chainstate"]): | ||
| assert_raises_rpc_error(-32603, "Unable to load UTXO snapshot", node.loadtxoutset, dump_output_path) | ||
| self.restart_node(0, extra_args=self.extra_args[0]) |
There was a problem hiding this comment.
why restart if you aren't doing anything with the node?
|
utACK df6dc2a |
|
utACK df6dc2a |
|
ACK df6dc2a |
… chain 2f9bde6 test: Remove unnecessary restart in assumeutxo test (Fabian Jahr) 19ce3d4 assumeutxo: Check snapshot base block is not marked invalid (Fabian Jahr) 80315c0 refactor: Move early loadtxoutset checks into ActiveSnapshot (Fabian Jahr) Pull request description: This was discovered in a discussion in #29996 If the base block of the snapshot is marked invalid or part of an invalid chain, we currently still load the snapshot and get stuck in a weird state where we have the snapshot chainstate but it will never connect to our valid chain. While this scenario is highly unlikely to occur on mainnet, it still seems good to prevent this inconsistent state. The behavior change described above is in the second commit. The first commit refactors the early checks in the `loadtxoutset` RPC by moving them into `ActivateSnapshot()` in order to have the chance to cover them by unit tests in the future and have a more consistent interface. Previously checks were spread out between `rpc/blockchain.cpp` and `validation.cpp`. In order to be able to return the error message to users of the RPC, the return type of `ActivateSnapshot()` is changed from `bool` to `util::Result`. The third commit removes an unnecessary restart introduced in #29428. ACKs for top commit: mzumsande: re-ACK 2f9bde6 alfonsoromanz: Re-ACK 2f9bde6. The RPC code looks much cleaner after the refactor. Also, it seems very useful to get the error message in the RPC response rather than having to rely on the logs in some scenarios if you are an RPC user. achow101: ACK 2f9bde6 Tree-SHA512: 5328dd88c3c7be3f1be97c9eef52ac3666c27188c30a798b3e949f3ffcb83be075127c107e4046f7f39f961a79911ea3d61b61f3c11e451b3e4c541c264eeed4
This PR adds a test which checks that snapshots with less accumulated work than the node's active chain, should not be loaded and return with an error. Although in a different context of discussion the missing test was detect in a thread in #29394 (see #29394 (comment))