implement new assignment algorithm and lazy issuance #68

pifragile · 2021-10-07T06:45:12Z

No description provided.

brenzi

this is a first stage review. Please fix the mentioned comments before I continue to review
overall, the code looks clean. nice work

ceremonies/src/lib.rs

ceremonies/src/tests.rs

ceremonies/src/lib.rs

brenzi · 2021-10-11T14:24:36Z

I'll test and merge this AFTER we release 0.7.0. Can you please bump the crate versions to 0.8.0 in this PR?

NicJak

Since you showed interest in the code(-quality) review, I reviewed your code with some remarks. The code is readable and is consistent in its type of writing. (nearly) all variables have meaningful and descriptive names.

ceremonies/src/lib.rs

NicJak · 2021-11-07T09:12:14Z

ceremonies/src/lib.rs

+                    &Self::attestation_index((cid, cindex), &other_participant),
+                );
+                if attestees_from_other.contains(&p) {
+                    was_attested_count += 1;
                }


nested loops have high potential of confusion.. the operation you're doing here is a good use case for the iter() api..
pseudo-code:
let was_attested_count = meetup_participants.iter()
.filter(o -> p!=o)
.map(o -> Self::attestation_index(...))
.map(o -> Self::attestation_registry(...))
.filter(o -> attestees_from_other(p))
.count();

ceremonies/src/lib.rs

pifragile · 2021-11-08T10:28:31Z

@NicJak thank you for your review! i added some remarks and questions. no comment means that i agree :) i will leave your comments unresolved for the moment being, as i will soon have to talk to @brenzi about my future role in the project and then we can take it from there!

brenzi

subject to approval by @clangenb

pifragile · 2021-12-06T08:00:45Z

ceremonies/src/lib.rs

@@ -523,28 +523,11 @@ impl<T: Config> Module<T> {
    }

    fn find_coprime_below(upper_bound: u64) -> u64 {


@NicJak i just stumbled upon this. if you always take n-1 the algorithm becomes predictable. s1 is supposed to be random and coprime, that's the whole point of the algorithm. but it is my bad, i was not explicit about that when i explained it to you, sorry!

This reverts commit 47282e2.

clangenb

Finally, I managed to look at the code. :) It looks very good, but I also have some comments. I reckon this is the essence of my comments.

I tested that it does indeed not compile with the no due to no_std issues
Some math stuff I want to see in a dedicated module in primitives/core
Some rust technicalities

General impression:

it is very technical code, more docs would be really helpful
I have not double-checked yet, but I think some functions are not unit-tested, and as it's quite complex sometimes. Therefore, I created [ceremonies] double check if we test well enough #89. Additions are welcome, but I don't think we need to do that in this PR. I would like to merge this soon.

clangenb · 2021-11-23T09:25:16Z

primitives/src/ceremonies.rs

+    pub m: u64,
+    pub s1: u64,
+    pub s2: u64,


Can you add some doc here? I don't understand what these fields mean.

ceremonies/src/tests.rs

ceremonies/src/lib.rs

clangenb · 2021-12-09T10:20:40Z

ceremonies/src/tests.rs


-		let meetups: Vec<Vec<AccountId>> = get_meetups(cid);
-		assert_eq!(meetups.len(), 2);
-		// whitepaper III-B Rule 3: no more than 1/3 participants without reputation


ceremonies/src/tests.rs

clangenb · 2021-12-09T10:29:51Z

ceremonies/src/tests.rs

