Add sentences about order of ByRoot responses

[jtraglia]

I noticed that BlocksByRoot and BlobSidecarsByRoot do not mention anything about order of responses. This PR adds a sentence to each section which states responses must be sent in the order they were requested.

Here's where the ByRange sections state this:

consensus-specs/specs/phase0/p2p-interface.md

Line 804 in 36f0bb0

The following blocks, where they exist, MUST be sent in consecutive order.

consensus-specs/specs/deneb/p2p-interface.md

Line 365 in 36f0bb0

The following blob sidecars, where they exist, MUST be sent in consecutive `(slot, index)` order.

[jtraglia]

Also, are duplicate roots allowed? For example, should a request for [A, B, C, B] return [A, B, C]?

[terencechain]

This was discussed at the ACD call. Currently, Prysm implementation does not sort the 'by root' response. Instead, it matches the original input, such as [0xA, 0xB, 0xC] -> [BlockA, BlockB, BlockC], even if the slots are in a different order. I don't feel strongly about sorting them, and it's good to spec out the response indeed

[nisdas]

Also, are duplicate roots allowed? For example, should a request for [A, B, C, B] return [A, B, C]?

There was nothing in the spec disallowing this, so duplicate roots are allowed. Prysm will respond back with a duplicate block rather than filter it out.

[rolfyone]

Is there value in enforcing the order? Currently we're just processing in the order and not de-duplicating, so i'm curious why we feel that we now require an ordering...

Reading our request structure, it does appear we'd just return in line with the request - if you request duplicates you'd get duplicates...

[arnetheduck]

We discussed it at some point but didn't take action at the time - when processing a byroot response you necessarily need to compute the hash anyway so the order of responses doesn't greatly matter practically. In fact, imposing an order means that requesters must verify the order and penalise out-of-order responses which ends up being extra code for no practical benefit.

I wouldn't impose any order on the response - it might be worth documenting the above rationale since this question comes up every now and then.

I would support specifying that duplicate roots in request are not allowed as these can trivially be filtered by the requesting side and they are detrimental to the overall quality of service since they waste bandwidth, giving us a reason to write the dupe-checking code in the request handler.

After a hardfork, I'd happily penalise any client sending them.

[ppopth]

In fact, imposing an order means that requesters must verify the order and penalise out-of-order responses which ends up being extra code for no practical benefit.

The requesters don't have to verify that, right? We don't have a statement that states that the requesters have to do so, so, even if we enforces the statements in this PR, it doesn't mean that we enforce the verification on the requester side as well. So no extra code on the requester side.

I think the requesters should assume that the response is arbitrary and care only about whether they get what they want.

The benefit of this PR may be just consistency among clients. If we don't want to be too strict on it, we can change from MUST to SHOULD.

[jtraglia]

Thanks everyone. Sounds like enforcing order doesn't have any benefits besides consistency. I'm open to changing that to a SHOULD or even specifying that order does not matter. As for duplicate roots, I like the idea of rejecting these in the request handler and potentially penalizing peers for that later. I see no reason these should be allowed.

[jtraglia]

@zilm13's comment on a Teku issue about this seems relevant:

Btw I've checked our code and if we get a response in a wrong order we will fail on import because for BlobSidecars it could mean that you request [b0s0, b0s1, b0s2] and receive [b0s2, b0s0, b0s1]. So if MUST is not added to the spec, we need to add sorting somewhere.

[zilm13]

The question is not about any sorting, instead about requirement to match originally requested input:
[A, C, B] -> [entityA, entityC, entityB] correct, [entityA, entityB] correct (gaps are allowed by the current spec)
[A, C, B] -> [entityA, entityB, entityC] not correct

[jtraglia]

This is stale & doesn't really matter. Closing.