-
Notifications
You must be signed in to change notification settings - Fork 20.4k
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
whisper: messge bundling #15666
whisper: messge bundling #15666
Conversation
Need review from @gballet |
whisper/whisperv6/whisper.go
Outdated
cached, err := wh.add(env) | ||
if err != nil { | ||
log.Warn("bad envelope received, peer will be disconnected", "peer", p.peer.ID(), "err", err) | ||
return errors.New("invalid envelope") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, does that mean that if 1 of the envelopes has a bad format, then you will stop decoding all the envelopes? That sounds very harsh, and a potential threat to the network: all an attacker has to do to disrupt communications is to start sending invalid envelopes. They would get disconnected, sure, but if they hold a significant portion of the network they can still use this as a kill switch.
So instead, I would rather skip the bad envelope and call wh.add
on correct envelopes.
whisper/whisperv6/peer.go
Outdated
if cnt > 0 { | ||
log.Trace("broadcast", "num. messages", cnt) | ||
|
||
// transmit the unknown batch (potentially empty) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
transmit the (potentially empty) batch of unknown envelopes
@gballet thanks for your review, i have fixed accordingly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to have more than one error reported, if there are more than one errors.
whisper/whisperv6/whisper.go
Outdated
@@ -520,16 +520,23 @@ func (wh *Whisper) runMessageLoop(p *Peer, rw p2p.MsgReadWriter) error { | |||
log.Warn("failed to decode envelopes, peer will be disconnected", "peer", p.peer.ID(), "err", err) | |||
return errors.New("invalid envelopes") | |||
} | |||
|
|||
var trouble error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe have it to be trouble := []error{}
and store the n
first errors ? More than 1 error can happen in real life.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i am reluctant to complicate the processing logic. instead, i have changed the function so that it will report all the errors by logging. i have even upgraded the log level from warning to error, because this kind of error should be exceptional.
Changed the communication protocol for ordinary message, according to EIP 627. Messages will be send in bundles, i.e. array of messages will be sent instead of single message.
Changed the communication protocol for ordinary message, according to the EIP#627.
Messages will be send in bundles, i.e. array of messages will be sent instead of single message.