-
Notifications
You must be signed in to change notification settings - Fork 52
docs: cip-Privval Interface Extension for Arbitrary Message Signing #337
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
Hello @evan-forbes, 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 new Celestia Improvement Proposal (CIP-40) that outlines an extension to the CometBFT privval.Message interface. The primary goal is to enable validators to securely sign arbitrary off-chain message hashes using their existing consensus keys, thereby facilitating the development of high-throughput, low-latency gossiping protocols while maintaining backward compatibility.
Highlights
- New CIP Document: Adds CIP-40, titled "Privval Interface Extension for Arbitrary Message Signing," which details a standard for extending the CometBFT
privvalinterface. - Interface Extension: Specifies the addition of
SignP2PMessageRequestandSignedP2PMessageResponseprotobuf messages to theprivval.Messageinterface, allowing for the signing of generic P2P message hashes. - Off-chain Protocol Support: Enables validators to leverage their consensus keys and Key Management Systems (KMS) to sign messages for off-chain protocols, such as Full Mesh Overlay (FMO) and Vacuum!, without affecting on-chain state.
- Design Principles: Emphasizes hash-based signing, inclusion of
chain_idfor security, an optionalunique_idfor flexibility, and clarifies that double-signing protection is not required for these off-chain messages. - Security and Compatibility: Addresses key reuse, signature verification, DoS protection, and key compromise, while ensuring full backward compatibility with existing KMS implementations.
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 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 or fill out our survey 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This PR introduces CIP-40, which proposes an extension to the CometBFT privval interface for signing arbitrary messages. The proposal is well-structured and detailed. My review includes a few suggestions to improve the document's clarity and completeness, such as standardizing the author format, adding relevant discussion and implementation links, and clarifying the definition of an external type used in the protobuf specification.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
cips/cip-040.md
Outdated
|
|
||
| - Maintain consistent message size regardless of original content length | ||
| - Ensure cryptographic properties of the signed data | ||
| - Simplify KMS implementations by avoiding message content validation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does each validator get the raw message in addition to the hash of the message before signing?
If not, is the validator blindly signing over a hash without access to the raw message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is only for the KMS, so the validator is the one verifying and serializing the message
the KMS doesn't have any verification, no. this is different from votes, which have a "water mark" to prevent double signing
cmwaters
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just concerned about the digest we sign over
|
|
||
| ## Backwards Compatibility | ||
|
|
||
| This proposal is fully backwards compatible. Existing KMS implementations will continue to function normally, as the new message types use previously unused field numbers in the protobuf oneof union. That being said, all repos that import the interface MUST update their implementions to at least use a noop. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it fully backwards compatible. I know that adding fields won't be breaking, but Celestia's block propagation depends on this. Users have to update their KMS in order to continue running a consensus node. You can't run a new consensus node with an older version of KMS right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Users have to update their KMS in order to continue running a consensus node.
technically speaking, this is not currently the case. they just can't propose blocks 😅 . we're discussing if this needs to be changed in celestiaorg/celestia-core#2243
old KMS will still work, and new KMS will work with old versions of comet. the compatibility is a bit messy, if you have other suggestions I'm open to not using the term backwards compatible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, perhaps they can continue to run and they just have noisy logs
cips/cip-040.md
Outdated
| ### Field Specifications | ||
|
|
||
| - `chain_id`: The chain identifier to prevent cross-chain signature reuse. | ||
| - `raw_bytes`: The digest that is signed over. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the digest sign over the chain_id, raw_bytes and unique_id (if present)?
Else a rogue consensus node could make the KMS double sign votes and proposals. I know we don't care about double signing these messages but it should not allow double signing of existing messages that have double signing protection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see good point, we can reword this field doc to clarify that this digest is not the entire digest and that the unique id and chain id are also included.
we can also reword the unique field id to mention that its not really optional, since its included in the sign bytes
Unique ID Field: The optional unique_id field allows protocols to differentiate between different message types or contexts without requiring separate interface extensions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see
message SignRawBytesRequest {
string chain_id = 1;
bytes raw_bytes = 2;
string unique_id = 3;
}
I forgot that uniqueID is actually optional since its a string and therefore can be "", same with chain-id
a rogue consensus node could make the KMS double sign votes and proposals.
then this makes sense to me if we're using strings across the board cc @rach-id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the fix here is to not make unique-id optional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a very good point 👍 making it optional will have the same vulnerability. Maybe we should generate a nonce on the KMS, prepend it before signature, return it along with the signed bytes. This would guarantee uniqueness and add random data even if the payload is trying to mimic another message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
making it optional will have the same vulnerability
yeah defo, I think the fix is make it not optional
do you think that fixes the issue? we could use a nonce, but the nonce must be known by all nodes verifying the signature as they must be the ones adding it to the sign bytes in order to get the benefits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see 89e64eb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The nonce can be included in the compact block and can be used during verification of the signature.
I meant making it required will not fully fix the issue as there could be edge cases where some vote bytes, for example, coincide with the protobuf encoding overhead (the wire type + the length) which would result in the same thing. It's very less likely but could be possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the protobuf thing is interesting, and might also apply to nonces then as well if they are an issue no?
either way, the chain-id is being added by the kms, so as long as that's the case I think we're fine and the original concern also isn't valid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-authored-by: Callum Waters <[email protected]>
Co-authored-by: CHAMI Rachid <[email protected]>
jcstein
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM aside from unresolved comments from @rach-id and @evan-forbes
cmwaters
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing my suggestions. LGTM!
| ### Sign Bytes Construction | ||
|
|
||
| The actual bytes that are signed MUST be constructed by concatenating: | ||
|
|
||
| ```text | ||
| sign_bytes = chain_id + unique_id + raw_bytes | ||
| ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 thanks
|
|
||
| ## Backwards Compatibility | ||
|
|
||
| This proposal is fully backwards compatible. Existing KMS implementations will continue to function normally, as the new message types use previously unused field numbers in the protobuf oneof union. That being said, all repos that import the interface MUST update their implementions to at least use a noop. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, perhaps they can continue to run and they just have noisy logs
…elestiaorg#337) Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Callum Waters <[email protected]> Co-authored-by: Josh Stein <[email protected]> Co-authored-by: CHAMI Rachid <[email protected]>

Overview
adds CIP-40
for editors