Skip to content
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

Merged
merged 36 commits into from
Nov 6, 2023
Merged

Switch from bls12_381 to bn254 #11

merged 36 commits into from
Nov 6, 2023

Conversation

moshababo
Copy link
Contributor

@moshababo moshababo commented Oct 23, 2023

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-bn254
  • Verify that signature aggregation can support different messages (the newly added bn254::tests::aggregate_signature_distinct_messages should pass)
  • Improve bn254::tests
  • Remove bls12_381 from protobuf schemas
  • Run integration tests

Closing BFT-325.

@moshababo moshababo self-assigned this Oct 23, 2023
@brunoffranca brunoffranca changed the title [WIP] Switch from bls12_381 to bn254 [WIP] Switch from bls12_381 to bn254 (BFT-325) Oct 23, 2023
@brunoffranca brunoffranca changed the title [WIP] Switch from bls12_381 to bn254 (BFT-325) [WIP] Switch from bls12_381 to bn254 Oct 23, 2023
@brunoffranca brunoffranca changed the title [WIP] Switch from bls12_381 to bn254 [WIP] Switch from bls12_381 to bn254 (BFT-325) Oct 23, 2023
@brunoffranca brunoffranca changed the title [WIP] Switch from bls12_381 to bn254 (BFT-325) [WIP] Switch from bls12_381 to bn254 Oct 23, 2023
@moshababo
Copy link
Contributor Author

@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.

Copy link
Member

@brunoffranca brunoffranca left a 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.

node/libs/crypto/src/bn254_2/mod.rs Outdated Show resolved Hide resolved
@moshababo moshababo changed the title [WIP] Switch from bls12_381 to bn254 Switch from bls12_381 to bn254 Oct 29, 2023
@moshababo moshababo marked this pull request as ready for review October 29, 2023 23:06
brunoffranca
brunoffranca previously approved these changes Oct 30, 2023
Copy link
Member

@brunoffranca brunoffranca left a 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.

node/Cargo.toml Outdated Show resolved Hide resolved
node/deny.toml Outdated Show resolved Hide resolved
node/libs/crypto/src/bn254/hash.rs Outdated Show resolved Hide resolved
node/libs/crypto/src/bn254/hash.rs Outdated Show resolved Hide resolved
node/libs/crypto/src/bn254/mod.rs Outdated Show resolved Hide resolved
node/libs/crypto/src/bn254/mod.rs Outdated Show resolved Hide resolved
node/libs/crypto/src/bn254/testonly.rs Outdated Show resolved Hide resolved
node/libs/crypto/src/bn254/testonly.rs Outdated Show resolved Hide resolved
node/libs/crypto/src/bn254/testonly.rs Outdated Show resolved Hide resolved
node/libs/crypto/src/lib.rs Show resolved Hide resolved
node/libs/crypto/src/bn254/mod.rs Outdated Show resolved Hide resolved
impl SecretKey {
/// Gets the corresponding [`PublicKey`] for this [`SecretKey`]
pub fn public(&self) -> PublicKey {
let p = G2::generator() * self.0;
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Collaborator

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?

Copy link
Collaborator

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

Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

node/libs/crypto/src/bn254/mod.rs Outdated Show resolved Hide resolved
node/libs/crypto/src/bn254/mod.rs Outdated Show resolved Hide resolved
node/libs/crypto/src/bn254/mod.rs Outdated Show resolved Hide resolved
node/libs/crypto/src/bn254/mod.rs Show resolved Hide resolved
}
}

impl PartialOrd for AggregateSignature {
Copy link
Contributor

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 BTreeMaps in PrepareQC? Could HashMap be used there? It's not quite obvious why signatures should be ordered semantically.

Copy link
Contributor Author

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 for BTreeMaps in PrepareQC?

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?

Copy link
Collaborator

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

node/libs/crypto/src/bn254/hash.rs Outdated Show resolved Hide resolved
pompon0
pompon0 previously approved these changes Nov 2, 2023
Copy link
Collaborator

@pompon0 pompon0 left a 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

@moshababo
Copy link
Contributor Author

No code was copied.

@pompon0 pompon0 merged commit ccc3ee9 into main Nov 6, 2023
3 of 4 checks passed
@pompon0 pompon0 deleted the bn254 branch November 6, 2023 09:27
brunoffranca pushed a commit that referenced this pull request Nov 10, 2023
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants