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.

Sign up for GitHub

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

Jump to bottom

Initial implementation of Poisson Sortition #2084

Merged
merged 16 commits into from
Jun 26, 2020
Merged

Initial implementation of Poisson Sortition #2084

merged 16 commits into from
Jun 26, 2020

Conversation

Kubuxu
Copy link
Contributor

@Kubuxu Kubuxu commented Jun 21, 2020 •
edited
Loading

TODO:

  • Fix debug_advance
  • Propagate wincount to actors
  • Fix TestDealMining

davidad
davidad approved these changes Jun 21, 2020
View reviewed changes
Copy link
Member

@davidad davidad left a comment

Choose a reason for hiding this comment

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

Try as I might, and despite several potential pitfalls, I cannot spot a single bug in this implementation. Couldn't have done it better myself 💯🙏☺️

chain/types/electionproof.go Outdated Show resolved Hide resolved
chain/types/electionproof_test.go Outdated Show resolved Hide resolved
chain/types/electionproof.go Outdated Show resolved Hide resolved
davidad
davidad reviewed Jun 22, 2020
View reviewed changes
Copy link
Member

@davidad davidad left a comment

Choose a reason for hiding this comment

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

refactored structure still looks correct to me 👍

chain/types/electionproof.go Outdated Show resolved Hide resolved
chain/types/electionproof.go Outdated Show resolved Hide resolved
chain/types/electionproof.go Outdated Show resolved Hide resolved
chain/types/electionproof.go Outdated Show resolved Hide resolved
Jakub Sztandera added 8 commits June 23, 2020 16:24
Initial implementation of Poisson Sortition
ee5639a 
Signed-off-by: Jakub Sztandera <[email protected]>
Improve performance of ComputeWinCount
f523b17 
As it turns out `big.Int#Mul` doesn't like it when you reuse input as
output.

```
name         old time/op    new time/op    delta
WinCounts-8    4.56µs ± 1%    3.90µs ± 2%  -14.44%  (p=0.000 n=9+10)

name         old alloc/op   new alloc/op   delta
WinCounts-8    3.50kB ± 0%    1.15kB ± 0%  -67.23%  (p=0.000 n=10+10)

name         old allocs/op  new allocs/op  delta
WinCounts-8      45.0 ± 0%      24.0 ± 0%  -46.67%  (p=0.000 n=10+10)
```

Signed-off-by: Jakub Sztandera <[email protected]>
Rename fxToD
84c0b50 
Signed-off-by: Jakub Sztandera <[email protected]>
Refactor poisson to struct for easier testing
8fea872 
Signed-off-by: Jakub Sztandera <[email protected]>
Add comments about algorithm
d92362f 
Signed-off-by: Jakub Sztandera <[email protected]>
Implement golden tests for Poisson distribution
d7f7108 
Signed-off-by: Jakub Sztandera <[email protected]>
Fix lint warnings
efeaa9b 
Signed-off-by: Jakub Sztandera <[email protected]>
Add tests for lambda
2b597fb 
Signed-off-by: Jakub Sztandera <[email protected]>
Jakub Sztandera added 2 commits June 23, 2020 16:33
Fix TestDealMining
2165728 
Signed-off-by: Jakub Sztandera <[email protected]>
Jakub Sztandera added 2 commits June 23, 2020 20:37
Fix debug_advance
1f967c6 
Signed-off-by: Jakub Sztandera <[email protected]>
Fix TestInteropBH
19ed695 
Signed-off-by: Jakub Sztandera <[email protected]>
@Kubuxu Kubuxu marked this pull request as ready for review June 23, 2020 18:43
Remove TestElectionExp
36d5880 
Signed-off-by: Jakub Sztandera <[email protected]>
magik6k
magik6k reviewed Jun 24, 2020
View reviewed changes
Copy link
Contributor

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

Couple of nitpicks. I didn't go very deep into the math, it feels like it does the correct thing, but I'm mostly putting my trust in @davidad saying it's correct

Probably want @whyrusleeping to have a look at this before merging

chain/store/weight.go Outdated Show resolved Hide resolved
chain/store/weight.go Outdated Show resolved Hide resolved
chain/store/weight.go Outdated Show resolved Hide resolved
chain/types/bigint_test.go Outdated Show resolved Hide resolved
chain/gen/gen.go Outdated Show resolved Hide resolved
Address @magik6k CR
88352c7 
Signed-off-by: Jakub Sztandera <[email protected]>
arajasek
arajasek approved these changes Jun 24, 2020
View reviewed changes
Copy link
Contributor

@arajasek arajasek left a comment

Choose a reason for hiding this comment

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

Math seems good, though I'm guessing the big Mults and Divs will be slow...probably not the critical path, though

@Kubuxu
Copy link
Contributor Author

Kubuxu commented Jun 24, 2020

The whole sortition for 100% miner (the most expensive) take 3.8μs.

@magik6k magik6k merged commit 2db8611 into next Jun 26, 2020
@magik6k magik6k deleted the feat/poiss-sortition branch June 26, 2020 10:26
@nicola nicola mentioned this pull request Jul 2, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidad davidad davidad approved these changes

@arajasek arajasek arajasek approved these changes

@magik6k magik6k magik6k approved these changes

@whyrusleeping whyrusleeping Awaiting requested review from whyrusleeping

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Kubuxu @davidad @magik6k @arajasek