feat: build commit from attestations #178
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @randygrok, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a fundamental change to how CometBFT block commits are generated, moving towards a more robust and verifiable system. It integrates soft confirmation attestations from the network module directly into the commit process, enhancing the integrity and finality of blocks. This involves extending the network module's query capabilities and updating the core RPC logic to leverage these new data sources for building cryptographically sound commits.
Highlights
- Core Logic for Commit Generation: The
CommitRPC endpoint has been significantly refactored to dynamically build the CometBFTCommitobject. This process now involves querying the network module for soft confirmation status and attestation bitmaps, replacing a previous static commit generation and re-signing process. This ensures that final block commits accurately reflect validator participation. - New Network Module Query: A new gRPC query,
ValidatorSignature, has been introduced within the network module. This allows clients to retrieve a specific validator's signature for a given block height, which is crucial for constructing the full CometBFT commit. - Attestation-based Commit Signatures: The system now leverages attestation bitmaps to identify which validators have participated in soft confirmation. The
buildCommitFromAttestationsfunction uses this information to include the corresponding validator signatures in the final CometBFT commit, enhancing the cryptographic proof of block finality. - RPC Endpoint for Validator Signatures: A new HTTP GET endpoint,
/rollkit/network/v1/signature/{height}/{validator}, has been exposed via gRPC-gateway. This provides a convenient way to query individual validator signatures for a specific height and validator address.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a new feature to build a commit from attestations. The changes include adding a new gRPC endpoint to query validator signatures and updating the Commit RPC. The implementation in pkg/rpc/core/blocks.go has critical issues that need to be addressed, including potential panics in error handling and an incomplete buildCommitFromAttestations function.
| if err != nil || abciRes.Code != 0 { | ||
| var msg string | ||
| if abciRes != nil { | ||
| msg = abciRes.Log | ||
| } | ||
| env.Logger.Error("query soft confirmation status", "height", height, "error", err, "log", msg) | ||
| return false, nil, fmt.Errorf("failed to query soft confirmation status: %w", err) | ||
| } |
There was a problem hiding this comment.
There is a potential panic here. If env.Adapter.App.Query returns a non-nil error, abciRes might be nil, and accessing abciRes.Code will cause a panic. Split the error handling into two separate checks.
if err != nil {
return false, nil, fmt.Errorf("failed to query soft confirmation status: %w", err)
}
if abciRes.Code != 0 {
msg := ""
if abciRes != nil {
msg = abciRes.Log
}
env.Logger.Error("query soft confirmation status failed", "height", height, "log", msg)
return false, nil, fmt.Errorf("failed to query soft confirmation status: %s", msg)
}| if err != nil || abciRes.Code != 0 { | ||
| var msg string | ||
| if abciRes != nil { | ||
| msg = abciRes.Log | ||
| } | ||
| env.Logger.Error("query attestation bitmap", "height", height, "error", err, "log", msg) | ||
| return false, nil, fmt.Errorf("failed to query attestation bitmap: %w", err) | ||
| } |
There was a problem hiding this comment.
Similar to the previous query, this error handling can cause a panic. If env.Adapter.App.Query returns an error, abciRes can be nil, and accessing abciRes.Code or abciRes.Log will result in a panic. Separate the error handling.
if err != nil {
return false, nil, fmt.Errorf("failed to query attestation bitmap: %w", err)
}
if abciRes.Code != 0 {
msg := ""
if abciRes != nil {
msg = abciRes.Log
}
env.Logger.Error("query attestation bitmap failed", "height", height, "log", msg)
return false, nil, fmt.Errorf("failed to query attestation bitmap: %s", msg)
}
pkg/rpc/core/blocks.go
Outdated
| // buildCommitFromAttestations constructs a commit with real signatures from attestations | ||
| func buildCommitFromAttestations(ctx context.Context, height uint64, attestationData *networktypes.QueryAttestationBitmapResponse) (*cmttypes.Commit, error) { | ||
| // Get the attestation bitmap | ||
| bitmap := attestationData.Bitmap.Bitmap | ||
| if bitmap == nil { | ||
| return nil, fmt.Errorf("no attestation bitmap found for height %d", height) | ||
| } | ||
|
|
||
| // Query all validators to get their addresses | ||
| queryReq := &abci.RequestQuery{ | ||
| Path: "/cosmos.staking.v1beta1.Query/Validators", | ||
| Data: []byte{}, // Empty request to get all validators | ||
| } | ||
|
|
||
| valQueryRes, err := env.Adapter.App.Query(ctx, queryReq) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to query validators: %w", err) | ||
| } | ||
| if valQueryRes.Code != 0 { | ||
| return nil, fmt.Errorf("failed to query validators: %s", valQueryRes.Log) | ||
| } | ||
|
|
||
| // For now, we'll construct a basic commit structure with the available data | ||
| // In a real implementation, you'd need to decode the validator response properly | ||
|
|
||
| votes := make([]cmttypes.CommitSig, 0) | ||
|
|
||
| // We need to iterate through the bitmap and for each set bit, get the validator signature | ||
| for i := 0; i < len(bitmap)*8; i++ { | ||
| // Check if validator at index i voted (bit is set) | ||
| if (bitmap[i/8] & (1 << (i % 8))) != 0 { | ||
| // This validator voted, let's try to get their signature | ||
| // For this we need the validator address corresponding to index i | ||
| // This would require a proper validator index mapping | ||
|
|
||
| // For now, we'll create a placeholder vote with empty signature | ||
| // In a real implementation, you'd: | ||
| // 1. Get validator address from index i | ||
| // 2. Query the signature using ValidatorSignature query | ||
| // 3. Construct the proper CommitSig | ||
|
|
||
| vote := cmttypes.CommitSig{ | ||
| BlockIDFlag: cmttypes.BlockIDFlagCommit, | ||
| ValidatorAddress: make([]byte, 20), // Placeholder validator address | ||
| Timestamp: time.Now(), // Should be actual timestamp | ||
| Signature: nil, // We'll get this from the query below | ||
| } | ||
|
|
||
| // Try to get the real signature (this is a simplified approach) | ||
| // In practice, you'd need to map the bitmap index to validator address | ||
| validatorAddr := fmt.Sprintf("validator_%d", i) // Placeholder | ||
| signature, err := getValidatorSignatureFromQuery(ctx, int64(height), validatorAddr) | ||
| if err == nil { | ||
| vote.Signature = signature | ||
| } | ||
|
|
||
| votes = append(votes, vote) | ||
| } else { | ||
| // Validator didn't vote, add nil vote | ||
| votes = append(votes, cmttypes.CommitSig{ | ||
| BlockIDFlag: cmttypes.BlockIDFlagAbsent, | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| commit := &cmttypes.Commit{ | ||
| Height: int64(height), | ||
| Round: 0, // Default round | ||
| BlockID: cmttypes.BlockID{ | ||
| Hash: make([]byte, 32), // Should be actual block hash | ||
| PartSetHeader: cmttypes.PartSetHeader{ | ||
| Total: 1, | ||
| Hash: make([]byte, 32), | ||
| }, | ||
| }, | ||
| Signatures: votes, | ||
| } | ||
|
|
||
| return commit, nil | ||
| } |
There was a problem hiding this comment.
This function is incomplete and contains placeholders, preventing correct operation.
- The validator address used for querying signatures is a placeholder:
validatorAddr := fmt.Sprintf("validator_%d", i). - The
ValidatorAddressin theCommitSigis a placeholder:make([]byte, 20). - The
Timestampis set totime.Now(), which is incorrect. - A query for all validators is made, but the result is not used to map the bitmap index to a validator address.
- Errors from
getValidatorSignatureFromQueryare silently ignored.
Overview