feat: discard non-finalizable blocks #7746
Conversation
| fn check_if_finalizable(&self, header: &BlockHeader) -> Result<(), Error> { | ||
| let mut header = header.clone(); | ||
| let final_head = self.final_head()?; | ||
| for _ in 0..NUM_PARENTS_TO_CHECK_FINALITY { |
There was a problem hiding this comment.
I'm not sure about the need for this constant. This actually assumes that in the chain, we would never have a fork with 20 blocks. While this rarely happens, adding this assumption doesn't help much with the implementation in this function, so I don't think it should be added. the check that compares the block height with the final head height should be enough.
There was a problem hiding this comment.
This actually assumes that in the chain, we would never have a fork with 20 blocks.
I don't see where we need this assumption. If there is a fork with >20 blocks, it means that we just can't apply our optimization and discard it here. Having constant 20 helps us to discard more useless blocks (see below).
the check that compares the block height with the final head height should be enough.
I think the current check is helpful for scenarios as follows:
/-> 2 -> 3 -> 5 -> 6 -> 7
1
(final)
\-> 4 -> 8
- processing blocks (1) - (6) don't change final head, so no blocks can be discarded
- block (7) moves final head to (5)
- now block (8) can be discarded during preprocessing, but to know it, we need to look at its parent (4).
There was a problem hiding this comment.
I see. I still don't understand why we need to only check the last 20 blocks. What do you think about just changing the for loop to loop?
There was a problem hiding this comment.
I'm concerned about very long forks for which it can take a while to iterate over parents. Maybe there is some reason not to worry about it, but I don't see it.
There was a problem hiding this comment.
If I put the check to preprocess_block, then these tests are failing:
tests::challenges::challenges_new_head_prev
tests::simple_chain::blocks_at_height
They create some tree of blocks on chain, and the tests fail because some of blocks are being discarded.
Moved it to Client::start_process_block, because it looks like optimization on Client side, and I wouldn't make Chain tests assuming this optimization as well. Even after this, two other tests had to be changed.
| } | ||
|
|
||
| /// Verifies that the block at height are updated correctly when blocks from different forks are | ||
| /// processed, especially when certain heights are skipped | ||
| /// processed, especially when certain heights are skipped. |
There was a problem hiding this comment.
Could you add a comment on what the chain looks like in this test?
Because flat storage head is correlated with final head, we won't be able to process blocks on top of blocks which are behind final head. Processing such blocks doesn't make sense anyway, because they will never be finalized. We can check it during header validation and avoid attempts to apply chunks. More concretely - we check parents of the new block, and if we fall behind final head at some point, we declare that block can't be finalized and return an error. To avoid long delays, we limit number of checked parents to 20. ## Testing * Existing tests. * Updating `simple_chain` tests to reflect new behaviour * `process_blocks::test_discard_non_finalizable_block` - integration test for checking chain behaviour
Because flat storage head is correlated with final head, we won't be able to process blocks on top of blocks which are behind final head. Processing such blocks doesn't make sense anyway, because they will never be finalized. We can check it during header validation and avoid attempts to apply chunks.
More concretely - we check parents of the new block, and if we fall behind final head at some point, we declare that block can't be finalized and return an error. To avoid long delays, we limit number of checked parents to 20.
Testing
simple_chaintests to reflect new behaviourprocess_blocks::test_discard_non_finalizable_block- integration test for checking chain behaviour