message validation #1289

synzhu · 2021-09-13T21:47:35Z

Implements a single topic validator that handles deserializing the message, extracting the sender ID, and then runs all application level MessageValidators on the message.
Deprecates the OriginID field on the Message struct. Instead, the network layer, upon receiving a message, can simply deduce the OriginID based on the sender's peer ID.

closes https://github.com/dapperlabs/flow-go/issues/5804

huitseeker

Thanks! I like that TopicValidator is called systematically. I think there's a small "bug" in calling the inner validators, see inline.

huitseeker · 2021-09-15T15:35:53Z

network/message/message.proto

-    string Type = 6;
+  string ChannelID = 1;
+  bytes EventID = 2;
+  bytes OriginID = 3 [deprecated = true];


If we just deprecate this, we're not removing the field and are still allowing its use.

Which means, technically, that we could still have OriginID claims in this field, which would still be ser/deser-ialized, and that we should still report if different from the result of the validation logic.

Is it possible to just remove this field and mark the release that would include this PR as breaking? I think it's worth it.

/cc @ramtinms @vishalchangrani for advice on the Message format change.

Yes, although I'm hoping we can eventually remove all of these fields except Payload, and I think this is something that can wait until later and we can remove them all in one fell swoop.

OK. Do we have an issue for that?

Not exactly, but it is part of https://github.com/dapperlabs/flow-go/issues/5847

As long as the network layer is passing the application layer the origin ID (which we are), it doesn't matter if we remove the field now or later.

network/validator/pubsub/validator.go

network/p2p/middleware.go

huitseeker · 2021-09-15T15:47:46Z

network/p2p/readSubscription.go

