forked from anoma/ferveo
-
Notifications
You must be signed in to change notification settings - Fork 10
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
Validator ordering changes #172
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
830cbc8
feat(validator): add share_index field to validator
piotr-roslaniec 6670da7
feat(validator): enforce canonical share indices instead of validator…
piotr-roslaniec 935be2d
refactor(validator): replace dkg validator with validator
piotr-roslaniec 280c37e
chore: remove unused code
piotr-roslaniec 4af8017
refactor: unify share creating methods
piotr-roslaniec d547a81
chore: remove unused code
piotr-roslaniec e6a7f6e
feature(dkg): prevent panics during transcript aggregation
piotr-roslaniec 4bb4158
refactor: refactor aggregate method params
piotr-roslaniec 72b8484
chore: document todos
piotr-roslaniec File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,7 +30,7 @@ use crate::bindings_python; | |
use crate::bindings_wasm; | ||
pub use crate::EthereumAddress; | ||
use crate::{ | ||
do_verify_aggregation, Error, PVSSMap, PubliclyVerifiableParams, | ||
do_verify_aggregation, Error, Message, PVSSMap, PubliclyVerifiableParams, | ||
PubliclyVerifiableSS, Result, | ||
}; | ||
|
||
|
@@ -222,15 +222,21 @@ impl Dkg { | |
} | ||
|
||
pub fn generate_transcript<R: RngCore>( | ||
&self, | ||
&mut self, | ||
rng: &mut R, | ||
// TODO: Replace with Message::Deal? | ||
) -> Result<Transcript> { | ||
self.0.create_share(rng) | ||
match self.0.share(rng) { | ||
Ok(Message::Deal(transcript)) => Ok(transcript), | ||
Err(e) => Err(e), | ||
_ => Err(Error::InvalidDkgStateToDeal), | ||
} | ||
} | ||
|
||
pub fn aggregate_transcripts( | ||
&mut self, | ||
messages: &[ValidatorMessage], | ||
// TODO: Replace with Message::Aggregate? | ||
) -> Result<AggregatedTranscript> { | ||
// We must use `deal` here instead of to produce AggregatedTranscript instead of simply | ||
// creating an AggregatedTranscript from the messages, because `deal` also updates the | ||
|
@@ -242,7 +248,12 @@ impl Dkg { | |
for (validator, transcript) in messages { | ||
self.0.deal(validator, transcript)?; | ||
} | ||
Ok(AggregatedTranscript(crate::pvss::aggregate(&self.0.vss))) | ||
let pvss = messages | ||
.iter() | ||
.map(|(_, t)| t) | ||
.cloned() | ||
.collect::<Vec<PubliclyVerifiableSS<E>>>(); | ||
Ok(AggregatedTranscript(crate::pvss::aggregate(&pvss)?)) | ||
} | ||
|
||
pub fn public_params(&self) -> DkgPublicParameters { | ||
|
@@ -264,9 +275,13 @@ fn make_pvss_map(messages: &[ValidatorMessage]) -> PVSSMap<E> { | |
pub struct AggregatedTranscript(PubliclyVerifiableSS<E, crate::Aggregated>); | ||
|
||
impl AggregatedTranscript { | ||
pub fn new(messages: &[ValidatorMessage]) -> Self { | ||
let pvss_map = make_pvss_map(messages); | ||
AggregatedTranscript(crate::pvss::aggregate(&pvss_map)) | ||
pub fn new(messages: &[ValidatorMessage]) -> Result<Self> { | ||
let pvss_list = messages | ||
.iter() | ||
.map(|(_, t)| t) | ||
.cloned() | ||
.collect::<Vec<PubliclyVerifiableSS<E>>>(); | ||
Ok(AggregatedTranscript(crate::pvss::aggregate(&pvss_list)?)) | ||
} | ||
|
||
pub fn verify( | ||
|
@@ -327,7 +342,7 @@ impl AggregatedTranscript { | |
&ciphertext_header.0, | ||
aad, | ||
&validator_keypair.decryption_key, | ||
dkg.0.me.share_index, | ||
dkg.0.me.share_index as usize, | ||
&domain_points, | ||
&dkg.0.pvss_params.g_inv(), | ||
) | ||
|
@@ -344,12 +359,13 @@ impl AggregatedTranscript { | |
&ciphertext_header.0, | ||
aad, | ||
&validator_keypair.decryption_key, | ||
dkg.0.me.share_index, | ||
dkg.0.me.share_index as usize, | ||
&dkg.0.pvss_params.g_inv(), | ||
)?; | ||
let domain_point = dkg.0.domain.element(dkg.0.me.share_index as usize); | ||
Ok(DecryptionShareSimple { | ||
share, | ||
domain_point: dkg.0.domain.element(dkg.0.me.share_index), | ||
domain_point, | ||
}) | ||
} | ||
} | ||
|
@@ -425,6 +441,7 @@ mod test_ferveo_api { | |
.map(|(i, keypair)| Validator { | ||
address: gen_address(i), | ||
public_key: keypair.public_key(), | ||
share_index: i as u32, | ||
}) | ||
.collect::<Vec<_>>(); | ||
|
||
|
@@ -433,7 +450,7 @@ mod test_ferveo_api { | |
let messages: Vec<_> = validators | ||
.iter() | ||
.map(|sender| { | ||
let dkg = Dkg::new( | ||
let mut dkg = Dkg::new( | ||
tau, | ||
shares_num, | ||
security_threshold, | ||
|
@@ -619,6 +636,10 @@ mod test_ferveo_api { | |
assert!(result.is_err()); | ||
} | ||
|
||
// Note that the server and client code are using the same underlying | ||
// implementation for aggregation and aggregate verification. | ||
// Here, we focus on testing user-facing APIs for server and client users. | ||
|
||
#[test] | ||
fn server_side_local_verification() { | ||
let rng = &mut StdRng::seed_from_u64(0); | ||
|
@@ -637,6 +658,41 @@ mod test_ferveo_api { | |
assert!(local_aggregate | ||
.verify(dkg.0.dkg_params.shares_num(), &messages) | ||
.is_ok()); | ||
|
||
// Test negative cases | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These test cases were adapted from the server variant of tests above |
||
|
||
// Notice that the dkg instance is mutable, so we need to get a fresh one | ||
// for every test case | ||
|
||
// Should fail if no transcripts are provided | ||
let mut dkg = | ||
Dkg::new(TAU, SHARES_NUM, SECURITY_THRESHOLD, &validators, &me) | ||
.unwrap(); | ||
let result = dkg.aggregate_transcripts(&[]); | ||
assert!(result.is_err()); | ||
|
||
// Not enough transcripts | ||
let mut dkg = | ||
Dkg::new(TAU, SHARES_NUM, SECURITY_THRESHOLD, &validators, &me) | ||
.unwrap(); | ||
let not_enough_messages = &messages[..SECURITY_THRESHOLD as usize - 1]; | ||
assert!(not_enough_messages.len() < SECURITY_THRESHOLD as usize); | ||
let insufficient_aggregate = | ||
dkg.aggregate_transcripts(not_enough_messages).unwrap(); | ||
let result = insufficient_aggregate.verify(SHARES_NUM, &messages); | ||
assert!(result.is_err()); | ||
|
||
// Unexpected transcripts in the aggregate or transcripts from a different ritual | ||
// Using same DKG parameters, but different DKG instances and validators | ||
let mut dkg = | ||
Dkg::new(TAU, SHARES_NUM, SECURITY_THRESHOLD, &validators, &me) | ||
.unwrap(); | ||
let (bad_messages, _, _) = | ||
make_test_inputs(rng, TAU, SECURITY_THRESHOLD, SHARES_NUM); | ||
let mixed_messages = [&messages[..2], &bad_messages[..1]].concat(); | ||
let bad_aggregate = dkg.aggregate_transcripts(&mixed_messages).unwrap(); | ||
let result = bad_aggregate.verify(SHARES_NUM, &messages); | ||
assert!(result.is_err()); | ||
} | ||
|
||
#[test] | ||
|
@@ -650,7 +706,8 @@ mod test_ferveo_api { | |
let messages = &messages[..SECURITY_THRESHOLD as usize]; | ||
|
||
// Create an aggregated transcript on the client side | ||
let aggregated_transcript = AggregatedTranscript::new(messages); | ||
let aggregated_transcript = | ||
AggregatedTranscript::new(messages).unwrap(); | ||
|
||
// We are separating the verification from the aggregation since the client may fetch | ||
// the aggregate from a side-channel or decide to persist it and verify it later | ||
|
@@ -662,11 +719,15 @@ mod test_ferveo_api { | |
|
||
// Test negative cases | ||
|
||
// Should fail if no transcripts are provided | ||
let result = AggregatedTranscript::new(&[]); | ||
assert!(result.is_err()); | ||
|
||
// Not enough transcripts | ||
let not_enough_messages = &messages[..SECURITY_THRESHOLD as usize - 1]; | ||
assert!(not_enough_messages.len() < SECURITY_THRESHOLD as usize); | ||
let insufficient_aggregate = | ||
AggregatedTranscript::new(not_enough_messages); | ||
AggregatedTranscript::new(not_enough_messages).unwrap(); | ||
let result = insufficient_aggregate.verify(SHARES_NUM, messages); | ||
assert!(result.is_err()); | ||
|
||
|
@@ -675,7 +736,7 @@ mod test_ferveo_api { | |
let (bad_messages, _, _) = | ||
make_test_inputs(rng, TAU, SECURITY_THRESHOLD, SHARES_NUM); | ||
let mixed_messages = [&messages[..2], &bad_messages[..1]].concat(); | ||
let bad_aggregate = AggregatedTranscript::new(&mixed_messages); | ||
let bad_aggregate = AggregatedTranscript::new(&mixed_messages).unwrap(); | ||
let result = bad_aggregate.verify(SHARES_NUM, messages); | ||
assert!(result.is_err()); | ||
} | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -76,7 +76,6 @@ impl From<FerveoPythonError> for PyErr { | |
Error::InvalidTranscriptAggregate => { | ||
InvalidTranscriptAggregate::new_err("") | ||
} | ||
Error::ValidatorsNotSorted => ValidatorsNotSorted::new_err(""), | ||
Error::ValidatorPublicKeyMismatch => { | ||
ValidatorPublicKeyMismatch::new_err("") | ||
} | ||
|
@@ -109,6 +108,14 @@ impl From<FerveoPythonError> for PyErr { | |
"num_shares: {num_shares}, security_threshold: {security_threshold}" | ||
)) | ||
}, | ||
Error::DuplicatedShareIndex(index) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. New exceptions |
||
InvalidShareIndex::new_err(format!( | ||
"{index}" | ||
)) | ||
}, | ||
Error::NoTranscriptsToAggregate => { | ||
NoTranscriptsToAggregate::new_err("") | ||
}, | ||
}, | ||
_ => default(), | ||
} | ||
|
@@ -145,6 +152,7 @@ create_exception!(exceptions, InvalidByteLength, PyValueError); | |
create_exception!(exceptions, InvalidVariant, PyValueError); | ||
create_exception!(exceptions, InvalidDkgParameters, PyValueError); | ||
create_exception!(exceptions, InvalidShareIndex, PyValueError); | ||
create_exception!(exceptions, NoTranscriptsToAggregate, PyValueError); | ||
|
||
fn from_py_bytes<T: FromBytes>(bytes: &[u8]) -> PyResult<T> { | ||
T::from_bytes(bytes) | ||
|
@@ -396,8 +404,9 @@ impl Validator { | |
pub fn new( | ||
address: String, | ||
public_key: &FerveoPublicKey, | ||
share_index: u32, | ||
) -> PyResult<Self> { | ||
let validator = api::Validator::new(address, public_key.0) | ||
let validator = api::Validator::new(address, public_key.0, share_index) | ||
.map_err(|err| FerveoPythonError::Other(err.to_string()))?; | ||
Ok(Self(validator)) | ||
} | ||
|
@@ -486,7 +495,7 @@ impl Dkg { | |
DkgPublicKey(self.0.public_key()) | ||
} | ||
|
||
pub fn generate_transcript(&self) -> PyResult<Transcript> { | ||
pub fn generate_transcript(&mut self) -> PyResult<Transcript> { | ||
let rng = &mut thread_rng(); | ||
let transcript = self | ||
.0 | ||
|
@@ -575,10 +584,12 @@ generate_bytes_serialization!(AggregatedTranscript); | |
#[pymethods] | ||
impl AggregatedTranscript { | ||
#[new] | ||
pub fn new(messages: Vec<ValidatorMessage>) -> Self { | ||
pub fn new(messages: Vec<ValidatorMessage>) -> PyResult<Self> { | ||
let messages: Vec<_> = | ||
messages.into_iter().map(|vm| vm.to_inner()).collect(); | ||
Self(api::AggregatedTranscript::new(&messages)) | ||
let inner = api::AggregatedTranscript::new(&messages) | ||
.map_err(FerveoPythonError::FerveoError)?; | ||
Ok(Self(inner)) | ||
} | ||
|
||
pub fn verify( | ||
|
@@ -756,8 +767,12 @@ mod test_ferveo_python { | |
.iter() | ||
.enumerate() | ||
.map(|(i, keypair)| { | ||
Validator::new(format!("0x{i:040}"), &keypair.public_key()) | ||
.unwrap() | ||
Validator::new( | ||
format!("0x{i:040}"), | ||
&keypair.public_key(), | ||
i as u32, | ||
) | ||
.unwrap() | ||
}) | ||
.collect(); | ||
|
||
|
@@ -767,7 +782,7 @@ mod test_ferveo_python { | |
.iter() | ||
.cloned() | ||
.map(|sender| { | ||
let dkg = Dkg::new( | ||
let mut dkg = Dkg::new( | ||
tau, | ||
shares_num, | ||
security_threshold, | ||
|
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
What does this block code do?
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.
It's a pattern-matching expression with different cases on each line:
transcript
from aMessage
struct, and specifically fromMessage::Deal
variant. I'm considering deprecatingMessage
as we only use it internally.Message
variants. There are two currently,Message::Deal
andMessage:Aggregate
. We treat results other thanMessage::Deal
as errors, as we don't expect the DKG to be ready to aggregate at this point.