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

Hare rolacle #458

Merged
merged 5 commits into from Jan 17, 2019

Conversation

Projects
None yet
2 participants
@gavraz
Copy link
Contributor

gavraz commented Jan 16, 2019

  • Added rolacle mock for testing
  • Added unit tests
  • Rebased to include last changes pushed to develop
func NewMockHashOracle(expectedSize int, comitySize int) *MockHashOracle {
mock := new(MockHashOracle)
mock.clients = make(map[string]struct{}, expectedSize)
mock.comitySize = comitySize

This comment has been minimized.

@zalmen

zalmen Jan 17, 2019

Member

typo: committee

This comment has been minimized.

@gavraz

gavraz Jan 17, 2019

Author Contributor

Fixed

}

func (mock *MockHashOracle) calcThreshold(comitySize int) uint32 {
if len(mock.clients) == 0 {

This comment has been minimized.

@zalmen

zalmen Jan 17, 2019

Member

you should also lock when checking for the length. Consider using RWMutex instead of Mutex

This comment has been minimized.

@zalmen

zalmen Jan 17, 2019

Member

It is good to have this check to detect bug, but you also need to check that committeeSize <= len(mock.clients)

This comment has been minimized.

@gavraz

gavraz Jan 17, 2019

Author Contributor

Fixed.


func (mock *MockHashOracle) calcThreshold(comitySize int) uint32 {
if len(mock.clients) == 0 {
panic("Called calcThreshold with 0 clients registered")

This comment has been minimized.

@zalmen

zalmen Jan 17, 2019

Member

No reason to panic, print a fat error message and return 0

This comment has been minimized.

@gavraz

gavraz Jan 17, 2019

Author Contributor

Changed

data := binary.LittleEndian.Uint32(proof)
// calculate thresholds
threshLeader := mock.calcThreshold(3) // expect 3 leaders
threshActive := mock.calcThreshold(mock.comitySize) // expect comitySize-3 actives

This comment has been minimized.

@zalmen

zalmen Jan 17, 2019

Member

don't understand the comment, why -3?


if data < 10 {
// calculate hash of proof
hash := fnv.New32()

This comment has been minimized.

@zalmen

zalmen Jan 17, 2019

Member

you should have a struct responsible for hashing, this struct should also return the max value (instead of the maxUint32 above). This way you'll make sure that no one changes the hash type without forgetting to update the max value

This comment has been minimized.

@gavraz

gavraz Jan 17, 2019

Author Contributor

Fixed

@gavraz gavraz force-pushed the hare_rolacle branch from 95af5bb to fa922ca Jan 17, 2019

@zalmen

zalmen approved these changes Jan 17, 2019

gavraz added some commits Jan 17, 2019

@gavraz gavraz merged commit 6f699fc into develop Jan 17, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

y0sher added a commit that referenced this pull request Jan 21, 2019

Hare rolacle (#458)
* Added mock oracle for testing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment