ABCI++ markdown integration into Tendermint (for spec)#327
ABCI++ markdown integration into Tendermint (for spec)#327cmwaters merged 9 commits intotendermint:masterfrom
Conversation
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
I am going to review this week. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
Hrmm, can we-reopen / merge this, and then iteratively update the markdown spec per review? Would rather it be in the branch then lost in a PR. I don't have perms to re-open the PR tho |
|
|
||
| ### ReceiveProposal | ||
|
|
||
| In the case where the local node is not locked on any round, the following is ran: |
There was a problem hiding this comment.
This rule is for the case when fresh proposal has been received (validRound in Proposal message is -1). Note that at line 42 lockedValue = v. Difference between this rule and the rule at line 54 is that in this case no proof of locking is needed (quorum of PREVOTE messages) as validRound = -1.
| } | ||
| ``` | ||
|
|
||
| In the case where the node is locked on a round, the following is ran: |
There was a problem hiding this comment.
In this case, the same as above, local node can have locked some value or not, but the importance is on the fact that proposed value need to be supported by the quorum of PREVOTE messages as validRound >= 0.
| } | ||
| broadcast ⟨PROPOSAL, h_p, round_p, proposal, validRound_p⟩ | ||
| } else { | ||
| schedule OnTimeoutPropose(h_p,round_p) to be executed after timeoutPropose(round_p) |
There was a problem hiding this comment.
We are lacking logic for OnTimeoutPropose.
Co-authored-by: Zarko Milosevic <zarko@informal.systems>
Co-authored-by: Zarko Milosevic <zarko@informal.systems>
Co-authored-by: Zarko Milosevic <zarko@informal.systems>
|
|
||
| ```go | ||
| upon ⟨PROPOSAL, h_p, round_p, v, −1) from proposer(h_p, round_p) while step_p = propose do { | ||
| if valid(v) ∧ (lockedRound_p = −1 ∨ lockedValue_p = v) { |
There was a problem hiding this comment.
We should clarify the semantic of valid(v) function, i.e., is it done purely at the Tendermint level (basically block header verification), or we assume it requires also application level checks.
| // getUnpreparedBlockProposal takes tx data, and fills in the unprepared header data | ||
| unpreparedProposal ← getUnpreparedBlockProposal(txdata) | ||
| // ABCI++: the proposer may reorder/update transactions in `unpreparedProposal` | ||
| proposal ← ABCI.PrepareProposal(unpreparedProposal) |
There was a problem hiding this comment.
Might be good idea adding note that PrepareProposal is adding latency on the critical path, and therefore, timeoutPropose would need to be updated accordingly. One idea would be allowing application to provide some hint to the Tendermint regarding duration of PrepareProposal so the timeoutPropose could be estimated more correctly.
| } else { | ||
| txdata ← mempool.GetBlock() | ||
| // getUnpreparedBlockProposal takes tx data, and fills in the unprepared header data | ||
| unpreparedProposal ← getUnpreparedBlockProposal(txdata) |
There was a problem hiding this comment.
I guess this is the same as getBlockProposal(txdata) in v0. Why we use different function name here?
|
|
||
| ```go | ||
| upon ⟨PROPOSAL, h_p, round_p, v, −1) from proposer(h_p, round_p) while step_p = propose do { | ||
| if valid(v) ∧ ABCI.ProcessProposal(h_p, v).accept ∧ (lockedRound_p = −1 ∨ lockedValue_p = v) { |
There was a problem hiding this comment.
I would suggest storing the result of ABCI.ProcessProposal in a local variable and using it at line 52, as this presentation might suggest two ABCI calls.
| } else { | ||
| broadcast ⟨PREVOTE, h_p, round_p, nil⟩ | ||
| // Include any slashing evidence that may be sent in the process proposal response | ||
| for evidence in ABCI.ProcessProposal(h_p, v).evidence_list { |
There was a problem hiding this comment.
I am not sure to see benefit of having evidence handling at this level. I guess we want to signal that evidences will be provided by the application earlier to Tendermint (compared to the current version where it is after block is committed), right?
| ```go | ||
| upon ⟨PROPOSAL, h_p, r, v, *⟩ | ||
| from proposer(h_p, r) | ||
| AND 2f + 1 ⟨ PRECOMMIT, h_p, r, id(v)⟩ |
There was a problem hiding this comment.
I guess we need also precommit_extension field as part of PRECOMMIT message?
| In the case where the local node is not locked on any round, the following is ran: | ||
|
|
||
| ```go | ||
| upon ⟨PROPOSAL, h_p, round_p, v_header, −1) from proposer(h_p, round_p) while step_p = propose do { |
There was a problem hiding this comment.
From semantic perspective this presentation is not ideal as upon rules are atomic, so blocking within upon rule prevents any other rule being triggered. If we want to separate sending/receiving proposal_header from the proposal_data, we should probably introduce two message types.
| The following code is ran upon receiving 2f + 1 prevotes for the same block | ||
|
|
||
| ```go | ||
| upon ⟨PROPOSAL, h_p, round_p, v, *⟩ |
There was a problem hiding this comment.
PROPOSAL message contains full block proposal in this rule, while above it is only header. As mentioned above, we can make it clear by having in addition to PROPOSAL, also PROPOSAL_HEADER message and then have a separate rule for PROPOSAL_HEADER message.
| upon ⟨PROPOSAL, h_p, round_p, v, −1) from proposer(h_p, round_p) while step_p = propose do { | ||
| if valid(v) ∧ ABCI.VerifyHeader(h_p, v.header) ∧ (lockedRound_p = −1 ∨ lockedValue_p = v) { | ||
| // We fork process proposal into a parallel process | ||
| Fork ABCI.ProcessProposal(h_p, v) |
There was a problem hiding this comment.
I would prefer using slightly different semantics to signal it is asynchronous computation: we can distinct between synchronous ABCI calls where we are synchronously waiting for result, or asynchronous where we are essentially sending a request and will receive reply later (eventually). I don't have strong preference how we can capture this, maybe using Future or Promise as a return value. Then instead of Join we can have upon rule contains condition based on response. The benefit is that there is no blocking wait inside the upon rule as it semantically prevents other upon rules being triggered.
| if (step_p = prevote) { | ||
| lockedValue_p ← v | ||
| lockedRound_p ← round_p | ||
| processProposalOutput ← Join ABCI.ProcessProposal(h_p, v) |
There was a problem hiding this comment.
I guess the invariant here is that we should not commit a block proposal before being processed successfully, right?
milosevic
left a comment
There was a problem hiding this comment.
I have left few comments but they don't necessarily need to be addressed as part of this PR. I would suggest to merge this PR and continue working in smaller PRs. Thanks a lot @ValarDragon for amazing work here!
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
Going to merge these changes so that we have a solid base that we can come back to if we need to make any tweaks. Some follow up points have already been highlighted in this issue. |
This is a continuation of #283. This recombines the changes in an easier to view format, namely:
v0.md contains a transcription of the Tendermint BFT paper into markdown directly.
v1.md contains how ABCI hooks in.
v2.md contains a conversion to single-threaded ABCI++, with no optimization for splitting up verification of the header and block data
v3.md contains the optimization for splitting up verification of the header and block data
v4.md contains the multi-threaded optimization for delaying the end of ProcessProposal.
The structure for reviewing this is to review it commit by commit. Each commit shows the update to one file, so that it may be incrementally reviewed. (As a side effect, it also involves copying the file as well)
The final commit here is resolving the remaining unaddressed comments from #283