test: Assumeutxo: snapshots with less work should not be loaded

hernanmarino commented at 8:01 pm on February 13, 2024: contributor

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 (review))

DrahtBot commented at 8:01 pm on February 13, 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	maflcko, kevkevinpal, alfonsoromanz, achow101
Stale ACK	BrandonOdiwuor, rkrux

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:

#29996 (test: Assumeutxo: import snapshot in a node with a divergent chain by alfonsoromanz)
#29553 (assumeutxo: Add dumptxoutset height param, remove shell scripts 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 Tests on Feb 13, 2024

maflcko commented at 10:34 am on February 14, 2024: member

@aureleoules Looks like https://corecheck.dev/bitcoin/bitcoin/pulls/29428 is dead?

aureleoules commented at 1:07 am on February 17, 2024: member

Looks like 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.

maflcko commented at 1:42 pm on February 19, 2024: member

You’ll have to remove the TODO on the top of the file as well, when the test was added?

hernanmarino commented at 7:57 pm on February 26, 2024: contributor

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

BrandonOdiwuor commented at 6:03 am on February 27, 2024: contributor

Code Review ACK 86037415dd83da34d829f2f6bfb8dcbe08968180

Should remove the TODO fixed by this commit

maflcko commented at 12:53 pm on April 23, 2024: member

Are you still working on this?

hernanmarino marked this as ready for review on Apr 23, 2024

hernanmarino commented at 5:25 pm on April 23, 2024: contributor

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.

maflcko commented at 5:33 pm on April 23, 2024: member

I was asking, because you said you’d update the branch in February: #29428 (comment)

hernanmarino renamed this:
~~test: add missing tests for Assumeutxo~~
test: Assumeutxo: snapshots with less work should not be loaded
on Apr 23, 2024

hernanmarino force-pushed on Apr 23, 2024

hernanmarino commented at 7:44 pm on April 23, 2024: contributor

Updated to remove a (no lonnger valid) TODO comment as requested in #29428 (comment). Also updated the PR title and description.

maflcko commented at 7:01 am on April 24, 2024: member

Please delete the hidden comment in the pull description, because this will end up in the merge commit.

hernanmarino commented at 1:20 pm on April 24, 2024: contributor

Please delete the hidden comment in the pull description, because this will end up in the merge commit.

Oh, sorry for that. Deleted.

alfonsoromanz approved

alfonsoromanz commented at 2:59 pm on April 24, 2024: contributor

Code review and tested ACK 4922f56660ae7834598c6dc4904a6711c23edfa3

DrahtBot requested review from BrandonOdiwuor on Apr 24, 2024

maflcko commented at 11:10 am on April 29, 2024: member

For detailed information about the code coverage, see the test coverage report. @aureleoules @adamjonas Looks like this is down again. LambdaTimeout - 2024-04-29T11:09:51.844Z 3957d9f6-7e67-4e79-bf45-9569b4491cd0 Task timed out after 10.01 seconds

in test/functional/feature_assumeutxo.py:19 in 4922f56660 outdated

19@@ -20,8 +20,6 @@
20       bad other serialization)
21 - TODO: Valid snapshot file, but referencing a snapshot block that turns out to be
22       invalid, or has an invalid parent
23-- TODO: Valid snapshot file and snapshot block, but the block is not on the
24-      most-work chain

maflcko commented at 11:17 am on April 29, 2024:

Why is this removed? The block is in the most-work chain. I think this comment meant a block that is not in the most-work chain, no?

alfonsoromanz commented at 3:21 pm on April 29, 2024:

Maybe the line that should be removed here is this one? - TODO: A descendant of the snapshot block. I believe the scenario where the snapshot block is not on the most-work chain is addressed here: https://github.com/bitcoin/bitcoin/pull/29996

hernanmarino commented at 6:05 pm on April 30, 2024:

Why is this removed? The block is in the most-work chain. I think this comment meant a block that is not in the most-work chain, no?

Maybe i misinterpreted the TODO comment meaning . To which TODO were you referring here
#29428 (comment) ?

