ProcessProposal: avoid calls when we know one correct process has already accepted it

[sergio-mena]

In the current implementation of ProcessProposal (and spec?), it is called just after doing Comet-level validations on the received proposal block.

However, there are some cases where the local node can conclude that the received proposal has already been processed and accepted by another correct node's ProcessProposal.

Since ProcessProposal's main goal is to detect and drop bad proposals from byzantine validators, calling ProcessProposal on a proposal that we know has passed ProcessProposal elsewhere is, at minimum, suboptimal.

These are the cases where we know ProcessProposal has already accepted the proposal at some correct node:

• If the proposal's POLRound is -1 and the proposed block matches the one locally locked (part of line 23 of the arXiv algorithm)
• If the proposal's POLRound is not -1 and less than the current round, and
  • we have received 2f+1 (worth of voting power) valid prevotes for the proposed block in POLRound, and
    • either POLRound is greater than or `LockedRound?
    • or the proposed block matches the one locally locked
    • (line 29 of the arXiv algorithm)
• If the proposed block matches our ValidBlock

In the arXiv algorithm, this is equivalent to removing $valid(v)$ from line 29, and changing line 23 as follows:
if $(lockedRound_p = −1 ∧ valid(v)) ∨ lockedValue_p = v$ then.
Also, we assume that $valid(v)$ does not include ProcessProposal in lines 36 and 50 (do we really need to call $valid(v)$ at all there?).

This is a similar reasoning to the one followed to minimize calls to timely in the Proposer-Based Timestamp (PBTS) specification.

@hvanz @lasarojc @BrendanChou

[nenadmilosevic95]

We can go even further than proposed. The minimal condition would be receiving $f+1$ (worth of voting power) valid prevotes messages from different validators for the proposed block. They can even be from different rounds because, from the perspective of the ABCI, ProcessProposal for the same block should always return the same result no matter the round. This is true because due to the determinism requirements, the output of ProcessProposal should exclusively depend on a) the state of the app at the beginning of the current height and
b) the data passed in the ABCI call. Consequently, as we do not pass round information in ProcessProposal and we talk about the same height, the output needs to be the same.

Since all proposed cases satisfy the minimal condition presented here, they are safe.

[hvanz]

• If the proposed block matches our ValidBlock

In terms of the pseudo-code, the final version of lines 23 and 29 would be:
23: if $(lockedRound_p = −1 ∧ valid(v)) ∨ lockedValue_p = v ∨ validValue_p = v$ then
and
29: if $lockedRound_p ≤ vr ∨ lockedValue_p = v ∨ validValue_p = v$ then

Is it correct?

[lasarojc]

Seems correct. But to make it explicit the short-circuiting that should take place, maybe we should 23 as

23: if validValuep=v ∨ (lockedRoundp=−1∧valid(v)) ∨ lockedValuep=v then

This will avoid the check when lockedRound = -1 but validValuep is already set to v, as in the following run.

• v1 is proposed in round 0
• every validator pre-votes for v1
• pre-votes are slow to reach all validator vA;
• vA locks and sets validValue to v1, but its pre-commit is slow to reach other nodes
• v2 is proposed and all validators but vA pre-vote for v2
• vA timeoutPropose and revotes nil
• vA receives 2f+1 prevotes, including its own prevote for nil. This triggers onTimeoutPropose
• vA timeoutPropose, precommit nil
• vA receives another non-nil pre-vote and executes line 36, does not update lockedValue but updates validValue
• all other nodes timeout and v2 is proposed again
• vA executes line 23 and is able to skip verifying v2 because it has been recorded on valueValue.

[sergio-mena]

Is it correct?

Thanks, I realise my initial pseudocode didn't account for $validValue$. I think it's not 100% correct, because $validValue$ should only affect the execution of $valid(v)$ not what we end up prevoting.

Or maybe it's equivalent (in term of logic algebra) to what you're proposing?

[lasarojc]

As discussed, the following rule for line 23 is probably the simpler, that does not change the algorithm

validVMatch = (validRoundp != -1 && validValuep=v)
if [lockedRoundp=−1 && (validVMatch || valid(v))] || lockedValuep=v then

[lasarojc]

We can go even further than proposed. The minimal condition would be receiving f+1 (worth of voting power) valid prevotes messages from different validators for the proposed block. They can even be from different rounds because, from the perspective of the ABCI, ProcessProposal for the same block should always return the same result no matter the round. This is true because due to the determinism requirements, the output of ProcessProposal should exclusively depend on a) the state of the app at the beginning of the current height and b) the data passed in the ABCI call. Consequently, as we do not pass round information in ProcessProposal and we talk about the same height, the output needs to be the same.

Since all proposed cases satisfy the minimal condition presented here, they are safe.

Thanks for your comment, @nenadmilosevic95
We agree with you that the condition might be simplified, with implications in lines 23, 29, 36, and 50.
However, since stronger conditions are already required in lines 28, 36, and 49, there would be no gains there, unless these conditions are also weakened, which would require really changing the algorithm and is out of the scope of this PR.

There could be gains in line 23, but that would require more involved changes and would also help only in the uncommon case of needing multiple rounds to decide. Hence we need to further discuss this change as well, possibly in the context of porting PBTS. It is non-blocking for this PR.

Also, I would like to note that even if ProcessProposal is non-deterministic, your proposal would be valid. The reason is validators don't really reject invalid proposals, but simply abstain from voting on them. So as long as the number of acceptance votes grows across rounds, at some point the value will be considered valid without querying the app, but this should also be further discussed in the context of porting PBTS and closing #1174