Isn't the bad coin height test also already covered with the test where you made the fix?

Yeah, I wonder how one could test a "valid" hash but invalid snapshot file. I guess one would have to add a hash of the invalid file to the source code.

Though, this comment presumably refers to https://maflcko.github.io/b-c-cov/total.coverage/src/validation.cpp.gcov.html#5398 which is still uncovered.

I wonder how one could test a "valid" hash but invalid snapshot file

I would just generally understand this as taking a valid snapshot file and altering the content so that it still serializes correctly but results in a different hash. The way I understand it "valid hash" here means that the hash in the metadata of the file matches the hash in the chainparams.

I guess one would have to add a hash of the invalid file to the source code.

That implies that the attacker is able to give the user a build that is not a release, no? (or we have made a huge mistake when adding the hash) If the attackers are able to do that they could do anything so it doesn't seem feasible to cover such a scenario in our tests.

Right. Though, in that case, it could make sense to transform the unreachable/untestable code, if there is any, into an assert fail, or use the Assume fail.

Yeah, I wonder how one could test a "valid" hash but invalid snapshot file. I guess one would have to add a hash of the invalid file to the source code.

Yes, my suggestion was to add an invalid regtest hash to the source code so we can verify that assumeutxo code does what it is supposed to do when an invalid hash is added.

And I don't like the idea of making the executable abort when an invalid hash is detected, instead of returning a normal error message, because that would make it more difficult than it needs to be to write a test covering all the error cases, and also make error handling internally more fragile and less straightforward.

This is just to explain my earlier suggestion though. I see these hashes as external inputs that should be validated like other inputs, even they happen to be hard coded to prevent usage errors. But if you want to treat these hashes as normal source code that should be able to trigger crashes in other parts of the source code, that is also reasonable.