feat: run for higher epoch_limit #2
Conversation
There was a problem hiding this comment.
LGTM! Thank you.
Though I noticed there are few places where clippy & fmt may be unhappy.
I pushed a Rust CI to check this. You'll probably also need to pull last changes to pass the checks (as I added Cargo.toml in root, to make it workspace).
Thanks, will wait for your fixes and then I'll merge it.
|
@ssjeon-p As you see Thank you! |
|
@curryrasul, |
versionA/src/main.rs
Outdated
| fn recover_key(shares: [(Fr, Fr); (EPOCH_LIMIT + 1) as usize]) -> Fr { | ||
| let (x1, y1) = shares[0]; | ||
| let (x2, y2) = shares[1]; | ||
| fn recover_key(shares: Vec<(Fr, Fr)>) -> Fr { |
There was a problem hiding this comment.
shares should be &[(Fr, Fr)] - slice - as it's immutable. It's a good practice.
versionA/src/main.rs
Outdated
|
|
||
| if messages.len() > self.limit as usize { | ||
| let key = Self::recover_key([messages[0], messages[1]]); | ||
| let key = Self::recover_key(messages.to_vec()); |
There was a problem hiding this comment.
It's not good to do to_vec() without good reason, as it clones whole vector. If recover_key function takes just slice (as I commented further) it would be possible to just pass messages.
There was a problem hiding this comment.
I understand. I removed this and some other useless cloning.
| } | ||
| } | ||
|
|
||
| fn determinant(mut matrix: Vec<Vec<Fr>>) -> Fr { |
There was a problem hiding this comment.
Same thing here, don't need to take ownership of Vector, can just get slice
|
|
||
| let numerator = y2 * x1 - y1 * x2; | ||
| let denominator = x1 - x2; | ||
| let denominator = determinant(matrix.clone()); |
There was a problem hiding this comment.
If determinant() takes slice - you won't need to clone matrix
There was a problem hiding this comment.
The determinant function manipulates the matrix, but I need the original matrix one more time.
I think this needs cloning here.
versionA/src/main.rs
Outdated
| let numerator = y2 * x1 - y1 * x2; | ||
| let denominator = x1 - x2; | ||
| let denominator = determinant(matrix.clone()); | ||
| _ = std::mem::replace(&mut matrix[0], vec_y); |
There was a problem hiding this comment.
Why not just
matrix[0] = vec_y;?
I didn't test it, but am I missing something ?
There was a problem hiding this comment.
That simple code didn't work previously for some reason.
But, as I tested it now, it works properly.
|
Thank you for the detailed review! |
2 participants
This runs in the case EPOCH_LIMIT > 1.
To recover key f(0), this uses the Cramer's rule for Vandermonde matrix(https://en.wikipedia.org/wiki/Vandermonde_matrix#Applications).
I could not find a proper mod for determinant, so copy and modify "https://github.com/EdgarBarrantes/field-matrix"