-
Notifications
You must be signed in to change notification settings - Fork 3
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
[ceremonies/#65] generate random meetup indices #73
Conversation
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.
pretty sure this way of doing it won't be compatible with #68. @pifragile can you please comment?
Might be better to store the mapping meetupindex->locationindex separately or derive it directly from a seed as the new assignment does it too. May still be worth merging this for v0.7.0 and then adapting it as part of #68 for v0.8.0?
@NicJak @brenzi No this wont be compatible, because the way assignments are handled will be fundamentally changed in #68. And just out of curiosity, what is the rationale behind randomizing the meetup locations, when we already randomize the pariticipants? Is it about bootstrappers not always being assigned to the first couple locations? |
The set of locations should always be >> set of meetups. So we need to randomize that too |
Having predictable meetup locations could ease preventive actions towards encointer. |
on a public chain all assignments will be public knowledge beforehand anyway. Later, the ceremony pallet will be on a TEE sidechain and this will be solved |
I was just thinking, might there be some nice way to return the locations in a "randomized" order based on some seed input here: pallets/communities/src/lib.rs Line 479 in c2626da
|
not sure if a simple getter should involve random. bad for caching i.e. for geo view explorer |
true that makes sense. one could move the functionality to any place actually, was just worried about getting a mess with #68. maybe we could keep this really simple and for locations not do a full random permutation, but just a random offset, because unpredictable locations we already get from the assignment process. just thinking aloud :) |
no, needs random because we don't want predictability of where a meetup will take place to make sabotage more difficult. |
i dont understand, a random offset also makes the location unpredictable but takes O(1) and a random permutation takes O(N). i think permutation on locations AND participants is an overkill. what am i missing? |
O(n) of a simple index permutation should not worry us. The difference is: With random offset you only need two samples and you learn that ALL locations in between will hold a meetup. So a simple offset has inferior sabotage resistance than random permutation |
ok if O(N) does not hurt us if there many locations its better for sure! |
if O(1) is preferred, we could also use an expensive O(1) + C like a hash. One would have elaborate the impact of a collision and probably do a small performance comparison as to at what n the break-even-point is.. |
@pifragile you derive an entire assignment from two primes and a seed, right? why not derive the location permutation the same way? |
that would be possible, yes! |
how do we proceed with this change? |
we should derive a location permutation client side from the randomness we use for assignments. @NicJak If you see how you could do that from the existing code, please go ahead! Otherwise, @pifragile might be able to help |
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 know this has been merged, but I still wanted to give my two cents here.
In general, this looks good, but I believe, we should be wary about unnecessary collects
because these are comparatively expensive, and it's a shame if it's done unnecessarily.
This reverts commit 47282e2.
* Revert "[ceremonies/#65] generate random meetup indices (#73)" This reverts commit 47282e2. * new assignment, lazy issuance, random locations * [ceremonies/#75] use random coprime for assignment * [ceremonies] fix no_std compilation and `fn find_coprime_below` * [ceremonies] print error in `generate_meetup_assignment_params` * [ceremonies] use checked math operations in `checked_ceil_division`, `assignment_fn` * [ceremonies] extract most math functions to primitives * [ceremonies] move `mod_inv` to math module * [ceremonies] fix randomness in `generate_assignment_function_params` -> introduces failing tests * [ceremonies] remove unnecessary loc_ix >= 0 comparison for u64 type * [ceremonies] fix failing tests by fixing `pick_non_zero_u32` * [ceremonies] minor cleanup * [ceremonies] add tests for `get_greatest_common_denominator` * [primitives/random_number_generator] simplify pick_non_zero_u32 * [ceremonies/math] fix-no std compilation; successfully compiles with the node. * [ceremonies] remove unnecessary result in `generate_all_meetup_assignment_params` * [ceremonies] add test `generate_meetup_assignment_params_is_random` Co-authored-by: Nicolas Jakob <jakobnicolas@gmail.com> Co-authored-by: Christian Langenbacher <clangenb+gh@protonmail.ch>
the changes in tests can easily be verified by commenting out the performing the random permutation on the indices (ceremonies/src/lib.rs:487)