Add various protocol writing helpers from synedrion - #113
Conversation
68a7e8c to
b9d9a96
Compare
Pull Request Test Coverage Report for Build 17520327573Details
💛 - Coveralls |
6c5309f to
f868868
Compare
|
Marking as ready for review to start the conversation about naming, I expect we'll have a lot of back and forth on this. |
dvdplm
left a comment
dvdplm
left a comment
There was a problem hiding this comment.
First read, focusing on docs/comments and some general remarks. Will loop back and think about naming.
| /// Executes the sessions for the given entry points, | ||
| /// making one party (first in alphabetical order) the malicious one with the wrapper `M` and the given `behavior`. | ||
| #[allow(clippy::type_complexity)] | ||
| pub fn run_with_one_malicious_party<SP, M, B>( |
There was a problem hiding this comment.
I think this method is doing more work than it should. How about we change it to take a list of entry points and return a new list with the first malicious? The it's the caller's job to call run_sync with it (or run_async).
There was a problem hiding this comment.
Good option. Gotta think of a name though, taint_entry_points? :)
There was a problem hiding this comment.
Thinking out loud here. Maybe it would be good to make the application of the corrupt behaviour explicit. What if we had a dyn Behavior (or a dyn Misbehavior) that we could apply() to a mutable ref to an EntryPoint?
Something like so:
let misbehaving_id = ids
.first()
.ok_or_else(|| LocalError::new("Entry points list cannot be empty"))?;
// Make it very clear what kind of misbehavior we are using and on which EntryPoint
InvalidMessageOverride::apply(misbehaving_id, &mut entry_points);
let execution_result = run_sync(rng, entry_points)?;I think it'd require some re-jigging and perhaps some renaming here and there, but the idea is: less automation and use explicit calls using the "thing" that defines what the misbehavior is we are applying.
There was a problem hiding this comment.
Pushed a new commit splitting out the preparation stage and the checking to separate methods. What do you think?
| let modified_entry_points = entry_points | ||
| .into_iter() | ||
| .map(|(signer, entry_point)| { | ||
| let id = signer.verifying_key(); | ||
| let maybe_behavior = if &id == misbehaving_id { | ||
| Some(behavior.clone()) | ||
| } else { | ||
| None | ||
| }; | ||
| let entry_point = MisbehavingEntryPoint::<SP::Verifier, B, M>::new(entry_point, maybe_behavior); | ||
| (signer, entry_point) | ||
| }) | ||
| .collect(); |
There was a problem hiding this comment.
It's a bit annoying to have to use 12 lines of code to do this job. We know it's the first entry, can't we mutate in place somehow?
There was a problem hiding this comment.
We are mutating all of them, it's just the maybe_behavior argument that changes. They all have to be wrapped in the same MisbehavingEntryPoint type.
6f702a5 to
45e147e
Compare
adfada9 to
d99bc69
Compare
dvdplm
left a comment
dvdplm
left a comment
There was a problem hiding this comment.
I think this is good to go now, modulo the protocol error question, which can perhaps be tweaked separately?
51939fa to
6b4aac3
Compare
c4ccf6e to
4c4e62d
Compare
3 participants
dev::check_evidence_with_extension()andcheck_evidence_with_extensions()that are used to write misbehavior tests (to be renamed depending on what we decide regarding the Extend/Misbehave)utils::Withouttrait (implemented forBTreeSetandBTreeMap). Fixes Makewithoutavailable inmanul#108MapValuesandMapValuesRef(separate traits because the latter needs a bound on keys being cloneable)verify_that()GetOrLocalErrorandGetOrInvalidEvidencetraits to streamline map lookups in protocols and evidence verification codeThis takes care of #81 except the items 7 and 8