PrepareProposal/ProcessProposal invocation during replay inconsistent with spec

[kostko]

During our E2E fuzz testing using the CometBFT 0.37.x branch, where one of the tests is randomly killing and restarting nodes, it seems that PrepareProposal/ProcessProposal invocation during replay is inconsistent with the spec.

The spec currently says:

For every round, if the local process is the proposer of the current round, CometBFT calls PrepareProposal, followed by ProcessProposal. These two always come together because they reflect the same proposal that the process also delivers to itself.

However, it seems that there is an edge case with the following scenario:

• A validator node becomes the proposer, calls PrepareProposal, generates and broadcasts the proposal (proposal A). Then the node crashes/is killed.
• When the node restarts, it enters the consensus replay mode.
• CometBFT replays the timeout event, triggering a new round where the validator node is again the proposer (as this is a replay, the height/round is the same). This causes the recovering validator node to run PrepareProposal again, generating a new proposal B. Since signing this proposal would be an equivocation, the proposal is dropped.
• CometBFT then replays proposal A, which causes it to be processed by the app via ProcessProposal.

This seems to contradict the specification, because based on it the following sequence would be expected in this scenario:

• B := PrepareProposal()
• ProcessProposal(B)
• ProcessProposal(A)

However, the actual sequence is:

• B := PrepareProposal()
• ProcessProposal(A)

So either the specification needs to be updated to clarify that these other sequences are possible or the implementation should be fixed.

[cason]

CometBFT then replays proposal A, which causes it to be processed by the app via PrepareProposal.

Are you sure that this happens? Or are you referring to ProcessProposal instead?

[kostko]

Yes, we have observed the replay process producing a timeout event which causes CometBFT to invoke PrepareProposal. Only then the replay process replays proposal A itself which triggers an invocation of ProcessProposal.

[cason]

So, during the regular operation the application produces A as a response for PrepareProposal. The node broadcasts A block then crashes.

When the node is restarted, it replays the conditions for starting the same round, therefore it invokes PrepareProposal again, to which the application, for instance, replies with B.

The problem, I assume, is that since the proposal for B cannot be signed, there is no ProcessProposal for B. And once block A was persisted to disk, it is replayed, causing ProcessProposal to be invoked for A.

I cannot see, however, a situation in which ProcessProposal can be called for B, since B cannot be signed, therefore it is not processed as a "received" message.

[kostko]

Yeah in that case the best thing would be to update the spec to explicitly mention that this can happen.

[cason]

CometBFT then replays proposal A, which causes it to be processed by the app via PrepareProposal.

I still think the wording here is wrong. The replayed proposal is passed to ProcessProposal, not PrepareProposal.

[cason]

However, the actual sequence is:

B := PrepareProposal()
ProcessProposal(A)

The actual sequence should include the event that happened before the crash, i.e., the first step here should be A := PrepareProposal(), then crash, then the sequence.

[cason]

In other words, we have two invocations of PrepareProposal, but only the output of one of them is passed to ProcessProposal.

[kostko]

CometBFT then replays proposal A, which causes it to be processed by the app via PrepareProposal.

I still think the wording here is wrong. The replayed proposal is passed to ProcessProposal, not PrepareProposal.

Oops sorry, yeah that was supposed to say ProcessProposal.

[cason]

If we introduce rounds on PrepareProposal, the spec should be updated accordingly. I am afraid, however, that with the current design we cannot ensure that we call PrepareProposal at most once per each round of consensus.

#882

[sergio-mena]

Some info on the reason for closing this issue for now.

After some internal discussions on the best way to tackle this, we decided that the best alternative was to fix the specification so that the replay case was properly covered (the spec as it was when this issue was created was inconsistent with the implementation). This work was done in #1033.

The main reason we went for this option in the short term is that fixing this problem in the Consensus replay implementation (which would allow us to keep the spec as it was) is far from trivial as you can appreciate from #1035.

[kostko]

Sounds like the right solution to me. Thanks!