Skip to content
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

early rewards #233

Merged
merged 5 commits into from Jun 22, 2022
Merged

early rewards #233

merged 5 commits into from Jun 22, 2022

Conversation

pifragile
Copy link
Contributor

@pifragile pifragile requested a review from clangenb June 17, 2022 09:26
Copy link
Member

@clangenb clangenb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, this looks very good! I only have two details!

ceremonies/meetup-validation/src/lib.rs Show resolved Hide resolved
ceremonies/meetup-validation/src/lib.rs Outdated Show resolved Hide resolved
ceremonies/meetup-validation/src/tests.rs Show resolved Hide resolved
ceremonies/meetup-validation/src/lib.rs Outdated Show resolved Hide resolved
@pifragile pifragile requested a review from clangenb June 22, 2022 07:12
Copy link
Member

@clangenb clangenb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@clangenb clangenb requested a review from brenzi June 22, 2022 08:25
Copy link
Member

@brenzi brenzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll need to look at the tests in detail.

legit_participants: Participants,
participant_attestations: Attestations,
) -> bool {
for (i, mut attestations) in participant_attestations.into_iter().enumerate() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens, if i attested people that fell out of the legit participant set? I believe your logic with sorting and testing equality would not see this as fully connected. but it should

Anyway, sorting twice within a for loop doesn't strike me as efficient. Wouldn't a nested for loop through legit participants be more efficient?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test is made before any participants can fall out of the set, because the attestation graph needs to be fully connected for all participants in order for early payout to be possible, right?

about the efficiency. my implementation:
O(n) (loop over attestations) * O(n^2) (sorting, worstcase)

nested loops:
O(n) (loop over attestations) * O(n) (loop over each elem in attestations) * O(n) (check if legit_participant contains elem)

so in asymptotic complexity they are the same.
i agree that in the first implementation we have some overhead because it is 2*O(n^2), but i would argue that for those small input sizes it shouldnt really matter.

but you decide, should i change it to the nested loop?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nah, its bounded, so I guess it's fine. Do we already have a worst case benchmark for this claim function? that will tell us the price

ceremonies/src/lib.rs Outdated Show resolved Hide resolved
@brenzi
Copy link
Member

brenzi commented Jun 22, 2022

I need more time to review the tests, do not merge yet

Copy link
Member

@brenzi brenzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, I prefer to review the big picture once all PR's are done, so I trust @clangenb 's review for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants