Test if checkpoint verifier and state service are correctly reset on block commit errors

[conradoplg]

Motivation

We want to test that errors in the state are handled correctly.

Design

We want to do these tests here:

• when committing checkpointed blocks, fail a finalized state block commit
  • commit_finalized
• when committing fully verified blocks, fail a non-finalized state block commit
  • commit_block
  • commit_new_chain
• drop finalized blocks in the queue
• drop non-finalized blocks in the queue

If these tests take longer than a day to write, we're going to close this ticket as "too hard for now".

Old Description

#2633 adds ZIP-221/244 block commitment validation in the checkpoint verifier. The check is actually done in the finalized state, and when that happens, the checkpoint verifier is reset to the tip of the finalized state in order to keep running correctly.

However, this hasn't been tested, since it is tricky to test.

Implement a test to verify that it's working correctly.

(Note that is only required if/when we have a checkpoint post-Nu5. Before that all block contents are committed to by the block hash, so they must be correct. Nu5-onward that no longer applies due to ZIP-244.)

Previous attempt

I attempted to write this test in #2633 but gave up because it was too difficult and we had to keep working on other stuff.
See discussion. However, most of the problems were caused by trying to test it along with the "fake activation heights" feature in that PR, which would require generating fake blocks with correct difficulties, etc. in order to pass the checkpoint verifier. Another alternative would be to use real blocks similar to the continuous_blockchain - but the problem would then be how to ensure that one block fails to be comitted in order to test if that's handled correctly (the real blocks have small heights, and thus they don't use the block commitment in the header, so there is nothing to corrupt)

[teor2345]

(the real blocks have small heights, and thus they don't use the block commitment in the header, so there is nothing to corrupt)

I think we can test using the fake network upgrade heights, and the fake transaction version conversion methods.

[mpguerra]

Hey team! Please add your planning poker estimate with ZenHub @conradoplg @dconnolly @jvff @oxarbitrage @teor2345 @upbqdn

[teor2345]

We don't have any post-NU5 checkpoints yet. This ticket is optional until we do.

[ftm1000]

redistributing issues that can be separately worked from Epic #2311

[teor2345]

This is an obscure case, we can test it if it causes problems later.

[teor2345]

We might need to do this for #4937

[teor2345]

Just to expand on my last comment a bit more: #4937 changes how error handling works in the state, and makes it more complicated, so we probably need to test that it actually works.

[mpguerra]

let's timebox this to one day's work and move on if it's too hard...

[teor2345]

We think we want to do these tests here:

• when committing checkpointed blocks, fail a finalized state block commit
• when committing fully verified blocks, fail a non-finalized state block commit
• when committing fully verified blocks, fail a finalized state block commit edit: this can't happen, it's currently a panic

[teor2345]

I can do this, but I might want to work on it with either @arya2 or @upbqdn (whoever is available).

[upbqdn]

I can help.

[teor2345]

Just a reminder that this task has a time limit of 1 day, so if it's not done by the end of the sprint, we should just close it.

We're looking for a really quick test here, we just need to send one error, and check that the state doesn't crash or hang.

[teor2345]

@mpguerra we decided not to do this in this sprint, because it's not a release blocker, and we've done enough manual testing.

[mpguerra]

re-opening for #5377

[mpguerra]

Closing for now. See test-block-commit-service for draft solution in case we want to pick this up again in future.

[upbqdn]

Closing for now. See test-block-commit-service for draft solution in case we want to pick this up again in future.

The corresponding PR is #5377.