Use libp2p gossipsub upstream validator framework #4318

prestonvanloon · 2019-12-18T17:34:47Z

Resolves #4317

Thanks to @protolambda for pointing this out.

This is now a refactoring to use the gossipsub validator framework.

…ages

prestonvanloon · 2019-12-18T18:06:46Z

This solution is not going to work as is. Adding a pubsub validator to reject all re-propagation will block all messages from being received.

The problem with using the pubsub validator framework for validation is that when you need to inspect the message contents, you would need to deserialize the message more than once for any given valid message. With the assumption that the majority of the messages will be valid, this could be an expensive task.

As is, we all repropagate messages once, thanks to content addressable message IDs. The problem is that we want to stop propagation of messages that are invalid.

My thinking now is that we consider these two approaches:

Add some functionality in gossipsub to disable automatic propagation of any topic message
Use the existing validator framework and deserialize messages twice. Alternatively, come up with some caching mechanism to try to avoid deserializing the messages twice, but this idea smells.

I've reached out to @raulk on telegram to see if there is any expert feedback from their team.

protolambda · 2019-12-18T18:15:33Z

There's another option: introduce a preprocessing step, to then continue with the preprocessed message in validation and the subscription.
I see three options for continuation there:

overload the message interface that gets passed around, and decode it into the right typed data from preprocessing for validation/processing.
add a metadata getter to the message. To attach things to during processing, to use at a later stage.
refactor more, make the pipeline more compositional, and somehow progressively process the message at each stage, instead of repeating things like decoding

prestonvanloon · 2019-12-18T18:20:39Z

overload the message interface that gets passed around, and decode it into the right typed data from preprocessing for validation/processing.

refactor more, make the pipeline more compositional, and somehow progressively process the message at each stage, instead of repeating things like decoding

These options would certainly solve our problem.

I imagine this pipeline:

Incoming pubsub.Message
Check seen message ID, peer signature verification, other pubsub metadata checks
Preprocess -> decode message
Validate -> check message contents
Propagate
Subscriber receives (decoded) message -> handle message at application layer

raulk · 2019-12-18T19:08:44Z

Yes, I agree that we could add a baggage map where validators could store deserialised payloads or other computed/derived information, for downstream validators to reuse. We could also propagate this baggage to the final consumer.

whyrusleeping · 2019-12-18T21:24:56Z

Oh hey! we already have a place for the validators to put things for the consumer, you can set Message.ValidatorData in the validator, and it is accessible by the receiver.

ref: libp2p/go-libp2p-pubsub#231

raulk · 2019-12-18T21:26:46Z

Yep, it turns out this is already implemented and I had missed it :-)

prestonvanloon · 2019-12-18T21:33:01Z

Awesome. Thanks @whyrusleeping @raulk @protolambda. We'll refactor our validation pipeline and use that field to pass around the message data. Thanks!!!

…alidator

…o pubsub-validator

prestonvanloon · 2019-12-19T22:22:55Z

OK this is ready for review again. I was experiencing some weird behavior that appears to run a validator on broadcast as well as the subscriber. libp2p/go-libp2p-pubsub#250

If anyone knows how to disable the validator on publish, I'd appreciate that info.

…o pubsub-validator

protolambda

A few nitpicks, I'm following this PR, good work so far 👍

protolambda · 2019-12-19T23:16:20Z

beacon-chain/sync/subscriber.go

