Set pending block only after making sure it is valid. #2925
Conversation
| if manager | ||
| .requested_proposed | ||
| .as_ref() | ||
| .map(|proposal| &proposal.content) | ||
| != Some(&proposal.content) |
There was a problem hiding this comment.
I fail to see how this addition is related to the main objective of the PR - can you explain?
There was a problem hiding this comment.
We now already call handle_block_proposal earlier, before we set the pending block to that proposal. It would be wasteful to call it here again. So we check if the proposal the local node has already seen is the one we are about to send to the validators; if it is, there is no reason to call handle_block_proposal again.
There was a problem hiding this comment.
Ok, but we extracted the requested_propsed earlier already - in line 1950 - can we use that?
There was a problem hiding this comment.
We now already call handle_block_proposal earlier, before we set the pending block to that proposal.
Earlier than what? I can't find that it changed from what it used to be.
There was a problem hiding this comment.
I don't see how we could merge the two places where we look into requested_proposed; the logic is very different, and in the second case, the condition is also true in the None case.
Earlier than what?
I mean we call it already before we set the pending block, in execute_block.
There was a problem hiding this comment.
I mean we call it already before we set the pending block, in execute_block.
Now I see it - in execute_block we call
set_pending_blockwhich calls-
handle_block_proposaland then
-
self.state_mut().set_pending_block(block).
- After that we call
process_pending_block_without_preparewhich in turn calls -
- this
propose_block.
- this
So we don't need to re-handle.
It's all so clear now 🙃
linera-core/src/client/mod.rs
Outdated
| @@ -2108,12 +2123,20 @@ where | |||
| #[instrument(level = "trace", skip(incoming_bundles, operations))] | |||
| async fn set_pending_block( | |||
There was a problem hiding this comment.
Maybe we can rename this method to new_pending_block? Or maybe you have even better idea.
The reason I'm suggesting is:
- It's not setting per-se, it's creating and then setting. But it can still fail so it may not even set anything.
set_pending_blockcalls anotherset_pending_blockmethod and I don't really like having many methods with exact same name
| .local_node | ||
| .handle_block_proposal(*proposal.clone()) | ||
| .await?; | ||
| if manager |
There was a problem hiding this comment.
In line 1970 we extract manager.requested_locked but we did that in line 1938 as well. Are these duplications necessary?
There was a problem hiding this comment.
Probably not, and also some of the checks we do (already before this PR) are redundant; but I was considering to do those cleanups in a separate PR. I'll look into it, and if the change isn't huge I can do them right away.
|
Thanks! |
| self.stage_block_execution_and_discard_failing_messages(block) | ||
| .await? | ||
| .0 | ||
| self.stage_block_execution(block).await?.0 |
There was a problem hiding this comment.
Another thing I'm failing to understand: if we always stage the block execution before adding the block as pending, then why do we ever need to stage it here again? Is this where that ExecutedBlock cache that we've talked about will come in?
There was a problem hiding this comment.
Yes, exactly: It's only because we kind of need the executed block, and didn't cache that earlier.
Motivation
#2904 introduced block proposal size limits that are only enforced when the proposal was handled in the worker, not when staging block execution. This could result in the pending block getting set to an invalid proposal.
Proposal
Handle the proposal in the local node before setting the pending block.
Test Plan
The explicit
clear_pending_blockcall was removed from the test.Release Plan
Links