Switch from bls12_381 to bn254 #11
Conversation
brunoffranca
changed the title
brunoffranca
changed the title
brunoffranca
changed the title
brunoffranca
changed the title
|
@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. |
# Conflicts: # node/actors/consensus/src/leader/error.rs # node/actors/consensus/src/replica/error.rs
moshababo
force-pushed
the
bn254
branch
2 times, most recently
from
f9c1f3b to
1cfbeb3
Compare
moshababo
changed the title
| 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.
Is multiplication constant-time? Otherwise, the secret key may be leaked via timings side-channel.
There was a problem hiding this comment.
Implementation is here, and it isn't constant-time.
@pompon0 @brunoffranca any thoughts on this?
There was a problem hiding this comment.
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.
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.
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.
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.
Off-topic(?): Is PartialOrd / Ord required only for BTreeMaps 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.
Is
PartialOrd/Ordrequired only forBTreeMaps inPrepareQC?
Yes.
Could
HashMapbe 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.
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.
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_messagesshould pass)Improvebn254::testsRemovebls12_381from protobuf schemasRun integration testsClosing BFT-325.