-
Notifications
You must be signed in to change notification settings - Fork 39
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
Switch from bls12_381 to bn254 #11
Conversation
@pompon0 thanks for the review. Most of the code was temp and will be changed due to the replacement of the curve implementation, but I'll address the parts which will remain. |
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.
Looking good so far. The math is all correct.
# Conflicts: # node/actors/consensus/src/leader/error.rs # node/actors/consensus/src/replica/error.rs
f9c1f3b
to
1cfbeb3
Compare
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 to me. You just need to change the bls12_281 fields in the protobufs schema and it's good to go.
impl SecretKey { | ||
/// Gets the corresponding [`PublicKey`] for this [`SecretKey`] | ||
pub fn public(&self) -> PublicKey { | ||
let p = G2::generator() * self.0; |
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 multiplication constant-time? Otherwise, the secret key may be leaked via timings side-channel.
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.
Implementation is here, and it isn't constant-time.
@pompon0 @brunoffranca any thoughts on this?
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.
seems like a good point, is the library that we are using here actually suitable for cryptographic applications?
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.
for example, blst has this protection explicitly stated in its specification (although under "SHOULD" quantifier): https://datatracker.ietf.org/doc/html/draft-irtf-cfrg-bls-signature#name-side-channel-attacks
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.
No, it's not constant time. There's no way we are going to find a Rust library for BN254 that is constant time.
This crate is appropriate for use in production. The arkworks libraries are used in several blockchain and non-blockchain projects. But being resistant to side-channel attacks is a whole different level.
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.
zcash's lib implementation is similarly not constant-time: https://github.com/zcash-hackworks/bn/blob/ebf8b553f7e66f59e6221cf9b7a80bb31ff393a5/src/groups/mod.rs#L253
} | ||
} | ||
|
||
impl PartialOrd for AggregateSignature { |
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.
Off-topic(?): Is PartialOrd
/ Ord
required only for BTreeMap
s in PrepareQC
? Could HashMap
be used there? It's not quite obvious why signatures should be ordered semantically.
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
PartialOrd
/Ord
required only forBTreeMap
s inPrepareQC
?
Yes.
Could
HashMap
be used there? It's not quite obvious why signatures should be ordered semantically.
I didn't find use case for the current type of ordering. @pompon0 @brunoffranca any thoughts?
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.
We need reserialization of signable messages (like PrepareQC) to be an identity, so that signatures are valid at all times. This is kind of constraining, but the alternative would be to pass around the serialized message at all times so that reserialization is never needed (which is in fact more bullet-proof, but a bit costly and makes message handling more complex).
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.
+1
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 don't know if you copied over any code to this pr from external repos, but if you did please familiarize yourself with https://www.notion.so/matterlabs/Dependency-Attribution-dcb684a5455f483ba370c637c443157f
No code was copied. |
Switching [bn254](#11) implementation from [crates/ark-bn254](https://crates.io/crates/ark-bn254) to [matter-labs/pairing](https://github.com/matter-labs/pairing) due to compatibility issues of [zksync-era](https://github.com/matter-labs/zksync-era) with the former. ### Notes: * `bn254::tests::byte_fmt_correctness` tests were added, previously not covered
Switching from bls12_381 to bn254 for validator keys due to EIP-2537 delays, making the former incompatible with EVM.
This PR implements BLS signature scheme for bn254.
Notes:
TODO:
Replace the underlying crates/bn254 with crates/ark-bn254Verify that signature aggregation can support different messages (the newly addedbn254::tests::aggregate_signature_distinct_messages
should pass)Improvebn254::tests
Removebls12_381
from protobuf schemasRun integration testsClosing BFT-325.