re: #30214 (review)
I find the entire chainstatemanager_snapshot_init
subtest very confusing
Yes. And thanks for digging into the test. I added comments to try to make it less confusing. For reference the test was added in e4d7995 and seems basically unchanged since when it was written.
First, there is a comment saying “Test that simulating a shutdown (…) we end up with a single chainstate that is at tip”, followed by a check that there are still 2 chainstates - so the test contradicts itself.
Good catch, the comment is wrong. If you search for “single chainstate” you can see the same comment is copied from 2 other tests in this file and is ok in those places. Fixed in latest push.
Then the test calls the test-only function LoadVerifyActivateChainstate
which only calls ABC for the active chainstate, instead of doing it for all chainstates like its counterpart in ImportBlocks()
during actual init does - why is this using an artificial init process that differs from the way it’s done in production?
I don’t know the answer to this. The unit tests do a lot of artificial things and I’d agree it is usually better when we can remove artificial code so tests are more realistic and there is less code to maintain.
Because of that, only during mineBlocks()
(as a result of ProcessNewBlock()
calling ABC for both chainstates) the block that was previously disconnected in a hacky way (by simply calling DisconnectBlock()
instead of e.g. invalidating it) is being connected again, and the background snapshot is verified, which is a surprising side effects of just trying to connect some blocks on the tip - and the actual reason why the height is suddenly 110.
Added a comment about that instead of changing it so this is still a focused bugfix. Agree if test were more realistic it might be clearer.
I think that this deserves an explanation, but more importantly, I don’t see the point / strategy of the subtest in general - what situation is it actually trying to test?
I think the main thing it is trying to test is BOOST_CHECK_EQUAL(chainstates.size(), 2);
i.e. “that snapshot chainstates initialize properly when found on disk” and that the chainstates function as expected when more blocks are mined. Expanded the comment at the top of the test, too.
I realize that this isn’t really what this PR is about, so maybe there could be a separate PR to clean this test up properly, especially if the other subtests (which I didn’t check) would have similar problems?
I think it would be nice if tests were more realistic, and the main thing that annoys me about these tests that is instead of using literal numbers and expressions, the tests like to declare lots of helper variables, so there are many similarly named variables it it is difficult to keep track of what they all mean and how they are used. It probably would be nice to clean these tests up and make them simpler, though not a priority.