-
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
refactor(crypto): change hash_to_point algorithm (BFT-404) #80
Conversation
.finalize() | ||
.into(); | ||
pub(crate) fn hash_to_point(msg: &[u8]) -> (G1Affine, u8) { | ||
let hash: [u8; 32] = sha3::Keccak256::new().chain_update(msg).finalize().into(); |
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.
64 bytes by IETF suggestion, or please cite the literature why non-uniform field element for X doesn't affect a protocol
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.
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.
} | ||
|
||
// It should be statistically infeasible to finish the loop without finding a point. |
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.
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
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.
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.
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)) |
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.
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
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 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)
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. But let's wait for @shamatar's approval of course.
What ❔
Changing the existing
hash_to_point
algorithm to the standard try-and-increment algorithm:x
coordinatey
coordinate by computing the square root ofx^3 + b
x + 1
Why ❔
The existing hash to curve algorithm, which turns 32 bytes to
G1::compressed
and maps it intoG1::uncompressed
isn’t valid; it doesn’t produce a point with an unknown discrete log.Notes
x
, are sufficient, or that it should be extended to 64 bytes, given the onchain verification gas usage implications.0x30644e72e131a029b85045b68181585d97816a916871ca8d3c208c16d87cfd47
/21888242871839275222246405745257275088696311157297823662689037894645226208583
) is defined in the base field type macro definition, but I found no way to access it, hence it is currently redefined in the implementation. This should be fixed.get_point_from_x
is already implemented forG1Affine
, but it's private and unaccessible. This should be fixed.