-func noopValidator(_ context.Context, _ proto.Message, _ p2p.Broadcaster, _ bool) (bool, error) {
-	return true, nil
+// noopValidator is a no-op that only decodes the message, but does not check its contents.
+func (r *Service) noopValidator(ctx context.Context, _ peer.ID, msg *pubsub.Message) bool {


Maybe call it preProcessValidator to make clear what it really is?

Also, I think pubsub has a way to chain validators in sync, so you do not have to repeatedly call decodePubsubMessage, and put this as the preprocess validator instead.

This is actually only used in test. From what I recall, all validators were run asynchronously. Chaining would be nice with your suggestion though.

From godocs:

RegisterTopicValidator registers a validator for topic. By default validators are asynchronous, which means they will run in a separate goroutine. The number of active goroutines is controlled by global and per topic validator throttles; if it exceeds the throttle threshold, messages will be dropped.

I'll leave this for now unless we can chain these.

protolambda · 2019-12-19T23:23:35Z

beacon-chain/sync/validate_beacon_attestation.go

+		return false
+	}
+	topic := msg.TopicIDs[0]
+	topic = strings.TrimSuffix(topic, r.p2p.Encoding().ProtocolSuffix())


Maybe the msg checks and topic extraction can be refactored into a helper function?

Nice refactor, ninja'd this problem before I submitted the review. Great

Thanks! Terence beat you to it lol

terencechain

Looks good to me otherwise

beacon-chain/sync/validate_beacon_attestation.go

Co-Authored-By: terence tsao <terence@prysmaticlabs.com>

nisdas

For all our failed message validations, is there anyway to capture this in our metrics ?

nisdas · 2019-12-20T01:01:23Z

beacon-chain/sync/validate_attester_slashing.go

-	}
-	seenAttesterSlashings.Set(cacheKey, true /*value*/, 1 /*cost*/)
-
-	if fromSelf {


does libp2p not process messages from self ?

That's right, it won't send messages from itself to the subscription. It does, however, run validation on all messages published or subscribed. I've added a bit of a hack to immediately return true in validation if the pubsub message comes from ourselves. This bypasses the validation effectively as suggested here libp2p/go-libp2p-pubsub#250 (comment)

…ailure counter

…o pubsub-validator

@nisdas

* add reject all pubsub validator to stop automatic propagation of messages * gaz * Merge branch 'master' of github.com:prysmaticlabs/prysm into pubsub-validator * refactor p2p validator pipeline * add sanity check * Merge branch 'pubsub-validator' of github.com:prysmaticlabs/prysm into pubsub-validator * fixed up test * rem * gaz * Merge refs/heads/master into pubsub-validator * fix from self test * ensure validator data is set * resolve todo * Merge refs/heads/master into pubsub-validator * gaz * Merge refs/heads/master into pubsub-validator * Merge branch 'pubsub-validator' of github.com:prysmaticlabs/prysm into pubsub-validator * Merge refs/heads/master into pubsub-validator * remove all of the 'from self' logic. filed libp2p/go-libp2p-pubsub#250 * Merge branch 'pubsub-validator' of github.com:prysmaticlabs/prysm into pubsub-validator * gaz * update comment * Merge refs/heads/master into pubsub-validator * rename "VaidatorData" * Merge branch 'pubsub-validator' of github.com:prysmaticlabs/prysm into pubsub-validator * refactor * one more bit of refactoring * Update beacon-chain/sync/validate_beacon_attestation.go Co-Authored-By: terence tsao <terence@prysmaticlabs.com> * skip validation on self messages, add @nisdas feedback to increment failure counter * Merge branch 'pubsub-validator' of github.com:prysmaticlabs/prysm into pubsub-validator * remove flakey

@nisdas

* add reject all pubsub validator to stop automatic propagation of messages * gaz * Merge branch 'master' of github.com:prysmaticlabs/prysm into pubsub-validator * refactor p2p validator pipeline * add sanity check * Merge branch 'pubsub-validator' of github.com:prysmaticlabs/prysm into pubsub-validator * fixed up test * rem * gaz * Merge refs/heads/master into pubsub-validator * fix from self test * ensure validator data is set * resolve todo * Merge refs/heads/master into pubsub-validator * gaz * Merge refs/heads/master into pubsub-validator * Merge branch 'pubsub-validator' of github.com:prysmaticlabs/prysm into pubsub-validator * Merge refs/heads/master into pubsub-validator * remove all of the 'from self' logic. filed libp2p/go-libp2p-pubsub#250 * Merge branch 'pubsub-validator' of github.com:prysmaticlabs/prysm into pubsub-validator * gaz * update comment * Merge refs/heads/master into pubsub-validator * rename "VaidatorData" * Merge branch 'pubsub-validator' of github.com:prysmaticlabs/prysm into pubsub-validator * refactor * one more bit of refactoring * Update beacon-chain/sync/validate_beacon_attestation.go Co-Authored-By: terence tsao <terence@prysmaticlabs.com> * skip validation on self messages, add @nisdas feedback to increment failure counter * Merge branch 'pubsub-validator' of github.com:prysmaticlabs/prysm into pubsub-validator * remove flakey

add reject all pubsub validator to stop automatic propagation of mess…

7952358

…ages

prestonvanloon requested review from rauljordan, shayzluf, 0xKiwi, terencechain and nisdas December 18, 2019 17:34

nisdas previously approved these changes Dec 18, 2019

View reviewed changes

gaz

aeabc3a

prestonvanloon dismissed nisdas’s stale review via aeabc3a December 18, 2019 17:35

nisdas previously approved these changes Dec 18, 2019

View reviewed changes

prestonvanloon added OK to merge Blocked Blocked by research or external factors and removed OK to merge labels Dec 18, 2019

prestonvanloon added 2 commits December 18, 2019 16:46

Merge branch 'master' of github.com:prysmaticlabs/prysm into pubsub-v…

12484b9

…alidator

refactor p2p validator pipeline

2971c22

prestonvanloon dismissed nisdas’s stale review via 2971c22 December 19, 2019 03:18

prestonvanloon and others added 5 commits December 18, 2019 19:30

add sanity check

306ee88

Merge branch 'pubsub-validator' of github.com:prysmaticlabs/prysm int…

2412323

…o pubsub-validator

fixed up test

8813297

rem

3cacb53

gaz

12f4e9e

prestonvanloon added the In Progress label Dec 19, 2019

Merge refs/heads/master into pubsub-validator

e3e3a57

prestonvanloon added 3 commits December 19, 2019 14:19

remove all of the 'from self' logic. filed libp2p/go-libp2p-pubsub#250

41de504

Merge branch 'pubsub-validator' of github.com:prysmaticlabs/prysm int…

b6a726d

…o pubsub-validator

gaz

324e304

prestonvanloon added Ready For Review A pull request ready for code review and removed Blocked Blocked by research or external factors labels Dec 19, 2019

prestonvanloon changed the title ~~Add reject all pubsub validator to stop automatic propagation~~ Use libp2p gossipsub upstream validator framework Dec 19, 2019

prestonvanloon removed the In Progress label Dec 19, 2019

prestonvanloon requested a review from nisdas December 19, 2019 22:47

prestonvanloon and others added 4 commits December 19, 2019 15:07

update comment

188fc0e

Merge refs/heads/master into pubsub-validator

490043d

rename "VaidatorData"

587f138

Merge branch 'pubsub-validator' of github.com:prysmaticlabs/prysm int…

a01bd3f

…o pubsub-validator

rauljordan previously approved these changes Dec 19, 2019

View reviewed changes

refactor

86917e8

prestonvanloon dismissed rauljordan’s stale review via 86917e8 December 20, 2019 00:05

protolambda reviewed Dec 20, 2019

View reviewed changes

one more bit of refactoring

6183e31

terencechain reviewed Dec 20, 2019

View reviewed changes

beacon-chain/sync/validate_beacon_attestation.go Outdated Show resolved Hide resolved

Update beacon-chain/sync/validate_beacon_attestation.go

9cc77fb

Co-Authored-By: terence tsao <terence@prysmaticlabs.com>

nisdas reviewed Dec 20, 2019

View reviewed changes

prestonvanloon added 3 commits December 19, 2019 18:46

skip validation on self messages, add @nisdas feedback to increment f…

32b3150

…ailure counter

Merge branch 'pubsub-validator' of github.com:prysmaticlabs/prysm int…

1c12efe

…o pubsub-validator

remove flakey

612f9e6

nisdas approved these changes Dec 20, 2019

View reviewed changes

prestonvanloon added the OK to merge label Dec 20, 2019

prylabs-bulldozer bot merged commit 961dd21 into master Dec 20, 2019

delete-merged-branch bot deleted the pubsub-validator branch December 20, 2019 03:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use libp2p gossipsub upstream validator framework #4318

Use libp2p gossipsub upstream validator framework #4318

prestonvanloon commented Dec 18, 2019 •

edited

Loading

prestonvanloon commented Dec 18, 2019

protolambda commented Dec 18, 2019

prestonvanloon commented Dec 18, 2019 •

edited

Loading

raulk commented Dec 18, 2019 •

edited

Loading

whyrusleeping commented Dec 18, 2019

raulk commented Dec 18, 2019

prestonvanloon commented Dec 18, 2019

prestonvanloon commented Dec 19, 2019

protolambda left a comment

protolambda Dec 19, 2019

protolambda Dec 20, 2019

prestonvanloon Dec 20, 2019

protolambda Dec 19, 2019

protolambda Dec 20, 2019

prestonvanloon Dec 20, 2019

terencechain left a comment

nisdas left a comment

nisdas Dec 20, 2019

prestonvanloon Dec 20, 2019

Use libp2p gossipsub upstream validator framework #4318

Use libp2p gossipsub upstream validator framework #4318

Conversation

prestonvanloon commented Dec 18, 2019 • edited Loading

prestonvanloon commented Dec 18, 2019

protolambda commented Dec 18, 2019

prestonvanloon commented Dec 18, 2019 • edited Loading

raulk commented Dec 18, 2019 • edited Loading

whyrusleeping commented Dec 18, 2019

raulk commented Dec 18, 2019

prestonvanloon commented Dec 18, 2019

prestonvanloon commented Dec 19, 2019

protolambda 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

terencechain left a comment

Choose a reason for hiding this comment

nisdas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prestonvanloon commented Dec 18, 2019 •

edited

Loading

prestonvanloon commented Dec 18, 2019 •

edited

Loading

raulk commented Dec 18, 2019 •

edited

Loading