Conversation
proto/tendermint/abci/types.proto
Outdated
| repeated bytes evidence = 2; | ||
|
|
||
| enum Result { | ||
| UNKNOWN = 0; // Unknown result, treat as ACCEPT by default |
There was a problem hiding this comment.
Why should this be treated as accept?
There was a problem hiding this comment.
For the backwards compatability -plain abci applications that returns empty result are considered to be staying on previous sdk version and should be passed, but still need to be separated from the explicit accept case? If you think we don't need such distinction, we can just use 0 field for ACCEPT.
There was a problem hiding this comment.
Personally, I think this should be handled by the wrapper in the application, tendermint should treat UNKNOWN as not being set by the application.
There was a problem hiding this comment.
How does the consensus engine behave when the result is UKNOWN. Does it error out because the application isn't properly set?
There was a problem hiding this comment.
We talked about this and agreed that UNKNOWN should instead be used to signal that the application isn't properly set up and that the Tendermint process should panic and exit accordingly
There was a problem hiding this comment.
We should panic on UNKNOWN instead of treating it as ACCEPT, as it means the application side was not properly implementing ABCI++ breaking changes.
tac0turtle
left a comment
There was a problem hiding this comment.
LGTM. left one question.
|
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. |
cmwaters
left a comment
There was a problem hiding this comment.
Looks mostly good to me.
PS: Don't forget to lint
proto/tendermint/abci/types.proto
Outdated
| repeated bytes evidence = 2; | ||
|
|
||
| enum Result { | ||
| UNKNOWN = 0; // Unknown result, treat as ACCEPT by default |
There was a problem hiding this comment.
How does the consensus engine behave when the result is UKNOWN. Does it error out because the application isn't properly set?
|
|
||
| stateMachineValidBlock, err := cs.blockExec.ProcessProposal(cs.ProposalBlock) | ||
| if err != nil { | ||
| cs.Logger.Error("state machine returned an error when trying to process proposal block", "err", err) |
There was a problem hiding this comment.
Is it okay just to log an error when this happens (and then continue)?
There was a problem hiding this comment.
Ok this is fine as we'll prevote nil (so we're failing in a safe way)
There was a problem hiding this comment.
We are voting nil and return at the next clause of if !stateMachineValidBlock, it is okay.
| return | ||
| } | ||
|
|
||
| stateMachineValidBlock, err := cs.blockExec.ProcessProposal(cs.ProposalBlock) |
There was a problem hiding this comment.
Is it possible to run process proposal on the same block? I'm wondering if there should be some form of cache. (I think we cache ValidateBlock currently)
There was a problem hiding this comment.
From convo w/ @cmwaters: merge BlockExec.ValidateBlock and BlockExec.ProcessProposal, as they are both validating block but in different positions - one in Tendermint and one in Application side. Also reuse the caching logic.
|
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. |
|
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. |
|
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. |
add processproposal proto/boilerplate/logic mockery gofmt fix test gofmt move UNKNOWN response behaviour to reject
add processproposal proto/boilerplate/logic mockery gofmt fix test gofmt move UNKNOWN response behaviour to reject
add processproposal proto/boilerplate/logic mockery gofmt fix test gofmt move UNKNOWN response behaviour to reject
add processproposal proto/boilerplate/logic mockery gofmt fix test gofmt move UNKNOWN response behaviour to reject
* Rebased and git-squashed the commits in PR #7091 - add processproposal proto/boilerplate/logic - mockery - gofmt - fix test - gofmt - move UNKNOWN response behaviour to reject * Fixed build of some UTs * Addressed William's comment on context * Adapted TestProcessProposal * BaseApp needs to ACCEPT vote extensions by default * Added missing ProcessProposal to socket_server.go * Re-renamed TwoThirdPrevote... to Valid... * Addressed William's comment on ProcessProposal error * Addressed Callum's comments * fmt Co-authored-by: mconcat <monoidconcat@gmail.com>
add processproposal proto/boilerplate/logic mockery gofmt fix test gofmt move UNKNOWN response behaviour to reject
add processproposal proto/boilerplate/logic mockery gofmt fix test gofmt move UNKNOWN response behaviour to reject
Please add a description of the changes that this PR introduces and the files that
are the most critical to review.
If this PR fixes an open Issue, please include "Closes #XXX" (where "XXX" is the Issue number)
so that GitHub will automatically close the Issue when this PR is merged.