rework validator pipeline #176

vyzo · 2019-04-25T18:52:36Z

Prespawn goroutines for signature validation, avoiding new goroutines for each message.
Ensure user validators are invoked at most once for each message by marking them as seen as soon as the signature verification completes.
Support asynchronous validation that can take a long time to complete ~~and needs to be offloaded~~.
Support inline (synchronous) validation for simple validators.

vyzo · 2019-04-25T19:08:09Z

test failures are known, will fix in a separate pr.

Stebalien · 2019-04-25T20:56:34Z

pubsub.go

+		select {
+		case p.validateThrottle <- struct{}{}:
+			go func() {
+				p.doValidateTopic(vals, src, msg)


Doesn't this defeat the point of having a worker pool? Is there any reason to spawn this as a goroutine?

Yes, it kind of does. But at the moment all registered validators are asynchronous which means they can block, and we need to execute them in a goroutine.

Having said that, this is an artifact of WIP; I want to separate the validators to synchronous and asynchronous, and execute the synchronous ones inline.
The asynchronous can be in a goroutine or even better in an asynchronous api that doesn't use throttles at all.

Actually, since we are going to spawn a goroutine for async validators, we can still use the throttles.

Hm. But why not just have 1024 synchronous validators (well, dynamically adjusting)?

The asynchronous validators can block at will, I don't think we should have pinned goroutines for them. The cost of actually spawning the goroutine shouldn't be too high.

Also, if we had that many synchronous goroutines we could overtax the system doing synchronous validation.
That is we could end up validating signatures for 1024 messages concurrently which is not the intention (oversubscribes the cpu by a lot).
Perhaps we could have a second pool with prespawned (or dynamically adjustable) goroutines for the async validators but I don't think we gain much for the complexity.

I see. So we're assuming that some validators will be CPU bound and others will won't. Async validators are those non-CPU bound validators.

This is really where async/await shines.

Perhaps we could have a second pool with prespawned (or dynamically adjustable) goroutines for the async validators but I don't think we gain much for the complexity.

We can revisit this later.

We might want to allow multiple validators per topic: one could register an inline validator that fails fast with basic structural validation and an async validator that fetches stuff from the network for deep validation.

Ultimately I'd like to see something like a claim-check pattern here, but we can iterate towards that later. In summary:

The validator pipeline sends the message to the validator via a channel. The message includes the message ID, a deadline until which a result will be accepted, and a response channel.

The validator is free to manage its own goroutine pool, without the pipeline making assumptions whether the task is CPU or IO-bound.

The validator performs validation.

The validator returns the result to the pipeline via a channel, reporting the message ID and the result (OK/KO).

yutongp · 2019-04-26T00:39:03Z

pubsub.go

@@ -209,6 +217,11 @@ func NewPubSub(ctx context.Context, h host.Host, rt PubSubRouter, opts ...Option

 	go ps.processLoop(ctx)

+	numcpu := runtime.NumCPU()
+	for i := 0; i < numcpu; i++ {


can this numcpu be a configurable number?

We could make it configurable, but is there any benefit in using more than NumCPU synchronous validation workers?

If the validation function is not CPU-bound, yes. That’s the case for Eth2.0 folks.

These validators will be asynchronous and be executed in a goroutine.
This is the number of workers for the validation front-end; it handles signature verification, synchronous validators (TBD), and spawning goroutines for async validators with the throttle.

Added inline (synchronous) validation supprt.

If we give a number less than numcpu, would that help other components of the program utilize cpu resource better?

hrm maybe. We can add an option to make it less of a hog.

Added option to control the number of workers.

mhchia · 2019-04-26T06:22:59Z

pubsub.go

@@ -183,6 +190,7 @@ func NewPubSub(ctx context.Context, h host.Host, rt PubSubRouter, opts ...Option
 		topics:           make(map[string]map[peer.ID]struct{}),
 		peers:            make(map[peer.ID]chan *RPC),
 		topicVals:        make(map[string]*topicVal),
+		validateQ:        make(chan *validateReq, 32),


Should the size of validateQ the same as or greater than defaultValidateThrottle? Though it seems defaultValidateThrottle is the potential max number of the concurrent goroutines previously.

I don't think so, or we will have the same problems with overloading we have previously.

So to explain: the throttle controls the number of active asynchronous validation requests, which can take a while to complete.
The queue controls the front end to the validation pipeline, which can drop messages if we are overloaded - see #172.

Big thanks for the clear explanation. Are my following understandings correct?

validatorQ with the workers serve as the rate-limiting gate

Except for the removal of the timeout, the asynchronous verification part is roughly the same as the one before this PR(except for the fact that signature verification is done synchronously by the workers).

yes, this is correct.

vyzo · 2019-04-26T08:30:04Z

I've removed the WIP moniker, this is ready for review.

raulk

Had a quick look at this, shaping up well!

Now that validation is becoming more complex, could we please encapsulate it behind a ValidationPipeline type?

The logic is a bit scattered over the place now, and it's difficult to navigate how the different aspects interplay with one another, e.g. inline, timeout, throttles, concurrency, etc.

raulk · 2019-04-29T16:11:07Z

On a related note, do you feel strongly about async validator throttling being enforced by pubsub?

I think this may be an artificial need arising from us spawning a single goroutine per validator call -- once you do that you need a mechanism to prevent DoS, and hence you introduce a throttling mechanism.

If, however, we made async validators truly async by using channels, each validator would be in charge of accepting or rejecting jobs, and could use a different (more efficient) concurrency model instead of 1 goroutine per task.

vyzo · 2019-04-29T18:06:20Z

Now that validation is becoming more complex, could we please encapsulate it behind a ValidationPipeline type?

We can certainly do that. Or we can lump all validation related logic in its own file, which ~~would~~ may have similar effects in terms of readability.

On a related note, do you feel strongly about async validator throttling being enforced by pubsub?

I don't have particularly strong views, other than having a throttle somewhere.
The trade-off is in the complexity of writing validation routines, which has a really simple api now.

vyzo · 2019-04-29T19:46:51Z

I have split off the validation logic into its own component, per @raulk's request.

Note that the diff has become rather dirty because of this, and it's hard to see exactly what changed.
You might have to read each commit individually.

vyzo · 2019-04-30T08:09:33Z

Rebased on master for test fixes in #178.

…eline-update Upgrade to inflight pubsub This upgrade reworks the validator pipeline in pubsub. Testing shows that this is far more reliable than the commit we're currently pinned to. This has the unfortunate side-effect of pinning us to a commit that's currently under PR (in libp2p). A subsequent PR will need to upgrade this commit we're pinned to, to the latest go-libp2p-pubsub master. See libp2p/go-libp2p-pubsub#176

raulk

Sorry for the long turn-around! This looks really good, @vyzo. The separation of the validation logic is clean, and I'm excited to roll this out. Just a few small items, but nothing blocking from me. Good job.

validation.go

raulk · 2019-05-14T10:14:59Z

validation.go

+
+// validateWorker is an active goroutine performing inline validation
+func (v *validation) validateWorker() {
+	for {


Do we want to use a waitgroup here for an orderly shutdown of workers?

We could, but is it necessary?

raulk · 2019-05-14T10:15:47Z

validation.go

+	for {
+		select {
+		case req := <-v.validateQ:
+			v.validate(req.vals, req.src, req.msg)


~~I think validate should take a context. Otherwise we risk blocking a system shutdown if a lengthy validation is inflight.~~

Ah, nevermind. I see that validate does pass in the pubsub context to the validator itself.

validation.go

vyzo · 2019-05-14T10:59:43Z

I am going to rebase for merge.

…ronously

…lidation throttle. and some better documentation.

…eline-update Upgrade to inflight pubsub This upgrade reworks the validator pipeline in pubsub. Testing shows that this is far more reliable than the commit we're currently pinned to. This has the unfortunate side-effect of pinning us to a commit that's currently under PR (in libp2p). A subsequent PR will need to upgrade this commit we're pinned to, to the latest go-libp2p-pubsub master. See libp2p/go-libp2p-pubsub#176

vyzo requested review from Stebalien, raulk and whyrusleeping April 25, 2019 18:52

ghost assigned vyzo Apr 25, 2019

ghost added the in progress label Apr 25, 2019

This was referenced Apr 25, 2019

Validators are called multiple times for the same message #173

Closed

bandwidth limiting #172

Closed

Stebalien reviewed Apr 25, 2019

View reviewed changes

yutongp reviewed Apr 26, 2019

View reviewed changes

mhchia reviewed Apr 26, 2019

View reviewed changes

vyzo changed the title ~~[WIP] rework validator pipeline~~ rework validator pipeline Apr 26, 2019

vyzo mentioned this pull request Apr 26, 2019

Offloading messages for async validation #169

Open

raulk reviewed Apr 29, 2019

View reviewed changes

vyzo force-pushed the feat/rework-validators branch from 018dfbb to 9f0263a Compare April 30, 2019 08:09

pdyraga mentioned this pull request May 2, 2019

Upgrade to inflight pubsub keep-network/keep-core#772

Merged

raulk approved these changes May 14, 2019

View reviewed changes

vyzo added 6 commits May 14, 2019 13:59

validation pipeline front-end for handling signature validation synch…

7a38f76

…ronously

invoke user validators once for each message

b227afb

remove default async validation timeout and increase default topic va…

b84a32a

…lidation throttle. and some better documentation.

add support for inline (synchronous) validators

d8f08cd

add option to control number of synchronous validation workers

8d0c8d6

split off validation into its own type

cb423f4

improve godocs

f4d9eee

vyzo force-pushed the feat/rework-validators branch from 93590ff to f4d9eee Compare May 14, 2019 11:05

remove unnecessary context in validateSingleTopic

2df9d94

vyzo merged commit b962da5 into master May 15, 2019

ghost removed the in progress label May 15, 2019

vyzo deleted the feat/rework-validators branch May 15, 2019 08:22

vyzo mentioned this pull request May 15, 2019

Prevent propagation of invalid blocks with block pubsub topic validator filecoin-project/venus#2783

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rework validator pipeline #176

rework validator pipeline #176

vyzo commented Apr 25, 2019 •

edited

Loading

vyzo commented Apr 25, 2019

Stebalien Apr 25, 2019

vyzo Apr 26, 2019

vyzo Apr 26, 2019

Stebalien Apr 26, 2019

vyzo Apr 26, 2019

Stebalien Apr 26, 2019

vyzo Apr 26, 2019

raulk May 14, 2019

yutongp Apr 26, 2019

vyzo Apr 26, 2019

raulk Apr 26, 2019

vyzo Apr 26, 2019

vyzo Apr 26, 2019

yutongp Apr 26, 2019

vyzo Apr 26, 2019

vyzo Apr 26, 2019

mhchia Apr 26, 2019 •

edited

Loading

vyzo Apr 26, 2019

vyzo Apr 26, 2019

mhchia Apr 27, 2019 •

edited

Loading

vyzo Apr 27, 2019

vyzo commented Apr 26, 2019

raulk left a comment •

edited

Loading

raulk commented Apr 29, 2019

vyzo commented Apr 29, 2019 •

edited

Loading

vyzo commented Apr 29, 2019 •

edited

Loading

vyzo commented Apr 30, 2019

raulk left a comment

raulk May 14, 2019

vyzo May 14, 2019

raulk May 14, 2019

vyzo commented May 14, 2019

rework validator pipeline #176

rework validator pipeline #176

Conversation

vyzo commented Apr 25, 2019 • edited Loading

vyzo commented Apr 25, 2019

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

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

mhchia Apr 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhchia Apr 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyzo commented Apr 26, 2019

raulk left a comment • edited Loading

Choose a reason for hiding this comment

raulk commented Apr 29, 2019

vyzo commented Apr 29, 2019 • edited Loading

vyzo commented Apr 29, 2019 • edited Loading

vyzo commented Apr 30, 2019

raulk 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

vyzo commented May 14, 2019

vyzo commented Apr 25, 2019 •

edited

Loading

mhchia Apr 26, 2019 •

edited

Loading

mhchia Apr 27, 2019 •

edited

Loading

raulk left a comment •

edited

Loading

vyzo commented Apr 29, 2019 •

edited

Loading

vyzo commented Apr 29, 2019 •

edited

Loading