+		if !ok {
+			r.log.Fatal().Str("raw_msg", rawMsg.String()).Msg("validator data missing")


Not a fan of the log fatal here.

This would be triggered by an independent modification of TopicValidator that e.g. made it return before populating the data in some cases. We should definitely log a spectacular Error, but why not behave like the above failure cases and return?

We can, but personally I would consider it a bug if the validator returned ValidationAccept without populating the data. At the bare minimum, the validator should check that the message can be deserialized properly and that the origin id can be extracted. If it cannot do even this, then we should definitely reject the message.

My issue is neither with checking that the validation data is here, nor with considering this a bug. We should certainly transmit the message that "something is rotten in the state of Denmark" far and wide.

My issue is with crashing the node based on the input of one (externally controlled) message. A validator that accepts without populating the data has a bug, yes, but should its impact be to lose the entire blockchain?

network/validator/pubsub/validator.go

network/validator/pubsub/staked_validator.go

Co-authored-by: François Garillot <4142+huitseeker@users.noreply.github.com>

codecov-commenter · 2021-09-20T20:46:45Z

Codecov Report

Merging #1289 (abc4ae0) into master (a783037) will increase coverage by 0.02%.
The diff coverage is 45.45%.

@@            Coverage Diff             @@
##           master    #1289      +/-   ##
==========================================
+ Coverage   54.70%   54.73%   +0.02%     
==========================================
  Files         502      501       -1     
  Lines       31769    31743      -26     
==========================================
- Hits        17380    17373       -7     
+ Misses      12026    12011      -15     
+ Partials     2363     2359       -4

Flag	Coverage Δ
unittests	`54.73% <45.45%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
network/p2p/libp2pUtils.go	`59.74% <ø> (+5.48%)`	⬆️
network/p2p/middleware.go	`0.00% <0.00%> (ø)`
network/p2p/readSubscription.go	`0.00% <0.00%> (ø)`
network/p2p/libp2pNode.go	`67.08% <71.42%> (-0.01%)`	⬇️
engine/collection/synchronization/engine.go	`62.90% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a783037...abc4ae0. Read the comment docs.

huitseeker

This is swallowing the immediate return that should happen on absence of validation data. Otherwise looks good.

network/validator/pubsub/validator.go

network/p2p/readSubscription.go

Co-authored-by: François Garillot <4142+huitseeker@users.noreply.github.com>

huitseeker

Thanks a bunch for all your work on this, it looks great!

vishalchangrani · 2021-09-21T16:31:26Z

Since we only care about Accept or Reject, we will be better served with this Validator function instead of the Extended version.

synzhu · 2021-09-21T17:56:19Z

Since we only care about Accept or Reject, we will be better served with this Validator function instead of the Extended version.

We actually do care about Ignore as well. Currently there is no usecase which simply Ignore, but there could be in the future.

The topic validator should take the validation result from multiple MessageValidators and compute an "aggregate" result which is then returned to Libp2p.

vishalchangrani · 2021-09-21T18:11:01Z

Since we only care about Accept or Reject, we will be better served with this Validator function instead of the Extended version.

We actually do care about Ignore as well. Currently there is no usecase which simply Ignore, but there could be in the future.

The topic validator should take the validation result from multiple MessageValidators and compute an "aggregate" result which is then returned to Libp2p.

correct - and when we do have a use case in the future we can then switch over to the extended version. This seems like a premature generalization. But I went ahead and approved since that is not a big deal one way or the other.

synzhu · 2021-09-22T06:03:10Z

bors merge

bors · 2021-09-22T06:19:28Z

Build succeeded:

synzhu added 5 commits September 13, 2021 12:43

wip

d18d04f

update

25bf475

make topic validator required

21f8ff3

modify test

9b15c6b

Update topic_validator_test.go

4d2e538

synzhu requested review from huitseeker, vishalchangrani and yhassanzadeh13 September 13, 2021 23:32

synzhu changed the title ~~Smnzhu/validation consolidation~~ message validation Sep 13, 2021

synzhu marked this pull request as ready for review September 13, 2021 23:34

update

94273ca

huitseeker reviewed Sep 15, 2021

View reviewed changes

synzhu requested a review from huitseeker September 16, 2021 19:23

synzhu and others added 6 commits September 16, 2021 15:17

Update network/p2p/middleware.go

e2aee38

Co-authored-by: François Garillot <4142+huitseeker@users.noreply.github.com>

fix test failures

bc58d42

Update libp2pNode.go

113b00f

Comment messageValidator and remove fatal error.

a61080b

remove deprecated tests

5ab88eb

Merge branch 'master' into smnzhu/validation-consolidation

c60e78e

huitseeker suggested changes Sep 21, 2021

View reviewed changes

network/validator/pubsub/validator.go Show resolved Hide resolved

network/p2p/readSubscription.go Outdated Show resolved Hide resolved

Update network/validator/pubsub/validator.go

ed7f82d

Co-authored-by: François Garillot <4142+huitseeker@users.noreply.github.com>

synzhu requested a review from huitseeker September 21, 2021 03:32

add return after error log

abc4ae0

huitseeker approved these changes Sep 21, 2021

View reviewed changes

vishalchangrani approved these changes Sep 21, 2021

View reviewed changes

bors bot merged commit 2569fa4 into master Sep 22, 2021

bors bot deleted the smnzhu/validation-consolidation branch September 22, 2021 06:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

message validation #1289

message validation #1289

synzhu commented Sep 13, 2021 •

edited

huitseeker left a comment

huitseeker Sep 15, 2021

synzhu Sep 16, 2021

huitseeker Sep 18, 2021

synzhu Sep 20, 2021

vishalchangrani Sep 21, 2021

huitseeker Sep 15, 2021

synzhu Sep 17, 2021

huitseeker Sep 18, 2021

codecov-commenter commented Sep 20, 2021 •

edited

huitseeker left a comment

huitseeker left a comment

vishalchangrani commented Sep 21, 2021

synzhu commented Sep 21, 2021

vishalchangrani commented Sep 21, 2021

synzhu commented Sep 22, 2021

bors bot commented Sep 22, 2021

		if !ok {
		r.log.Fatal().Str("raw_msg", rawMsg.String()).Msg("validator data missing")

message validation #1289

message validation #1289

Conversation

synzhu commented Sep 13, 2021 • edited

huitseeker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 20, 2021 • edited

Codecov Report

huitseeker left a comment

Choose a reason for hiding this comment

huitseeker left a comment

Choose a reason for hiding this comment

vishalchangrani commented Sep 21, 2021

synzhu commented Sep 21, 2021

vishalchangrani commented Sep 21, 2021

synzhu commented Sep 22, 2021

bors bot commented Sep 22, 2021

synzhu commented Sep 13, 2021 •

edited

codecov-commenter commented Sep 20, 2021 •

edited