Conversation
|
marking as R4R to get reviews in, but there will be further doc changes |
cmwaters
left a comment
There was a problem hiding this comment.
Looking good so far. I assume there are more changes to come but I have a few comments in the meantime
| RequestCheckTx check_tx = 7; | ||
| RequestDeliverTx deliver_tx = 8 [deprecated = true]; | ||
| RequestEndBlock end_block = 9 [deprecated = true]; | ||
| RequestCommit commit = 10; |
There was a problem hiding this comment.
Isn't this also being deprecated?
There was a problem hiding this comment.
What's the flow here? something like this:
- Receive 2/3+ precommits
- Execute
FinalizeBlock - Persist the block and state changes
- Execute
Commit
There was a problem hiding this comment.
yes, this would be the flow
| enum Result { | ||
| UNKNOWN = 0; // Unknown result, treat as ACCEPT by default | ||
| ACCEPT = 1; // Vote extension verified, include the vote | ||
| SLASH = 2; // Vote extension verification aborted, continue but slash validator |
There was a problem hiding this comment.
What does this mean for tendermint? i.e. what does tendermint do when it gets a SLASH response?
| RequestOfferSnapshot offer_snapshot = 12; | ||
| RequestLoadSnapshotChunk load_snapshot_chunk = 13; | ||
| RequestApplySnapshotChunk apply_snapshot_chunk = 14; | ||
| RequestPrepareProposal prepare_proposal = 15; |
There was a problem hiding this comment.
Tag 15 was removed in 82e4693c, we should probably not re-use it.
Was:
RequestApplySnapshotChunk apply_snapshot_chunk = 15;
We can use a reserved clause to avoid old tags in the future.
I'm not sure if this is the only one, but it's the only one I found offhand.
There was a problem hiding this comment.
changing the number format in major releases, while not preferable, should be fine. Apps should refetch all the proto files when updating otherwise they could encounter other errors.
There was a problem hiding this comment.
Yeah it seems like we actually removed set_option:
RequestSetOption set_option = 4;
and pushed everything up so really we should reserve 4 but I think we can get away with this (we already did this in 0.34).
There was a problem hiding this comment.
I guess as long as chains don't persist old ABCI messages (just their contents) it's probably OK. We should probably get in the habit of not reusing tag numbers, though. That's one of the hardest breaking changes to detect and fix in protobuf.
|
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 reverts commit caaafc4.
* abci++ proto updates * add finalizeblock * add deprecated fields
* abci++ proto updates * add finalizeblock * add deprecated fields
* abci++ proto updates * add finalizeblock * add deprecated fields
* abci++ proto updates * add finalizeblock * add deprecated fields
* abci++ proto updates * add finalizeblock * add deprecated fields
Description
Add proto files to reflect changes being made for abci++