-
Notifications
You must be signed in to change notification settings - Fork 375
chore(crypto): CRP-1470 CRP-2305 Support compact CBOR encoding for commitment openings #8179
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
…mmitment openings
|
Since now commitment openings are only stored in memory it's quite likely we don't really need a transition strategy here, but to me it still feels a little safer. The PR changes the default encoding for P-256 to use bytestrings (since we don't have a P256 key yet) and uses the old encoding for K256 and Ed25519. There are two todos that would be addressed
We could alternately just do it all at once on the assumption that EccScalarBytes is really not serialized to disk anywhere. This would be a rather simpler patch. |
fspreiss
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, @randombit! Even though we assume that CspSecretKey::IDkgCommitmentOpening(CommitmentOpeningBytes) are only stored in the canister secret key store, which is only ever persisted in memory and not on disk, and thus this backwards-compatible de-serialization should not ever be needed, I'm OK with going the fail-safe way and adding it anyway.
rs/crypto/internal/crypto_lib/threshold_sig/canister_threshold_sig/tests/group.rs
Outdated
Show resolved
Hide resolved
rs/crypto/internal/crypto_lib/threshold_sig/canister_threshold_sig/src/utils/group.rs
Outdated
Show resolved
Hide resolved
rs/crypto/internal/crypto_lib/threshold_sig/canister_threshold_sig/src/utils/group.rs
Outdated
Show resolved
Hide resolved
|
Please add a PR description before merging: maybe all that is needed is to copy/paste most/parts of this 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.
Pull request overview
This PR implements support for compact CBOR encoding of commitment openings in threshold ECDSA/Schnorr signatures. The change enables deserialization of both compact (CBOR byte string) and non-compact (CBOR array) encodings while immediately switching P256 to use compact serialization since no P256 keys exist on mainnet yet.
Changes:
- Custom serialization/deserialization implementation for
EccScalarBytesto support both compact and legacy CBOR formats - Updated serialization tests to reflect P256's new compact encoding format
- Added comprehensive tests validating both old and new CBOR format deserialization
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| rs/crypto/internal/crypto_lib/threshold_sig/canister_threshold_sig/src/utils/group.rs | Implements custom Serialize and Deserialize traits for EccScalarBytes with support for both compact and legacy CBOR formats, with P256 using compact serialization |
| rs/crypto/internal/crypto_lib/threshold_sig/canister_threshold_sig/tests/group.rs | Adds tests verifying deserialization of both old array-based and new compact CBOR formats for all curve types |
| rs/crypto/internal/crypto_lib/threshold_sig/canister_threshold_sig/tests/serialization.rs | Updates expected serialization output for P256 commitment openings to reflect compact CBOR encoding |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I asked @andreacerulli about this today in our 1:1 he is ok with proceeding with the slow transition approach, so I will merge. |
Due to limitations in Rust and serde, the default encoding of bytestrings used by
serde_cboris a variable length integer encoding rather than using CBOR's native support for byte strings. Most other code in our system makes use ofserde_byteswhich allows encoding as compact strings, but this was missed for the commitment openings of threshold ECDSA/Schnorr.This change converts is so that both compact and non-compact encodings of commitment openings can be deserialized. It does not change the default serialization. The plan is to first merge this, then after a period of time change the default serialization to the compact form, and then finally after a suitable interval remove support for the old encodings.
This change the encoding of openings for P-256 immediately to the compact encoding, since no P-256 key exists on mainnet yet.