maflcko commented at 6:07 pm on April 30, 2024:

Sorry, I think this was my bad. It looks like there is no TODO for the test you are adding here.

hernanmarino commented at 0:35 am on May 1, 2024:

Restored that TODO in my last push. Thanks

hernanmarino force-pushed on May 1, 2024

alfonsoromanz approved

alfonsoromanz commented at 0:44 am on May 1, 2024: contributor

Re ACK 2f1b1eee8b8a8f546fb5975ac529c4032e46f069

hernanmarino commented at 0:50 am on May 1, 2024: contributor

Updated to re add an incorrectly removed TODO comment.

maflcko commented at 5:19 am on May 1, 2024: member

lgtm ACK 2f1b1eee8b8a8f546fb5975ac529c4032e46f069

rkrux approved

rkrux commented at 1:40 pm on May 7, 2024: none

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.

0util/time.cpp:54:9: error: use of undeclared identifier 'gmtime_s'; did you mean 'gmtime_r'?
1    if (gmtime_s(&epoch, &time_val) != 0) {
2        ^~~~~~~~
3        gmtime_r

achow101 commented at 3:58 pm on May 9, 2024: member

Needs rebase (for some reason Drahtbot isn’t detecting this)

achow101 added the label Needs rebase on May 9, 2024

maflcko removed the label Needs rebase on May 13, 2024

DrahtBot added the label Needs rebase on May 13, 2024

hernanmarino force-pushed on May 21, 2024

DrahtBot removed the label Needs rebase on May 21, 2024

in test/functional/feature_assumeutxo.py:171 in 3e1174687f outdated

166+
167+        node = self.nodes[0]
168+        # At this point, node 0 should have more accumulated work than the snapshot, assert that
169+        assert_equal(node.getblockcount(), FINAL_HEIGHT)
170+
171+        # Attempt to load the snapshot onto same node and expect it to fail

maflcko commented at 8:55 am on May 22, 2024:

style nit: The code should be self-explanatory, so could remove this comment? (This should also re-trigger the false positive failed CI)

test: Assumeutxo: snapshots with less work should not be loaded df6dc2aaae

hernanmarino force-pushed on May 23, 2024

hernanmarino commented at 0:11 am on May 23, 2024: contributor

Needs rebase (for some reason Drahtbot isn’t detecting this)

Rebased. Also deleted a couple of redundant comment as suggested by @maflcko to retrigger the false negative CI

in test/functional/feature_assumeutxo.py:170 in df6dc2aaae

165+        self.log.info("Test bitcoind should fail when snapshot has less accumulated work than this node.")
166+        node = self.nodes[0]
167+        assert_equal(node.getblockcount(), FINAL_HEIGHT)
168+        with node.assert_debug_log(expected_msgs=["[snapshot] activation failed - work does not exceed active chainstate"]):
169+            assert_raises_rpc_error(-32603, "Unable to load UTXO snapshot", node.loadtxoutset, dump_output_path)
170+        self.restart_node(0, extra_args=self.extra_args[0])

furszy commented at 0:22 am on May 23, 2024:

why restart if you aren’t doing anything with the node?

maflcko commented at 8:20 am on May 23, 2024: member

utACK df6dc2aaaeffc664006b86ee8c8797dc484ec40e

DrahtBot requested review from rkrux on May 23, 2024

DrahtBot requested review from alfonsoromanz on May 23, 2024

kevkevinpal commented at 1:47 pm on May 25, 2024: contributor

utACK df6dc2a

alfonsoromanz approved

alfonsoromanz commented at 8:00 am on May 28, 2024: contributor

Re ACK df6dc2aaaeffc664006b86ee8c8797dc484ec40e. Make is successful and the test passes.

achow101 commented at 11:05 pm on June 4, 2024: member

ACK df6dc2aaaeffc664006b86ee8c8797dc484ec40e

achow101 merged this on Jun 4, 2024

achow101 closed this on Jun 4, 2024

test: Assumeutxo: snapshots with less work should not be loaded #29428

Code Coverage

Reviews

Conflicts