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

Handled pre-round equivocation bug #600

Merged
merged 4 commits into from
Mar 5, 2019
Merged

Conversation

gavraz
Copy link
Contributor

@gavraz gavraz commented Mar 4, 2019

No description provided.

@@ -29,20 +29,24 @@ func (pre *PreRoundTracker) OnPreRound(msg *pb.HareMessage) {
return
}

// only handle first pre-round msg
if _, exist := pre.preRound[verifier.String()]; exist {
var sToTrack *Set = nil
Copy link
Contributor

Choose a reason for hiding this comment

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

you can refactor the else using better arrangement of parameters, sToTrack can start with msg.Message.Values as initial value and be modified else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is a matter of style but do understand it is a more simple and go-idiomatic approach so I changed it to fit the no else and use if for non-happy flows

preRound map[string]struct{} // maps PubKey->Pre-Round msg (unique)
tracker *RefCountTracker // keeps track of seen values
threshold uint32 // the threshold to prove a single value
preRound map[string]*Set // maps PubKey->Pre-Round msg (unique)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the comment - what is unique? and should it be Set instead of "Pre-Round msg"?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also (general comment) - please add tests to test these changes. It is suspicious that you a major logical change in the PreroundTracker without needing to change any of the test code, please make sure that this struct is well tested.

@gavraz gavraz force-pushed the hare_preround_equivocation branch from da60af5 to 30a91b2 Compare March 5, 2019 16:10
@gavraz gavraz merged commit 16c8b20 into develop Mar 5, 2019
beckmani pushed a commit that referenced this pull request Mar 7, 2019
* Handled pre-round equivocation bug
* Build certificate only from first f+1 commits
@y0sher y0sher deleted the hare_preround_equivocation branch September 10, 2019 09:30
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