VA: Fix performRemoteValidation goroutine leak

[beautifulentropy]

PerformValidation goroutines write to a buffered results channel to prevent blocking and terminate early when an exit condition is met.

[pavelnikolov]

No unit tests need to be changed?

[aarongable]

Suppose that the parent gRPC context is cancelled.

Today, the resulting sequence of events is:

1. the child rva.PerformValidation gRPC detect the cancellation and quit, writing their cancellation errors to the results channel
2. the for result := range results loop reads errors from the results channel until the errors surpass the threshold
3. it increments the remoteValidationFailures metric, modifies the first error to indicate it occurred during secondary validation, and returns
4. the remaining goroutines hang trying to write to the results channel

With this change, the resulting sequence of events becomes racy and non-deterministic, depending on what order the go runtime schedules selects. It might behave like the above (with the exception of leaking the child goroutines), or:

1. the for/select loop detects the cancellation and immediately quits, returning a different kind of error describing the cancellation and not incrementing the metric

I don't love this non-determinism.

I think a cleaner solution might be simply to make the results channel a buffered channel with capacity len(va.remoteVAs). We know that number is always going to be small -- likely never more than 6, even with the new BRs MPIC requirements -- and the items in it are small because they're all pointers.

This way all the rest of the logic can remain untouched: the child goroutines are guaranteed to be able to write to the results channel and exit, and the loop reading from the results channel can continue to early-return as soon as it gets enough results without having to worry about checking for cancellation itself.

That said, I think creating a new context for the child gRPCs and cancelling it when this function returns is still a good idea, as it will cause them to bail out faster.

[jsha]

I'd recommend removing this branch. Now that the results chan is buffered to the full length of possible outputs, writing to it (on line 479) is guaranteed not to block. Putting a case <-ctx.Done() in doesn't change anything.

[beautifulentropy]

This was added to facilitate early termination of the goroutines, not prevent blocking on the channel.

[jsha]

Right. I'm saying the goroutine will terminate at the same time even without the case <-ctx.Done(), because the only other thing that's happening here is the channel write, which doesn't block.