docs: documentation updates to abci #18625
Conversation
WalkthroughThe changes reflect a significant update to the Cosmos SDK documentation, specifically regarding the ABCI++ integration. The updates include guidance on new ABCI methods like Changes
Related issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on X ? TipsChat with CodeRabbit Bot (
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // logic in verifying a vote extension from another validator during the pre-commit | ||
| // phase. The response MUST be deterministic. An error is returned if vote | ||
| // extensions are not enabled or if verifyVoteExt fails or panics. | ||
| // We highly recommend a size validation due to performance degredation, | ||
| // see more here https://docs.cometbft.com/v0.38/qa/cometbft-qa-38#vote-extensions-testbed | ||
| func (app *BaseApp) VerifyVoteExtension(req *abci.RequestVerifyVoteExtension) (resp *abci.ResponseVerifyVoteExtension, err error) { | ||
| if app.verifyVoteExt == nil { |
There was a problem hiding this comment.
Change potentially affects state.
Call sequence:
(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).VerifyVoteExtension (baseapp/abci.go:644)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (4)
- UPGRADING.md (1 hunks)
- baseapp/abci.go (1 hunks)
- docs/build/abci++/00-introduction.md (1 hunks)
- docs/build/abci++/01-vote-extensions.md (2 hunks)
Files skipped from review due to trivial changes (2)
- UPGRADING.md
- docs/build/abci++/00-introduction.md
Additional comments: 3
baseapp/abci.go (1)
- 641-648: The PR objectives mention documentation updates for ABCI++ within the Cosmos SDK, but the provided code and summaries do not include any documentation changes. Please ensure that the documentation updates are included in the PR if they are part of the objectives.
docs/build/abci++/01-vote-extensions.md (2)
33-36:
The addition of hyperlinks and cautionary notes regarding the potential performance impact of large vote extensions aligns with the PR objectives and the summary of changes. This enhances the documentation by providing additional resources and important considerations for developers.57-57:
The inclusion of a recommendation for size validation in theVerifyVoteExtensionsection is a valuable addition that aligns with the PR objectives and the summary of changes. It provides guidance on maintaining performance, which is crucial for developers to consider when implementing vote extensions.
| // logic in verifying a vote extension from another validator during the pre-commit | ||
| // phase. The response MUST be deterministic. An error is returned if vote | ||
| // extensions are not enabled or if verifyVoteExt fails or panics. | ||
| // We highly recommend a size validation due to performance degredation, | ||
| // see more here https://docs.cometbft.com/v0.38/qa/cometbft-qa-38#vote-extensions-testbed |
There was a problem hiding this comment.
The comment in the VerifyVoteExtension function suggests implementing size validation to prevent performance degradation, but the actual validation logic is not present in the code. It would be beneficial to either implement the size validation or clarify the comment if the validation is expected to be handled elsewhere.
There was a problem hiding this comment.
Just a reminder to add a redirect for this in the docs repo, so that the previous links continue to work as well.
There was a problem hiding this comment.
yes perf, thank you for the reminder
| @@ -33,6 +33,9 @@ to consider the size of the vote extensions as they could increase latency in bl | |||
| production. See [here](https://github.com/cometbft/cometbft/blob/v0.38.0-rc1/docs/qa/CometBFT-QA-38.md#vote-extensions-testbed) | |||
| for more details. | |||
|
|
|||
| Click [here](https://docs.cosmos.network/main/user/tutorials/vote-extensions) if you would like a walkthrough of how to implement vote extensions. | |||
There was a problem hiding this comment.
This link will be available once the tutorials are merged
|
@tac0turtle's feedback looks good, thanks @samricotta |
There was a problem hiding this comment.
this document should be geared a bit closer to the cosmos sdk. Im not sure we should be writing high level cometbft docs in the sdk in the build section. Right now this seems like something that should live in learn but then it should be geared to our handlers and less so towards high level cometbft
|
@tac0turtle I think maybe you might need to have a look at the 2 separate tutorials I have open in the re second point, its a tough one because a lot of people who entered the hackathon asked questions, which would be answered by this text. They would also be considered more advanced users rather than beginners so im not sure having it in learn makes as much sense, coupled with that, to your point the docs need to be consolidated so not sure having abci++ everywhere is the best either. |
There was a problem hiding this comment.
messaged in slack, with server v2 this documentation will need to be updated to not mention abci as our code wont rely on abci anymore. the underlying consensus engine would need to support vote extensions prepare and process, but i could see others not supporting it with 100% compatibility but making it simpler and more modular than today.
might make sense to have build/advanced where it dives into modifying server and baseapp for now
samricotta
changed the title
|
@tac0turtle so should i move this to |
| Note, that the application defines the semantics of the `PrepareProposal` and it | ||
| MAY be non-deterministic and is only executed by the current block proposer. | ||
|
|
||
| Now, reading mempool twice in the previous sentence is confusing, lets break it down. |
There was a problem hiding this comment.
The note about reading "mempool" twice is a bit informal and could be rephrased for better clarity and formality.
| ## Process Proposal | ||
|
|
||
| `ProcessProposal` handles the validation of a proposal from `PrepareProposal`, | ||
| which also includes a block header. Meaning, that after a block has been proposed | ||
| the other validators have the right to vote on a block. The validator in the | ||
| default implementation of `PrepareProposal` runs basic validity checks on each | ||
| transaction. | ||
|
|
||
| Note, `ProcessProposal` MAY NOT be non-deterministic, i.e. it must be deterministic. | ||
| This means if `ProcessProposal` panics or fails and we reject, all honest validator | ||
| processes will prevote nil and the CometBFT round will proceed again until a valid | ||
| proposal is proposed. | ||
|
|
||
| Here is the implementation of the default implementation: | ||
|
|
||
| ```go reference | ||
| https://github.com/cosmos/cosmos-sdk/blob/v0.50.0-alpha.0/baseapp/abci_utils.go#L153-L159 | ||
| ``` | ||
|
|
||
| Like `PrepareProposal` this implementation is the default and can be modified by | ||
| the application developer in [`app.go`](./01-app-go-v2.md). If you decide to implement | ||
| your own `ProcessProposal` handler, you must be sure to ensure that the transactions | ||
| provided in the proposal DO NOT exceed the maximum block gas and `maxtxbytes` (if set). | ||
|
|
||
| ```go | ||
| processOpt := func(app *baseapp.BaseApp) { | ||
| abciPropHandler := baseapp.NewDefaultProposalHandler(mempool, app) | ||
| app.SetProcessProposal(abciPropHandler.ProcessProposalHandler()) | ||
| } | ||
|
|
||
| baseAppOptions = append(baseAppOptions, processOpt) | ||
| ``` |
There was a problem hiding this comment.
💡 NOTE
Codebase verification is a beta feature.
The output from the script indicates that the branch v0.50.0-alpha.0 does not exist in the repository, which means that the link provided in the documentation to the default implementation of ProcessProposal is incorrect, as it points to a non-existent branch. This is a significant issue because users following the documentation will not be able to access the referenced code.
Given this information, the correct action is to flag this issue to the author of the pull request so that they can update the documentation with the correct link.
Here is the review comment:
The link to the default implementation of ProcessProposal in the documentation points to a non-existent branch (v0.50.0-alpha.0) in the Cosmos SDK repository. This needs to be corrected to point to a valid branch or tag where the abci_utils.go file and the relevant code can be found.
- The provided link to the default implementation of
ProcessProposalis incorrect as the branchv0.50.0-alpha.0does not exist. The documentation should be updated with a valid link.
Analysis chain
The documentation updates in 02-process-proposal.md provide a clear explanation of the ProcessProposal method, its relationship with PrepareProposal, and the importance of determinism in its implementation. The inclusion of a link to the default implementation and guidance for application developers on customizing this handler is also helpful. Ensure that the link to the default implementation (line 17) is correct and that the referenced code aligns with the described behavior.
The output from the curl command indicates that the HTTP status code is 000, which is not a standard HTTP response code. This typically means that the curl command failed to make a connection to the server, which could be due to various reasons such as network issues, incorrect URL, or the resource at the URL not existing.
Given this result, we cannot confirm whether the link to the default implementation provided in the documentation is correct and accessible. To verify the correctness of the link, we should check if the file abci_utils.go exists in the specified repository and branch, and if the lines 153-159 contain the relevant default implementation of ProcessProposal.
Let's write a script to check the existence of the file and the content of the specified lines.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that the provided link to the default implementation is correct and accessible.
curl -o /dev/null -s -w "%{http_code}\n" https://github.com/cosmos/cosmos-sdk/blob/v0.50.0-alpha.0/baseapp/abci_utils.go#L153-L159
Length of output: 136
Script:
#!/bin/bash
# Check if the file abci_utils.go exists in the v0.50.0-alpha.0 branch of the cosmos-sdk repository
file_path="baseapp/abci_utils.go"
branch="v0.50.0-alpha.0"
# Use git to check if the file exists in the specified branch
git ls-tree -r $branch --name-only | grep "$file_path"
# If the file exists, use git to show the content of the specified lines
if git rev-parse --verify --quiet $branch; then
git show $branch:$file_path | sed -n '153,159p'
else
echo "Branch $branch does not exist."
fi
Length of output: 315
There was a problem hiding this comment.
Late to the party! (I was off most of December)
I traversed the docs and they look good to me. Since it's too late to leave suggestions in this PR, I made a branch with improvements. If there's capacity in the SDK team, please feel free to open a PR and adopt my changes. I did not find anything critical, just aesthetic improvements, so this is not urgent:
In any case, I'll start redirecting people with questions to these docs from now on.
Solid work Sam & team!
|
|
||
| `ProcessProposal` handles the validation of a proposal from `PrepareProposal`, | ||
| which also includes a block header. Meaning, that after a block has been proposed | ||
| the other validators have the right to vote on a block. The validator in the |
There was a problem hiding this comment.
I would not say 'vote' here. It can be confusing those unfamiliar with the Tendermint consensus voting process.
Might be simpler to say accept or reject that block.
The same language (accept/reject) is used in ABCI specs and default process proposal handler.
Description
This updates the flow and basic introduction to ABCI++ which is missing. This should be viewed in conjunction with the 2 abci++ tutorials cosmos/cosmos-sdk-docs#93 and cosmos/cosmos-sdk-docs#91 where the implementation details of adding the logic into the handlers live.
Note: now working out if prepare and process proposal needs to be moved from baseapp or if it still needs to remain and outlined here in a separate context.
Closes: #cosmos/cosmos-sdk-docs#87
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...