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

whisper: messge bundling #15666

Merged
merged 3 commits into from
Dec 21, 2017
Merged

whisper: messge bundling #15666

merged 3 commits into from
Dec 21, 2017

Conversation

gluk256
Copy link
Contributor

@gluk256 gluk256 commented Dec 13, 2017

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.

@fjl
Copy link
Contributor

fjl commented Dec 14, 2017

Need review from @gballet

@fjl fjl requested a review from gballet December 14, 2017 12:20
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")
Copy link
Member

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.

if cnt > 0 {
log.Trace("broadcast", "num. messages", cnt)

// transmit the unknown batch (potentially empty)
Copy link
Member

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

@gluk256
Copy link
Contributor Author

gluk256 commented Dec 20, 2017

@gballet thanks for your review, i have fixed accordingly

Copy link
Member

@gballet gballet left a 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.

@@ -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
Copy link
Member

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.

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 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.

@fjl fjl merged commit 9f1007e into ethereum:master Dec 21, 2017
@karalabe karalabe added this to the 1.8.0 milestone Dec 21, 2017
mariameda pushed a commit to NiluPlatform/go-nilu that referenced this pull request Aug 23, 2018
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.
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.

4 participants