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) |
hernanmarino
changed the title
hernanmarino
force-pushed
the
assumeutxo-test-several
branch
from
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. |
hernanmarino
force-pushed
the
assumeutxo-test-several
branch
from
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) |
hernanmarino
force-pushed
the
assumeutxo-test-several
branch
from
2f1b1ee
to
3e11746
Compare
hernanmarino
force-pushed
the
assumeutxo-test-several
branch
from
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 |
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))