That's the first time I see a ++ used in this way. While this works, it's probably best done as two statements.

If rit was incremented, then aren't you removing the wrong item here?

I'm not sure what's going on in this loop, but it looks liek to me there is an off by one somewhere in there.

First, this unit tests is very hard to follow, because it is not a unit test really, it is a ton of different unit tests merged in one.

But as far as I can tell, it only check proof with two different numbers of inputs, namely 1 and AVALANCHE_MAX_PROOF_STAKES. This make it so that proper stake count tracking is actually not tested (there could be an off by one in the loop as I suspect that the test would likely still pass).

You need to refactor this in such a way that it is obvious what is covered by the test - and, even more importantly what isn't. There are at least 2 or 3 tests waiting to be extracted from this.

That looks more reasonable than the previous iteration.

Even if this is stupid, it is expected that the pool will still work properly. So I really don't see why this needs to crash the node.

Why do you need to insert the proof in this way? proofs.insert(proof) or something doesn't work? Also, if you are taking ownership of the proof, just take it by value.

You are baking the assumption that all proof are of the same stake. You generally don't want to bake too many assumptions in the code.

This is not testing the boundaries condition of the removal. Start with what the test should tests instead of what it does (hint: proofpool_add_variable_size is about what it does). Once you ask yourself what needs to be tested, then the test and test case become obvious. Start with the problem.

Are you sure this is noexcept? The C++ runtime will hard crash the program here if an exception is thrown. Unless this is something that multi_index guarantees and something that you actually need, then I suggest leaving it off. It seems to me to be unlikely to matter considering you are already looping in there.

Does it really make sense to copy the proof here? I don't think it does.

Why is proof id passed by const ref in the previous function and by value in that one? One of the two must be wrong.

static ?

It doesn't look like this is used anywhere.

Just like for remove_proofs, this is unable to check the overflow behavior properly.

So this seems to be checking the eviction behavior, except it doesn't really check boundary condition - or at least, it doesn't ensure that the boundary conditions are tested.

The size seems to be the only thing that you use in the queue, so why keep a copy of the proof around?

Because al the proof are the same size, you do not check that the size if kept track of accurately. The pool could always remove 42, or remove the size of another proof, that you wouldn't know. This test looks like it is testing something, but it really isn't.

This is an excellent test, but what is it testing? It is testing what happens when you add a proof larger than the pool. This is what the test name should reflect.

Are the counter maintained properly through this process?

Maybe this is checking boundaries conditions, maybe not. There is no way to really know, except by checking the values provided and what happens with them, but even if this ends up testing everything, it'll do so up to the point someone change something and then it doesn't. this is extremely fragile.

You must test boundary conditions.

If you are going to make this constant size, just use an array.

This is 2 lines, you don't really make anything more readable by factoring this out, especially since parameters are not taken in explicitly, so you got all kind of action at a distance magic going on. Exactly what you do not want for a test.

What does this achieve?

Don't keep accounting. Just tell the goddamn test what is the value you expect.

This is what a good test looks like.