consensus-shipyard/mir

Checkpoint typo did not trigger FAILed tests, why?

Closed this issue · 3 comments

@matejpavlovic and I have recently found this bug in which we missed the ! before the reflect.DeepEqual. While this is an easy-to-solve typo (coming in a PR soon), it is not expected that all tests PASSed given this error when verifying the checkpoint.

We need to know why is that. We came up with the possibility that this is because the introduction of the async multisig collector means that in the first epoch all the transactions in all tests are delivered, so no checkpoint actually need to be verified in the tests. It could be this or something else, but we need to figure it out and resolve it so that the tests are comprehensive again.

Typo resolved in this PR, but issue remains that we need to understand and fix at least testing.

In addition to figuring out why the tests passed despite the error, we should write unit tests for the checkpoint, which should have discovered that bug in the first place.

Good news. It is what I suspected - the async multisig collector. The system is just too fast to notice the problem with the checkpoint, because everything gets delivered before the checkpoint. When I set the limit of sub-certificates in the multisig collector back to one, the test fails as it should.