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

Gossip protocol validation #496

Merged
merged 8 commits into from Feb 2, 2019
Merged

Gossip protocol validation #496

merged 8 commits into from Feb 2, 2019

Conversation

@zalmen
Copy link
Contributor

@zalmen zalmen commented Jan 30, 2019

No description provided.

@zalmen zalmen requested review from y0sher, gavraz and almogdepaz Jan 30, 2019
log/log.go Outdated
@@ -19,7 +19,7 @@ type Log struct {

// smlogger is the local app singleton logger.
var AppLog Log
var debugMode = false
var debugMode = true

This comment has been minimized.

@y0sher

y0sher Jan 30, 2019
Contributor

change back to false

This comment has been minimized.

@zalmen

zalmen Jan 31, 2019
Author Contributor

fixed

prot.oldMessageMu.RLock()
res, ok := prot.invalidMessageQ[h]
prot.oldMessageMu.RUnlock()
if ok {

This comment has been minimized.

@y0sher

y0sher Jan 30, 2019
Contributor

imo this looks better :

Suggested change
if ok {
if !ok {
return Unknown
}
if !res {
return Invalid
}
return Valid

This comment has been minimized.

@zalmen

zalmen Jan 31, 2019
Author Contributor

I agree, done

return true
}


func (validator *MessageValidator) ValidateMessage(m *pb.HareMessage, k uint32) bool {

This comment has been minimized.

@y0sher

y0sher Jan 30, 2019
Contributor

used only in tests. maybe not needed anymore ?

This comment has been minimized.

@zalmen

zalmen Jan 31, 2019
Author Contributor

True - adjusted the test and removed the method

Unknown
)

func (prot *Protocol) isMessageValid(h hash) Validity {

This comment has been minimized.

@y0sher

y0sher Jan 30, 2019
Contributor

IsMessageValid feels like it returns a bool, consider better naming

This comment has been minimized.

@zalmen

zalmen Jan 31, 2019
Author Contributor

Don't agree

close(c)
}
delete(sn.sim.protocolHandler, sn.Node.PublicKey().String())
delete(sn.sim.protocolDirectHandler, sn.Node.PublicKey().String())

This comment has been minimized.

@y0sher

y0sher Jan 30, 2019
Contributor

should be doing the same for protocolGossipHandler now..

This comment has been minimized.

@zalmen

zalmen Jan 31, 2019
Author Contributor

done


func (pm gossipProtocolMessage) ReportValidation(protocol string, isValid bool) {
if pm.validationChan != nil {
pm.validationChan <- *service.NewMessageValidation(pm.Bytes(), protocol, isValid)

This comment has been minimized.

@y0sher

y0sher Jan 30, 2019
Contributor

why de reference?

This comment has been minimized.

@zalmen

zalmen Jan 31, 2019
Author Contributor

fixed (see comment below)

}

func NewMessageValidation(msg []byte, prot string, isValid bool) *MessageValidation {
return &MessageValidation{msg, prot, isValid}

This comment has been minimized.

@y0sher

y0sher Jan 30, 2019
Contributor

you dereference that everywhere, why don't just return it by value ?

This comment has been minimized.

@zalmen

zalmen Jan 31, 2019
Author Contributor

true, changed it to return a value


// RegisterGossipProtocol registers an handler for gossip based `protocol`
func (s *swarm) RegisterGossipProtocol(protocol string) chan service.GossipMessage {
mchan := make(chan service.GossipMessage, config.ConfigValues.BufferSize)

This comment has been minimized.

@y0sher

y0sher Jan 30, 2019
Contributor

I know it was there already but it's better to take BufferSize from s.config.BufferSize. (where its loaded from config/flags)

This comment has been minimized.

@zalmen

zalmen Jan 31, 2019
Author Contributor

fixed

hare/algorithm.go Show resolved Hide resolved
hare/algorithm.go Show resolved Hide resolved
hare/algorithm.go Show resolved Hide resolved
// TODO: should return error from message validation to indicate what failed, should retry only for contextual failure
log.Warning("Message is not valid for either round")
log.Warning("DirectMessage is not valid for either round")

This comment has been minimized.

@gavraz

gavraz Jan 30, 2019
Contributor

DirectMessage? I believe it happened during refactoring.

This comment has been minimized.

@zalmen

zalmen Feb 1, 2019
Author Contributor

I believe you believe correctly

hare/algorithm.go Show resolved Hide resolved
hare/algorithm.go Show resolved Hide resolved
}

func (msg Message) reportValidationResult(isValid bool) {
msg.validationChan <- *service.NewMessageValidation(msg.bytes, ProtoName, isValid)

This comment has been minimized.

@gavraz

gavraz Jan 30, 2019
Contributor

Pass a pointer to the channel and don't deref.
To my understanding, it has no perf implications in this context. We can further discuss it if you like.

This comment has been minimized.

@gavraz

gavraz Jan 31, 2019
Contributor

msg.validationChan can be nil. nil will block.

This comment has been minimized.

@zalmen

zalmen Feb 1, 2019
Author Contributor

fixed

return true
}


func (validator *MessageValidator) ValidateMessage(m *pb.HareMessage, k uint32) bool {

This comment has been minimized.

@gavraz

gavraz Jan 30, 2019
Contributor

Remove. ValidateMessage is only used in the unit tests.

This comment has been minimized.

@zalmen

zalmen Feb 1, 2019
Author Contributor

removed

hare/messagevalidation.go Show resolved Hide resolved
@zalmen
Copy link
Contributor Author

@zalmen zalmen commented Feb 1, 2019

@y0sher @gavraz done addressing your comments

Copy link
Contributor

@gavraz gavraz left a comment

I reviewed the hare code and approve it.
Ask the last reviewer to also approve the PR.

@gavraz
gavraz approved these changes Feb 2, 2019
@zalmen zalmen merged commit 6dc06a8 into develop Feb 2, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zalmen zalmen deleted the gossip-protocol-validation branch Feb 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants