spec:ABCI2.0 - Uncommenting content related to vote xtensions and finalize block#421
Conversation
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
Co-authored-by: Lasaro <lasaro@informal.systems>
cason
left a comment
There was a problem hiding this comment.
I have left some comments, they should not block merging the PR, in particular since the most relevant were addressed by recent commits.
spec/abci/abci++_app_requirements.md
Outdated
| The sequence of `DeliverTx` calls is asynchronous but all those calls are enclosed by calls to `BeginBlock` and `EndBlock` which are synchronous. | ||
| --> | ||
|
|
||
| The Application must remember the latest height from which it |
There was a problem hiding this comment.
This could be moved to the next sub-section, right?
| to bound memory usage. As a general rule, the Application should be ready to discard candidate states | ||
| before block execution, even if one of them might end up corresponding to the | ||
| decided block and thus have to be reexecuted upon `BeginBlock-DeliverTx-EndBlock`. | ||
| before `FinalizeBlock`, even if one of them might end up corresponding to the |
spec/abci/abci++_app_requirements.md
Outdated
| When the consensus algorithm decides on a block, CometBFT uses `FinalizeBlock` to send the | ||
| decided block's data to the Application, which uses it to transition its state. | ||
|
|
||
| <!-- |
There was a problem hiding this comment.
We maybe state that this call is synchronous, isn't it?
| returned in `ResponsePrepareProposal.txs` . | ||
| * As a result of executing the prepared proposal, the Application may produce block events or transaction events. | ||
| The Application must keep those events until a block is decided. It will then forward the events to the `BeginBlock-DeliverTx-EndBlock` functions depending on where each event should be placed, thereby returning the events to CometBFT. | ||
| The Application must keep those events until a block is decided and then pass them on to CometBFT via |
There was a problem hiding this comment.
This here makes GitHub to format the remaining of this item list as a code. The comments, in particular, are rendered :D
| ABCI++ addresses these limitations by allowing the application to intervene at three key places of | ||
| consensus execution: (a) at the moment a new proposal is to be created and (b) at the moment a | ||
| proposal is to be validated. The new interface allows block proposers to perform application-dependent | ||
| proposal is to be validated, and (c) at the moment a (precommit) vote is sent/received. |
There was a problem hiding this comment.
This could be a numbered list, right?
I would try to avoid embarking in refactoring of the text structure at this point (this is supposed to be a backport)
| Note that violation of Requirement 1 may just lead to further rounds, but will not compromise correctness. | ||
|
|
||
| Requirement 1 is particularly important if *p*'s Application fully executes prepared blocks in `PrepareProposal` | ||
| as a form of optimistic execution. |
There was a problem hiding this comment.
| as a form of optimistic execution. | |
| as a form of immediate execution. |
There was a problem hiding this comment.
I believe that optimistic is more accurate here, because it signals that the execution may be wasted, while immediate does not. It is also a term commonly used for similar execution of transactions in DBs. I understand that we have used immediate in some other places so far, though, so would need to review them.
There was a problem hiding this comment.
I tend to agree with Lasaro, immediate sounds like the execution is final and the state change is persisted.
|
Thank you @sergio-mena , @cason and @lasarojc for reviewing. All the comments that were not applied (most of all Daniel's suggestions for text reformatting and the discussion on optimistic vs. immediate execution, I merged in a doc so we can get back to this later) but I will merge this as to unblock other minor PRs regarding the spec. |
* Applied last comments from PR #421 * Spelling errors
The PR will be a draft until PR #407 is merged. After that it will be rebased to
feature/abci++vef.Closes #403 .