change MAY to MUST for bbr request with step greater than 1 #3277
Conversation
specs/phase0/p2p-interface.md
Outdated
| @@ -743,7 +743,7 @@ For example, requesting blocks starting at `start_slot=2` and `count=4` would re | |||
| In cases where a slot is empty for a given slot number, no block is returned. | |||
| For example, if slot 4 were empty in the previous example, the returned array would contain `[2, 3, 5]`. | |||
|
|
|||
| `step` is deprecated and must be set to 1. Clients may respond with a single block if a larger step is returned during the deprecation transition period. | |||
| `step` is deprecated and must be set to 1. Clients MUST respond with a single block if a larger step is returned during the deprecation transition period. | |||
There was a problem hiding this comment.
It seems like this is directly at odds with 748 which states clients MAY respond with an empty list...
Changing this without changing the other would increase confusion IMO, as if we MUST return a single block, we can't return an empty list...
To me having MAY in both locations is probably more representative of desired behaviour...
There was a problem hiding this comment.
Ah, I wasn't looking at spec when discussing this with @0xTylerHolmes. I thought the intention was that during the deprecation window, you can either [still respond with step requests] or [just respond with one]. So I though the MAY was confusing as I thoguht it was you MUST do one of these.
But you're right the MAY below is at odds with my interpretation.
Let's not change this currently. Should we call non-1-step fully deprecated as of Capella? I don't think any client will be making such calls after that upgrade
There was a problem hiding this comment.
It seems like this is directly at odds with 748 which states clients MAY respond with an empty list...
Changing this without changing the other would increase confusion IMO, as if we MUST return a single block, we can't return an empty list...
To me having
MAYin both locations is probably more representative of desired behaviour...
perhaps specifying the MUST with an either clause for empty list or one for both paths would be better?
I think MAY is confusing as clients implement this differently. For example some return the full range and totally ignore the step. Which from my interpretation is incorrect but aligns with the spec as written.
There was a problem hiding this comment.
Let's not change this currently. Should we call non-1-step fully deprecated as of Capella? I don't think any client will be making such calls after that upgrade
Yep I think that would be more than fair.
perhaps specifying the MUST with an either clause for empty list or one for both paths would be better?
Im not sure if there's other areas to draw on, but logically MUST do one of X or Y or Z should be pretty clear I would think...
There was a problem hiding this comment.
Okay, so I've reread this and there is a confusion. There are two things that are attempted to be deprecated and the things are smashing together in a confusing way. 748 has nothing to do with step
step is defined in blocks_by_range/1 and blocks_by_range/2. The deprecation notice here is to say that it is being deprecated in both and that the path is that during that period, you can either honor the step or just return one message. You MUST do one of those.
line 748 says independently that blocks_by_range/1/ is deprecated. and you MAY respond with an empty list
So this is what the logic is
blocks_by_range/1/-- either respond with empty list OR [support step OR respond with one message]blocks_by_range/2/-- support step OR respond with one message
because the deprecation of step is for v1 and v2, you cannot just blanket use 748 to respond with empty in v2.
There was a problem hiding this comment.
I think the fact that this conversation is quite this large is an indication that something needs changing :)
There was a problem hiding this comment.
So this is what the logic is
blocks_by_range/1/-- either respond with empty list OR [support step OR respond with one message]blocks_by_range/2/-- support step OR respond with one messagebecause the deprecation of step is for v1 and v2, you cannot just blanket use 748 to respond with empty in v2.
This is a good description I think, helped me understand it all more clearly...
There was a problem hiding this comment.
I have tried making the language more clear by mentioning that step > 1 in all versions MAY return a single block or honor the step. I think this makes it as clear as possible without referring to a later version. I think it makes it clear enough without mentioning the edge case in the blocks_by_range/1/. If this still seems unclear I'll mention the edge case in 748
… MAY to avoid confusion in cases where a v1 request is used with step > 1
This PR seeks to clarify how clients handle BBR requests when the deprecated step parameter is greater than 1. See 2856