Fall back in proposal checking #457
Conversation
* FM-356: Rename TopDownConfig to TopDownSettings * FM-356: Snapshot settings * FM-356: Disable snapshots for now
* IPC-304: upgrade contract * upgrade contract dependencies
* FM-363: Break up smoke-test Makefile.toml into reusable parts * FM-363: Reuse the infra scripts for testing * FM-363: Move cometbft related command to cometbft.toml
|
Can you perhaps rebase your feature branch so that this PR doesn't contain snapshot related changes? |
|
I strongly disagree with this PR, as I have said the other day. We should not do IO during voting. |
| match retry!( | ||
| self.config.exponential_back_off, | ||
| self.config.exponential_retry_limit, | ||
| self.parent_client.get_block_hash(proposal.height).await | ||
| ) { |
There was a problem hiding this comment.
Doing exponential backoff during voting would be detrimental to liveness.
| self.config.exponential_retry_limit, | ||
| self.parent_client.get_block_hash(proposal.height).await | ||
| ) { | ||
| Ok(r) => Ok(r.block_hash == *proposal.block_hash), |
There was a problem hiding this comment.
I don't see how this checks whether that block is final or not.
There was a problem hiding this comment.
There is this check:
if let Some(valid_height) = self.inner.check_height(proposal)? {
if !valid_height {
return Ok(Some(false));
}I can't remember when this returns None or Some, neither true or false. Something like, "if the tip of the chain on the parent is less than 20 away from the proposal it's not final", right? And None would be if the cache is empty.
So, if our cache is empty, this fallback would accept any block hash, even if it's the tip of the chain itself, as long as we see the same thing. No finality check.
If the cache is not empty, then it will reject the height as not-final, and we don't go to the fallback.
Can check_height return Some(true) and check_hash return None in a way that it still makes sense to go to the fallback? When does this happen?
There was a problem hiding this comment.
Does it return None when the proposed height is beyond what's in our cache, because we are catching up?
For example say the parent chain is 1000 blocks long, the last finalized block is 500, and Alice has fetched the allowed +100 blocks after that, so she knows what's on the parent chain up to 600. Then comes Bob proposing that block 650 is final. Does Alice's check_height return None because it's beyond 600, and then this fallback becomes an automatic acceptance because if she looks at the parent it matches at 650, and this way Bob has circumvented the 100 limit, as well as the finality check?
Add fallback in proposal checking, i.e. if the proposal is not found in cache, do io.