Concurrent requests in a Portal protocol lookup get canceled before data is validated

[kdeme]

[bhartnett]

@kdeme Is the goal for this issue to update portal protocol to return the first valid content and then cancel all other pending requests? In other words do we want to add validation into the contentLookup? I'm assuming that cancelling the pending request isn't a problem as long as we know that at least a single valid content response is returned.

[kdeme]

Yes, this is about not yet canceling the request until we are certain that the current one that we received has been validated and is correct. Practically that would probably mean adding a validation handler in wire protocol as you have already mentioned in the other PR.

I think that would be beneficial over retrying getContent attempts in the specific networks. But I am also not 100% sure, because in the happy case (which might/will occur more often), it means that we might be starting validation for no reason on a second or a third piece of data. Perhaps the actual validation should get queued? I haven't really thought it out that well yet.

[bhartnett]

Yes, this is about not yet canceling the request until we are certain that the current one that we received has been validated and is correct. Practically that would probably mean adding a validation handler in wire protocol as you have already mentioned in the other PR.

Note that the validation handler will need to be passed in to each call to contentLookup because the handler will be different for each type of content being validated and in some cases some context data needs to be available such as the block header when validating a block body. For this reason, using a single handler defined per sub-network won't work in this case.

I think that would be beneficial over retrying getContent attempts in the specific networks. But I am also not 100% sure, because in the happy case (which might/will occur more often), it means that we might be starting validation for no reason on a second or a third piece of data. Perhaps the actual validation should get queued? I haven't really thought it out that well yet.

Since chronos is single threaded if we fetch the content, validate and then cancel remaining pending queries with no await calls then I believe the validation won't run on a second query. I'm not completely sure about this but I can try it out to see.

[kdeme]

Note that the validation handler will need to be passed in to each call to contentLookup because the handler will be different for each type of content being validated and in some cases some context data needs to be available such as the block header when validating a block body.

Well, you could handle each type of content in the handler as you do know the different content keys. A bit like is done for incoming offers?
Regarding the block header, I think the data would have to be requested, which adds to the issue that I was mentioned of double/triple validation.

[bhartnett]

Note that the validation handler will need to be passed in to each call to contentLookup because the handler will be different for each type of content being validated and in some cases some context data needs to be available such as the block header when validating a block body.

Well, you could handle each type of content in the handler as you do know the different content keys. A bit like is done for incoming offers? Regarding the block header, I think the data would have to be requested, which adds to the issue that I was mentioned of double/triple validation.

Hmm, I don't like the idea of requesting additional data over the network just for the purposes of validation. It's not a very nice solution because you might end up requesting the same content multiple times for no reason. For example have a look at the getBlock proc in history network. In this case we need both the block header and block body. The header is requested first, then it is passed into the getBlockBody call. This way we only look up the header once. If looking up the header was embedded into the getBlockBody validation then we would look up the header content two times.

[kdeme]

Yes, you're right. It is not needed and history network already asks the header upfront as is. The code changes I am doing also remain doing it like that, I confused it with the validation code for offers which do have to request the header on the spot.

[bhartnett]

I started on a change to pass in a validation handler to each call to contentLookup but it turns out that this design leads to double ssz decoding of the types which isn't ideal. See commit here: e881144

Doesn't appear to be a good solution so I won't spend more time on this for now.