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

refactor(crypto): change hash_to_point algorithm (BFT-404) #80

Merged
merged 4 commits into from
Apr 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions node/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions node/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,8 @@ kube = { version = "0.88.1", features = ["runtime", "derive"] }
k8s-openapi = { version = "0.21.0", features = ["latest"] }
jsonrpsee = { version = "0.21.0", features = ["server", "http-client"] }
tower = { version = "0.4.13" }
num-bigint = "0.4.4"
num-traits = "0.2.18"

# Note that "bench" profile inherits from "release" profile and
# "test" profile inherits from "dev" profile.
Expand Down
3 changes: 1 addition & 2 deletions node/actors/network/src/consensus/mod.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
//! Consensus network is a full graph of connections between all validators.
//! BFT consensus messages are exchanged over this network.
use crate::rpc::Rpc as _;
use crate::{config, gossip, io, noise, pool::PoolWatch, preface, rpc};
use crate::{config, gossip, io, noise, pool::PoolWatch, preface, rpc, rpc::Rpc as _};
use anyhow::Context as _;
use rand::seq::SliceRandom;
use std::{collections::HashSet, sync::Arc};
Expand Down
2 changes: 2 additions & 0 deletions node/libs/crypto/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ rand04.workspace = true
thiserror.workspace = true
tracing.workspace = true
ff_ce.workspace = true
num-bigint.workspace = true
num-traits.workspace = true

[dev-dependencies]
criterion.workspace = true
Expand Down
47 changes: 33 additions & 14 deletions node/libs/crypto/src/bn254/hash.rs
Original file line number Diff line number Diff line change
@@ -1,27 +1,46 @@
//! Hash operations.

use ff_ce::{Field, PrimeField, SqrtField};
use num_bigint::BigUint;
use num_traits::Num;
use pairing::{
bn256::{G1Affine, G1Compressed},
EncodedPoint,
bn256::{fq, Fq, FqRepr, G1Affine},
CurveAffine,
};
use sha3::Digest as _;

/// Hashes an arbitrary message and maps it to an elliptic curve point in G1.
pub(crate) fn hash_to_g1(msg: &[u8]) -> G1Affine {
for i in 0..256 {
// Hash the message with the index as suffix.
let bytes: [u8; 32] = sha3::Keccak256::new()
.chain_update(msg)
.chain_update((i as u32).to_be_bytes())
.finalize()
.into();
pub(crate) fn hash_to_point(msg: &[u8]) -> (G1Affine, u8) {
let hash: [u8; 32] = sha3::Keccak256::new().chain_update(msg).finalize().into();
Copy link
Member

Choose a reason for hiding this comment

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

64 bytes by IETF suggestion, or please cite the literature why non-uniform field element for X doesn't affect a protocol

Copy link
Member

@brunoffranca brunoffranca Mar 25, 2024

Choose a reason for hiding this comment

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

Why do we need 64 bytes? Maybe I'm misunderstanding something, but the prime field modulus is just 254 bits long. Generating a 64 bytes hash just seems pointless.


// Try to get a G1 point from the hash. The probability that this works is around 1/8.
let p = G1Compressed::from_fixed_bytes(bytes).into_affine();
if let Ok(p) = p {
return p;
let hash_num = BigUint::from_bytes_be(&hash);
let prime_field_modulus = BigUint::from_str_radix(
"30644e72e131a029b85045b68181585d97816a916871ca8d3c208c16d87cfd47",
16,
)
.unwrap();
let x_num = hash_num % prime_field_modulus;
let x_arr: [u64; 4] = x_num.to_u64_digits().try_into().unwrap();
let mut x = Fq::from_repr(FqRepr(x_arr)).unwrap();

for i in 0..255 {
let p = get_point_from_x(x);
if let Some(p) = p {
return (p, i);
}
x.add_assign(&Fq::one());
}

// It should be statistically infeasible to finish the loop without finding a point.
Copy link
Member

Choose a reason for hiding this comment

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

Same way, please cite the literature. Even though we know the numbers of group elements, I do not remember any estimates on the distance between X coordinates

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIU it’s about being quadratic residue, so distribution is about 50%. I’ll need some help in verifying that, and with finding the appropriate reference to literature.

unreachable!()
}

fn get_point_from_x(x: Fq) -> Option<G1Affine> {
// Compute x^3 + b.
let mut x3b = x;
x3b.square();
x3b.mul_assign(&x);
x3b.add_assign(&fq::B_COEFF);
// Try find the square root.
x3b.sqrt().map(|y| G1Affine::from_xy_unchecked(x, y))
Copy link
Member

Choose a reason for hiding this comment

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

Both +y and -y will give you a point on curve and in the correct subgroup. So you should pick and fix an ambiguity here, by e.g. picking one that is smaller as an integer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I refrained from that because it adds extra steps and so gas usage in the onchain verification.
Doesn't sqrt() always returns +, and so doing nothing with negation means we're picking it over -? (assuming there's no benefit in picking the smaller-as-integer value)

}
11 changes: 6 additions & 5 deletions node/libs/crypto/src/bn254/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ impl SecretKey {

/// Produces a signature using this [`SecretKey`]
pub fn sign(&self, msg: &[u8]) -> Signature {
let hash_point = hash::hash_to_g1(msg);
let sig = hash_point.mul(self.0);
let (msg_point, _) = hash::hash_to_point(msg);
let sig = msg_point.mul(self.0);
brunoffranca marked this conversation as resolved.
Show resolved Hide resolved
Signature(sig)
}
}
Expand Down Expand Up @@ -166,10 +166,10 @@ impl Signature {
// public key when constructing it, this should never fail (in theory).
pk.verify().unwrap();

let hash_point = hash::hash_to_g1(msg);
let (msg_point, _) = hash::hash_to_point(msg);

// First pair: e(H(m): G1, pk: G2)
let a = Bn256::pairing(hash_point, pk.0);
let a = Bn256::pairing(msg_point, pk.0);
// Second pair: e(sig: G1, generator: G2)
let b = Bn256::pairing(self.0, G2Affine::one());

Expand Down Expand Up @@ -246,7 +246,8 @@ impl AggregateSignature {
// Second pair: e(H(m1): G1, pk1: G2) * ... * e(H(m1000): G1, pk1000: G2)
let mut b = Fq12::one();
for (msg, pk) in pairs {
b.mul_assign(&Bn256::pairing(hash::hash_to_g1(msg), pk.0))
let (msg_point, _) = hash::hash_to_point(msg);
b.mul_assign(&Bn256::pairing(msg_point, pk.0))
}

anyhow::ensure!(a == b, "Aggregate signature verification failure");
Expand Down
6 changes: 4 additions & 2 deletions node/libs/crypto/src/ed25519/tests.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
use crate::ed25519::{PublicKey, SecretKey, Signature};
use crate::ByteFmt;
use crate::{
ed25519::{PublicKey, SecretKey, Signature},
ByteFmt,
};

#[test]
fn test_ed25519() -> anyhow::Result<()> {
Expand Down
Loading