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.

Sign up for GitHub

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

Jump to bottom

Disable PTO at server if path is not validated #1183

Closed
martinthomson opened this issue Jul 22, 2021 · 1 comment · Fixed by #1875
Closed

Disable PTO at server if path is not validated #1183

martinthomson opened this issue Jul 22, 2021 · 1 comment · Fixed by #1875
Labels
server-side Issue relates more to server-side

Comments

@martinthomson
Copy link
Member

We currently send probes on PTO always. However a server is not allowed to when the active path hasn't been validated. This is preventing us from passing amplification limit tests.

This is not particularly urgent as we don't ship a server currently. But it is also a relatively small amount of work. The only challenge might be getting the path validation state to the right place(s). We have tests for the amplification limit, but these are flawed (they pass, they should not), so the biggest part of this work is in tweaking those tests.
@martinthomson martinthomson added the server-side Issue relates more to server-side label Jul 22, 2021
@martinthomson martinthomson mentioned this issue Jul 22, 2021
Closed
9 tasks
@larseggert larseggert mentioned this issue Apr 19, 2024
Closed
@larseggert
Copy link
Collaborator

Things seem to have deteriorated in the meantime. The server makes the initial path permanent before it sends the server-initial, so the entire (possibly still flawed) amplification logic is disabled.

larseggert added a commit to larseggert/neqo that referenced this issue May 3, 2024
@larseggert
fix: Make neqo pass amplificationlimit QNS test
fb705c9 
Fixes mozilla#1183
@larseggert larseggert mentioned this issue May 3, 2024
Merged
larseggert added a commit to larseggert/neqo that referenced this issue Jul 17, 2024
@larseggert
fix: Make neqo pass amplificationlimit QNS test
c495ce4 
Fixes mozilla#1183
larseggert added a commit to larseggert/neqo that referenced this issue Jul 17, 2024
@larseggert
fix: Make neqo pass amplificationlimit QNS test
bcd953d 
Fixes mozilla#1183
larseggert added a commit to larseggert/neqo that referenced this issue Jul 19, 2024
@larseggert
fix: Make neqo pass amplificationlimit QNS test
49450c4 
Fixes mozilla#1183
github-merge-queue bot pushed a commit that referenced this issue Aug 1, 2024
@larseggert @martinthomson
fix: Make neqo pass amplificationlimit QNS test (#1875)
c80630b 
* fix: Make neqo pass `amplificationlimit` QNS test

Fixes #1183

* Fix some tests

* Update neqo-transport/src/connection/tests/handshake.rs

Co-authored-by: Martin Thomson <mt@lowentropy.net>
Signed-off-by: Lars Eggert <lars@eggert.org>

* Address code review

* Fix idle_timeout_crazy_rtt

* Clarify test

* Restore

* Hopefully, a fix

* Nit

* Tweak

* Fix tests

* Fix

* Minimize diff

* Minimize more

* Fix?

* Fix?

* Fix?

* Deal with cancelled runs

* Try

* Roll back

* Again

* fmt

* Suggestion from @martinthomson

---------

Signed-off-by: Lars Eggert <lars@eggert.org>
Co-authored-by: Martin Thomson <mt@lowentropy.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
server-side Issue relates more to server-side
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: Make neqo pass amplificationlimit QNS test larseggert/neqo
2 participants
@martinthomson @larseggert