-#[rstest(n_bootstrappers, n_reputables, n_endorsees, n_newbies, n_locations, exp_meetups,
-    case(8,0,0,4,3, vec![12]),
-    case(9,0,0,4,3, vec![7,6]),
-    case(3,7,3,3,3, vec![8,8]),
-    case(3,7,4,3,3, vec![9,8]),
-    case::do_not_assign_more_meetups_than_locations(3,7,50,0,3, vec![12,12,12]),
-    case::do_not_assign_more_meetups_than_there_are_experienced_participants(3,1,49,0,10, vec![12,12,12,12]),
-    case(12,48,12*AMOUNT_NEWBIE_TICKETS as usize,0,55, [12; 55].to_vec()),
-)]
-fn assigning_meetup_works(
-	n_bootstrappers: usize,
-	n_reputables: usize,
-	n_endorsees: usize,
-	n_newbies: usize,
-	n_locations: u32,
-	exp_meetups: Vec<usize>,
-) {


Do we not test that anymore?

clangenb · 2021-12-09T10:32:16Z

ceremonies/src/tests.rs

-		// TODO: whitepaper III-B Rule 1: minimize the number of participants that have met at previous ceremony
-		// TODO: whitepaper III-B Rule 2: maximize number of participants per meetup within 3<=N<=12


Track this in #88

…`assignment_fn`

ceremonies/src/lib.rs

…-> introduces failing tests

…the node.

clangenb

I did some cleanup:

fixed no_std,
fixed meetup assignment randomization
some rust best practices

Follow-ups:

[ceremonies] introduce checked math operations #90

Tested that it compiles successfully with the node.

…ment_params`

pifragile requested a review from brenzi October 7, 2021 08:01

brenzi requested changes Oct 10, 2021

View reviewed changes

pifragile mentioned this pull request Oct 18, 2021

[ceremonies/#65] generate random meetup indices #73

Merged

brenzi changed the title ~~implement new assignment algorithm~~ implement new assignment algorithm and lazy issuance Oct 23, 2021

NicJak reviewed Nov 7, 2021

View reviewed changes

NicJak mentioned this pull request Nov 7, 2021

Enhance new assignment algorithm #75

Closed

NicJak force-pushed the feat/assignment branch 3 times, most recently from fa8aee7 to 12c817c Compare November 17, 2021 15:02

NicJak force-pushed the feat/assignment branch from e748d72 to 635acd7 Compare November 20, 2021 09:02

brenzi mentioned this pull request Nov 21, 2021

reduce computational complexity of assign and issue in ceremony pallet #29

Closed

brenzi requested a review from clangenb November 22, 2021 12:10

brenzi approved these changes Nov 22, 2021

View reviewed changes

clangenb mentioned this pull request Nov 22, 2021

introduce rustfmt #82

Merged

pifragile commented Dec 6, 2021

View reviewed changes

Revert "[ceremonies/#65] generate random meetup indices (#73)"

d1cdbde

This reverts commit 47282e2.

pifragile force-pushed the feat/assignment branch from c9abcc0 to cbddd13 Compare December 6, 2021 08:48

new assignment, lazy issuance, random locations

33f3879

pifragile force-pushed the feat/assignment branch from cbddd13 to 33f3879 Compare December 6, 2021 08:51

[ceremonies/#75] use random coprime for assignment

ca10eb7

clangenb reviewed Dec 9, 2021

View reviewed changes

clangenb self-requested a review December 9, 2021 11:13

clangenb added 4 commits December 11, 2021 11:22

[ceremonies] fix no_std compilation and fn find_coprime_below

3e12790

[ceremonies] print error in generate_meetup_assignment_params

2906fdf

[ceremonies] use checked math operations in checked_ceil_division, …

540814f

…`assignment_fn`

[ceremonies] extract most math functions to primitives

9032521

clangenb reviewed Dec 11, 2021

View reviewed changes

ceremonies/src/lib.rs Outdated Show resolved Hide resolved

[ceremonies] move mod_inv to math module

0680395

clangenb added 2 commits December 11, 2021 14:33

[ceremonies] fix randomness in generate_assignment_function_params …

e8bb32e

…-> introduces failing tests

[ceremonies] remove unnecessary loc_ix >= 0 comparison for u64 type

4e93b0d

clangenb force-pushed the feat/assignment branch from 67de92b to 4e93b0d Compare December 11, 2021 13:34

clangenb added 4 commits December 11, 2021 14:48

[ceremonies] fix failing tests by fixing pick_non_zero_u32

7869552

[ceremonies] minor cleanup

f7258f8

[ceremonies] add tests for get_greatest_common_denominator

0283f1e

[primitives/random_number_generator] simplify pick_non_zero_u32

2e39798

clangenb mentioned this pull request Dec 11, 2021

[ceremonies] introduce checked math operations #90

Closed

[ceremonies/math] fix-no std compilation; successfully compiles with …

f3baabc

…the node.

clangenb approved these changes Dec 11, 2021

View reviewed changes

clangenb added 2 commits December 11, 2021 15:26

[ceremonies] remove unnecessary result in `generate_all_meetup_assign…

f10533f

…ment_params`

[ceremonies] add test generate_meetup_assignment_params_is_random

8bcea77

clangenb merged commit 14b5897 into master Dec 11, 2021

clangenb deleted the feat/assignment branch December 16, 2021 09:06

clangenb mentioned this pull request Dec 21, 2021

v0.7.0 encointer/encointer-node#141

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement new assignment algorithm and lazy issuance #68

implement new assignment algorithm and lazy issuance #68

pifragile commented Oct 7, 2021

brenzi left a comment •

edited

brenzi commented Oct 11, 2021 •

edited

NicJak left a comment

NicJak Nov 7, 2021

pifragile commented Nov 8, 2021

brenzi left a comment

pifragile Dec 6, 2021

clangenb left a comment

clangenb Nov 23, 2021

clangenb Dec 9, 2021

clangenb Dec 9, 2021

clangenb Dec 9, 2021

clangenb left a comment

		@@ -523,28 +523,11 @@ impl<T: Config> Module<T> {
		}

		fn find_coprime_below(upper_bound: u64) -> u64 {

		// TODO: whitepaper III-B Rule 1: minimize the number of participants that have met at previous ceremony
		// TODO: whitepaper III-B Rule 2: maximize number of participants per meetup within 3<=N<=12

implement new assignment algorithm and lazy issuance #68

implement new assignment algorithm and lazy issuance #68

Conversation

pifragile commented Oct 7, 2021

brenzi left a comment • edited

Choose a reason for hiding this comment

brenzi commented Oct 11, 2021 • edited

NicJak left a comment

Choose a reason for hiding this comment

NicJak Nov 7, 2021

Choose a reason for hiding this comment

pifragile commented Nov 8, 2021

brenzi left a comment

Choose a reason for hiding this comment

pifragile Dec 6, 2021

Choose a reason for hiding this comment

clangenb left a comment

Choose a reason for hiding this comment

clangenb Nov 23, 2021

Choose a reason for hiding this comment

clangenb Dec 9, 2021

Choose a reason for hiding this comment

clangenb Dec 9, 2021

Choose a reason for hiding this comment

clangenb Dec 9, 2021

Choose a reason for hiding this comment

clangenb left a comment

Choose a reason for hiding this comment

brenzi left a comment •

edited

brenzi commented Oct 11, 2021 •

edited