Clarify paramers_set merging of parameter values #409
Conversation
| - The local endpoint receives parameter values from the peer and logs a | ||
| `parameters_set` event with an `owner` of `remote` reflecting the desired | ||
| value(s) expressed by the peer. | ||
| - The local endpoint logs a `parameters_set` event with an `owner` of `local` |
There was a problem hiding this comment.
I find this very confusing. Why do we need to log anything at all? There are not many parameters that are negotiated (idle timeout. anything else?).
If we really want to log this, let's create a separate owner value for that.
There was a problem hiding this comment.
negotiated? I don't think this is super valuable to log, since it can easily be derived from the two parameters_set events anyway.
There was a problem hiding this comment.
So, I'm still not 100% sure what we're proposing here...
Option A:
- log
parameters_setwith owner=local for local value - log
parameters_setwith owner=remote for peer's value - log
parameters_setwith owner=negotiated for final value
or Option B:
- log
parameters_setwith owner=negotiated for local value - log
parameters_setwith owner=negotiated for peer's value - log
parameters_setwith owner=negotiated for final value
or Option C:
- log
parameters_setwith owner=negotiated final value
I'm not sure I like C (lose quite some information there... don't know which values the peers proposed originally?).
I'm also not sure how either A or B improves upon the proposed text though... you're still logging 3 events?
| Consequently, each parameter value in a `parameters_set` event supersedes the | ||
| previous value of that parameter if present. If a parameter does not appear in a | ||
| given `parameters_set` event, its value is unchanged from the previous event | ||
| which set that parameter. If a parameter never appears in a `parameters_set` |
There was a problem hiding this comment.
This doesn't work for 0-RTT, does it?
There was a problem hiding this comment.
0-RTT is special cased in the paragraph below this one and should use parameters_restored. (That's not to say that that is 100% watertight, I might have missed something in that flow, but it's indeed not the intent to use only parameters_set for 0-RTT and this new text is not intended for 0-RTT indeed).
There was a problem hiding this comment.
I don't think this event should be logged more than once per connection. Transport parameters are only exchanged once, after all. If we have a separate owner value as suggested above, this property would also hold in qlog.
There was a problem hiding this comment.
This event will be logged at least twice per connection by definition, since there are two sets of TPs: one for the local entity and one for the peer. This event also tracks more state than just transport parameters (e.g., TLS level params like resumption or early data) that are probably set in a different codepath/at a different time and benefit from having multiple parameters_set.
I agree with the subset of your statement that the event probably shouldn't be logged more than once per connection for specific fields with the same owner though.
Still, for me personally the most important part here is If a parameter does not appear in a given parameters_set event, its value is unchanged from the previous event which set that parameter. to help make clear that subsequent parameters_set do not invalidate earlier values, which I agree with @hlandau is not 100% clear right now.
| event, it retains its default value. This default value thus cannot be restored | ||
| implicitly (for example by omitting a parameter in a subsequent | ||
| `parameters_set`) and, if needed, should instead be restored explicitly by | ||
| re-assigning the default value in an new `parameters_set` event. |
There was a problem hiding this comment.
If we don't support null I guess that means some default values (i.e., absence of a TP) might be unrepresentable in qlog.
There was a problem hiding this comment.
I'm not sure I understand? Absence of a TP is simply that: absence of a TP in the parameters_set.
Whether this means the TP was actually absent on the wire or whether the implementation didn't/couldn't log it is indeed unclear, but the qlogger can do what they want (e.g., also simply couldn't log null), so that's never fixable imo.
If you mean to say that the default value of a TP is null, then I'd say that derives from the RFC that defines the TP, and absence implies that value?
There was a problem hiding this comment.
I guess this is not so much a problem for TPs as they are only exchanged once. But it seems like this event is intended to be more general.
For other kinds of parameters which might be renegotiable I could see a problem if it is possible for a feature to transition from negotiated to the default/non-negotiated state.
Maybe it is not a problem in practice? I'm just envisaging this event type being expanded to other things besides TPs in future.
Closes #398.
PTAL @hlandau.
The suggestion of using
nullas a JSON value in #398 makes conceptual sense, but not really since we're working with CDDL to define the events and their fields, as it doesn't easily allow to make a type "nullable" for something like this.As such, I changed the recommendation to explicitly setting the default value if needed; I hope that's acceptable.
For the rest, I mostly just used your text with minor editorial changes.