Spec clarifications for circuit timeouts and related things in path-spec.txt
This branch tries to fix the issues I found while working on Arti's circuit timeout code.
Closes #61 (closed).
Closes #48 (closed).
Please ignore all the changes in this branch made before 27 August: Apparently the "main" branch on this repository is out-of-date.
Merge request reports
Activity
requested review from @mikeperry
413 413 good fit over the long term, clients store 1000 most recent circuit build 414 414 times in a circular array. 415 415 416 These build times only include the times required to build three-hop 417 circuits. Other circuits' build times are not included. 418 -
This should be:
These build times only include the time required to build circuits up to the third hop. Circuits that are longer than the three hops are recorded at the point where the third hop is successfully extended, and are not recorded after that. Circuits shorter than three hops are not recorded.See
circuit_build_times_handle_completed_hop()andcircuit_timeout_want_to_count_circ()-- we actually check the opened length before recording there.This distinction is important because it allows us to still compute a timeout even if a client only builds onion service circuits.
Edited by Mike Perry
534 534 If the timeout was already at least `cbtinitialtimeout`, 535 535 the client doubles the timeout. 536 536 537 The records here (of how many circuits succeeded or failed among the most 538 recent 'cbrrecentcount') are also stored as persistent state. On reload, 539 the history here is reconstructed from the counts by placing successes and 540 failures in an arbitrary order. (The C Tor implementation shuffles them, 541 whereas Arti always places failures before successes so that they expire 542 sooner.) 543 -
In the interest of simplicity, I think ripping out the storage of this data is good. I am not seeing a super-strong reason to store this data.
The purpose of this liveness code is to determine if Tor previously picked a timeout that is now too low to provide enough path diversity (due to changing characteristics of the local internet connection, or the Tor guard/network).
I think it is a reasonable enough assumption that if Tor has restarted, this kind data is no longer fresh enough to be accurate for this purpose. This is also only 20 circuits here, and typical timeouts are now around 1-2 seconds or less.. So a restarted client with a timeout that is too low for a new internet connection will figure this out pretty quickly. I think that is OK.
Edited by Mike Perry
643 643 Default: 60000 644 644 Min: Value of cbtmintimeout 645 645 Max: 2147483647 (INT32_MAX) 646 Effect: This is the timeout value to use before computing a timeout, 647 in milliseconds. 646 Effect: This is the timeout value to use before we have enough data 647 to compute a timeout, in milliseconds. The circuit close 648 time defaults to 1.5 times this value. 648 649 -
I don't think there is a practical difference here. As per Section 2.4.5, if 60 seconds is not enough and causes the liveness test to fail due to too many timeouts, we will double the initial timeout. (Also, as long as we don't mess up congestion control, circuit build times should never approach anywhere near 60s again :)
If for some reason it made your implementation simpler to have these values be initialized differently, perhaps this can be a MAY? (I don't think it fragments anonymity set to do this differently at implementation discretion, again because this should not affect circuits that are actually usable anyway.)
mentioned in commit arti@5d7b0899
mentioned in commit arti@74fa1845
