This is somewhat orthogonal to abci++ but I still wanted to comment on this: I know that _p comes from the original bft paper but I'm wondering if we should just leave away the subscript here and express the proposer(h_p, round_p) = p slightly differently? IMO, it is clear that all this code runs in/on one "process" and dragging this subscript around everywhere just decreases readability. As I said, this highly unrelated to abci++ but I see these markdown files as becoming the place to look up how tendermint consensus works and how to implement it. So I'm in favour of expressing this as accessible and readable as possible (not necessarily in this PR though).

Hrmm, cc @milosevic @konnov @josef-widder for thoughts on this.

Its not super clear to me, since I think removing it may make it easier to conclude that when height / round increments, it increments for every validator.

(Its also not entirely clear to me, how / if ever this markdown will get merged)

I agree that the subscript _p is not optimal. However, it is crucial to see who is executing the code. This prefix is the only indication that the code is executed by a process p.

Given that some upon rules work as pattern matching, it is very important to distinguish the process variables (e.g., round_p) from those "temporary" variables (e.g., vr, r).

Speaking about the mempool, I think it would be great if the mempool connection was also mentioned somewhere (not necessarily in the pseudo code and just for the sake of completeness).

In the Tendermint paper, decision_p is a dynamically growing vector of values. I guess, that is why the authors wrote decision_p[] := nil in the paper. Can we write:

decision_p ← Vector()

Or Array(), or List(). I am fine with any choice here.

Set to vector for now, but I wonder if for technical correctness we should actually put Map, since its indexed by h_p, and we don't initialize at 0

The case of OnTimeoutPropose is missing.

Trying to clarify things for myself: Is v understood as a block header or a complete block? It would be great to have a comment that explains the expected behavior of ABCI.ProcessProposal(h_p, v).

Also, does ProcessProposal interact with mempool?

v here is understood as the complete block in v2. In v3 and v4 we can split up the methods according to when they are received in implementation, should we reflect this in the pseudocode?

(This would be done by changing the upon's usage of v to be v_header, and then if VerifyHeader passes, we have a line saying something to the effect of wait to receive the complete block proposal v that corresponds to v_header, and specifying that we cancel this wait if we get to TimeoutPropose)

ProcessProposal does not interact with the mempool, I'll add some more comments around here

I think that having v_header would help!

Could you add an example of precommit_extension in a comment?

Also, why would one need to extend the vote on nil? Is it a form of piggy-backing?

Sure, I'll add an example of precommit_extension!

You have a good point, I don't actually foresee a reason to extend a nil vote (since that precommit won't make it into a block).

That would only make sense in a consensus algorithm in which nil precommits made in time still get included in the next block, which doesn't happen at the moment. The only reasons I can imagine for this seem pretty contrived, so I'll remove it

Same here. An example of precommit_extension would be great.

should the call to ABCI.VerifyVoteExtension include the height h_p and value v as parameters?

Here the difference between the original pseudo-code and v0-v2 is crucial. In the original pseudo-code, a process could catch up with a round r that is different from round_p. I guess, you would like to avoid catch up here, as the application has partially processed the block for round_p.

If it is the case, we need a comment here.

Can a single validator run two instances of ProcessProposal at the same time? If so, what do we assume about the state of the ABCI app?

I think it should be impossible per the spec at the moment for this to happen? (I believe every process proposal process gets joined prior to incrementing rounds locally)

For instance, the last rule "Upon Receiving 2f + 1 precommits" can fire anytime. This would advance the height and start a new round. What happens to the ProcessProposal thread that is still running?

I think we should understand how the ABCI app is supposed to work with these requests:

1. The app processes requests one-by-one, that is, only a single ProcessProposal is working till it is joined, or
2. The app maintains potential future states (produced by different transactions) and then it finalizes one of them.

It looks like the pseudo code is written with Scenario 1 in mind. Then we have to be very careful about not sending concurrent requests to the app.

When I am saying "the app" here, I really mean the layer below Tendermint. So it is either the app or the ABCI layer resolving concurrency for the app.

We discussed in slack but just noting here the concern with this is that we could end up with a POLKA for a block that turns out to be invalid (ie. fails ProcessProposal) and this could have liveness consequences.

So we need to carefully work through the consequences of this and adjust as necessary or else just suffer the latency cost of completing ProcessProposal before prevoting.

It might be fine prevoting for v without waiting ProcessProposal(v) to return, but we should probably not set locked/valid value if ProcessProposal(v) hasn't returned successfully. Therefore, we ensure that validValue/lockedValue is never set to invalid block, and this ensures that invalid block is never committed.

What happens with precommit_extension upon commit?

They are handled by PrepareProposal, the data may be included within the block (either in the header or tx data), or not included at all and just have been there as an intermediate step.

should be v4?

and the updates in the following lines 3~5?