Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add sentences about order of ByRoot responses #3544

Closed
wants to merge 3 commits into from

Conversation

jtraglia
Copy link
Member

@jtraglia jtraglia commented Nov 6, 2023

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:

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

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

@jtraglia
Copy link
Member Author

jtraglia commented Nov 8, 2023

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

@terencechain
Copy link
Contributor

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
Copy link
Contributor

nisdas commented Nov 30, 2023

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
Copy link
Contributor

rolfyone commented Dec 4, 2023

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
Copy link
Contributor

arnetheduck commented Dec 4, 2023

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
Copy link
Member

ppopth commented Dec 5, 2023

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
Copy link
Member Author

jtraglia commented Dec 5, 2023

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
Copy link
Member Author

jtraglia commented Dec 6, 2023

@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
Copy link
Contributor

zilm13 commented Mar 14, 2024

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
Copy link
Member Author

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

@jtraglia jtraglia closed this Aug 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants