Coalesce outgoing packets #342
Conversation
| assert_eq!( | ||
| buf[..], | ||
| hex!("c0ff0000145006b858ec6f80452b00402100 00000000000000000000000000000000")[..] | ||
| ); |
There was a problem hiding this comment.
Why was this check removed? It's nice to isolate encoding breakage from crypto breakage.
There was a problem hiding this comment.
The new API no longer exposes the steps separately, so if we still want to have this test we have to duplicate the code from PartialEncode::finish() or break that code up in smaller pieces that can be called independently. I prefer not to make the functional code "less nice" in order to appease test code.
There was a problem hiding this comment.
Would it make sense to restore this test with an unset payload length, just to have specific coverage of the other aspects of header encoding?
|
The test failure is unrelated to the correctness of this change. |
There was a problem hiding this comment.
ACK encoding currently assumes that there is always room for at least MAX_ACK_BLOCKS (which is currently probably larger than necessary) ACK blocks in a packet, because without coalesced packets this follows from the minimum MTU. With coalesced packets, we need to handle the case where that is no longer true, for example by bailing out early, or by only sending the most recent ACKs that will fit.
|
There are some more issues to watch for: the spec mentions coalesced packets should follow the order of Initial, 0-RTT, Handshake, 1-RTT. As such, if the client is sending |
|
In lieu of a strict requirement by the draft or specific knowledge of an implementation that cannot decrypt 0-RTT packets received following handshake packets, I don't think that's worth going well out of our way for. After all, implementations should hold onto 0-RTT keys for a while even after reaching 1-RTT to improve performance under packet reordering. |
djc
force-pushed
the
coalesce
branch
2 times, most recently
from
0ee96b9
to
e516df1
Compare
We can no longer trivially test anti-amplification, but a simplified test for losing the server's initial flight is still useful.
| assert_eq!( | ||
| buf[..], | ||
| hex!("c0ff0000145006b858ec6f80452b00402100 00000000000000000000000000000000")[..] | ||
| ); |
There was a problem hiding this comment.
Would it make sense to restore this test with an unset payload length, just to have specific coverage of the other aspects of header encoding?
|
Might make sense, but in some experiments I couldn't get it to do something that seemed useful. Want to take a swing at it? |
Results in 3 fewer datagrams on the
echo_v4test.Currently the
server_hs_retransmit()test is still broken. @Ralith any suggestions if/how this should be fixed?