Skip to content
Snippets Groups Projects

Tor Manual: Alphabetize Client Options

    • Closed (moved) Issue created by Trac @tracbot

    This ticket is for the alphabetical sorting of Client options. Making this a child ticket for #4310 (moved).

    Trac:
    Username: swati

    To upload designs, you'll need to enable LFS and have an admin enable hashed storage. More information

    Child items ...

    • Activity

    • Trac
      Trac @tracbot ·
      Author

      Trac:
      Parent Ticket: #4310 (moved)

    • Trac added component::core tor/tor documentation extra-review gsod manpage parent::4310 priority::medium reporter::swati resolution::implemented reviewer::catalyst reviewer::teor severity::normal status::closed tor-client type::project labels

      added component::core tor/tor documentation extra-review gsod manpage parent::4310 priority::medium reporter::swati resolution::implemented reviewer::catalyst reviewer::teor severity::normal status::closed tor-client type::project labels

    • Trac
      Trac @tracbot ·
      Author

      This is the PR for this ticket - https://github.com/torproject/tor/pull/1632.

      Please review.

      Trac:
      Username: swati

    • Roger Dingledine
      Roger Dingledine @arma ·

      Trac:
      Status: new to needs_review

    • Roger Dingledine
      Roger Dingledine @arma ·

      swati: I just added you to GRP_user on trac, which should give you more permissions than you had before.

    • C
      Taylor Yu @catalyst ·

      Thanks for the pull request!

      Running

      sed -ne '/^== CLIEN/,/^==/p'|sed -ne 's/^\[\[\([^ ]*\)\]\].*/\1/p'

      to extract the entry anchors, and comparing sorted and unsorted versions, I see:

      --- /tmp/unsort.txt	2020-01-03 14:04:32.000000000 -0600
+++ /tmp/sorted.txt	2020-01-03 14:04:47.000000000 -0600
@@ -2,10 +2,9 @@
 AutomapHostsOnResolve
 AutomapHostsSuffixes
 Bridge
-CircuitsAvailableTimeout
-LearnCircuitBuildTimeout
 CircuitBuildTimeout
 CircuitPadding
+CircuitsAvailableTimeout
 CircuitStreamTimeout
 ClientAutoIPv6ORPort
 ClientBootstrapConsensusAuthorityDownloadInitialDelay
@@ -29,16 +28,19 @@
 DownloadExtraInfo
 EnforceDistinctSubnets
 EntryNodes
-ExcludeNodes
 ExcludeExitNodes
+ExcludeNodes
 ExitNodes
 FascistFirewall
 FirewallPorts
 GeoIPExcludeUnknown
+GuardfractionFile
+GuardLifetime
 HidServAuth
 HSLayer2Nodes
 HSLayer3Nodes
 HTTPTunnelPort
+LearnCircuitBuildTimeout
 LongLivedPorts
 MapAddress
 MaxCircuitDirtiness
@@ -47,17 +49,21 @@
 NATDPort
 NewCircuitPeriod
 NodeFamily
+NumDirectoryGuards
+NumEntryGuards
+NumPrimaryGuards
 OptimisticData
+OtherSocksPortFlags
 PathBiasCircThreshold
 PathBiasDropGuards
 PathBiasExtremeRate
+PathBiasExtremeUseRate
 PathBiasNoticeRate
-PathBiasWarnRate
-PathBiasScaleThreshold
-PathBiasUseThreshold
 PathBiasNoticeUseRate
-PathBiasExtremeUseRate
+PathBiasScaleThreshold
 PathBiasScaleUseThreshold
+PathBiasUseThreshold
+PathBiasWarnRate
 PathsNeededToBuildCircuits
 ReachableAddresses
 ReachableDirAddresses
@@ -68,7 +74,6 @@
 SafeSocks
 SocksPolicy
 SocksPort
-OtherSocksPortFlags
 SocksPortFlagsMisc
 SocksTimeout
 StrictNodes
@@ -81,12 +86,7 @@
 UpdateBridgesFromAuthority
 UseBridges
 UseEntryGuards
-GuardfractionFile
 UseGuardFraction
-GuardLifetime
-NumDirectoryGuards
-NumEntryGuards
-NumPrimaryGuards
 UseMicrodescriptors
 VirtualAddrNetworkIPv4
 VirtualAddrNetworkIPv6

      I'm checking to see whether each out-of-order entry is commented with a suitable explanation. Anyone else like to help out with that?

      It looks like this line was accidentally removed from CircuitBuildTimeout?

      -    (Default: 60 seconds)

      There are also a few possible line movements that git diff --color-moved isn't unambiguously marking as "moved", so I'll want to do some extra checking of those.

    • Trac
      Trac @tracbot ·
      Author

      Hi Taylor,

      Thanks for looking into this. I would also like to point out that I haven't really organized the PathBias config options because they are just listed out without any descriptions or information. Not sure if they're just placeholders.

      I just compared the files and confirmed that that line under CircuitBuildTimeout was accidentally removed.

      Trac:
      Username: swati

    • C
      Taylor Yu @catalyst ·

      Replying to swati:

      Thanks for looking into this. I would also like to point out that I haven't really organized the PathBias config options because they are just listed out without any descriptions or information. Not sure if they're just placeholders.

      There seem to be descriptions for the PathBias options collected together under a single item. These options seem very specialized, and we probably want to put them in their own subsection later. For now, leaving them out of order (and commented as logically belonging together) makes sense to me.

      I also just noticed that OtherSocksPortFlags is explicitly commented as being there only for formatting purposes, so we should also let it remain out of order. (Though maybe we should also consider putting SocksPort flags in their own subsection later?)

      I just compared the files and confirmed that that line under CircuitBuildTimeout was accidentally removed.

      Thanks!

    • David Goulet
      David Goulet @dgoulet ·

      Trac:
      Reviewer: N/A to catalyst

    • C
      Taylor Yu @catalyst ·

      CircuitsAvailableTimeout should go before CircuitStreamTimeout if we're alphabetizing ignoring case.

      I think LearnCircuitBuildTimeout can also go immediately after CircuitBuildTimeout, or it can go in alphabetic order because CircuitBuildTimeout adequately describes LearnCircuitBuildTimeout anyway.

      I'm undecided whether ExcludeExitNodes logically belongs out of order after ExcludeNodes; what do other people think?

      The PathBias options could use a comment explaining why they're intentionally out of order.

      I can make a branch that fixes up these, unless you want to make the changes yourself?

    • C
      Taylor Yu @catalyst ·

      Updated branch in https://github.com/torproject/tor/pull/1643 Please let me know what you think.

      Trac:
      Keywords: N/A deleted, extra-review added
      Reviewer: catalyst to catalyst, teor

    • T
      teor @teor ·

      Looks good!

      I've tried to prioritise the changes I'm suggesting. Feel free to mark as merge ready when the "must" and "useful" changes are done.

      Here are the things I think we must fix before merging:

      • GuardfractionFile should be moved to the directory authority section. It doesn't apply to clients.

      Here are some useful things that I think would help people read the manual:

      • ReducedConnectionPadding logically belongs immediately after ConnectionPadding.
      • ReducedCircuitPadding logically belongs immediately after CircuitPadding.
        • CircuitPadding splits the circuit timeout option group. If we're going to put it out of order anyway, maybe we should put it with ConnectionPadding.
      • FascistFirewall* logically belongs after Reachable*Addresses.
      • Reject and Warn PlaintextPorts belong together, probably near SafeSocks. They all do similar things.

      Here are things we could fix now, but we could also do the fix in another ticket, possibly by creating a new subsection:

      • StrictNodes and GeoIPExcludeUnknown might logically belong after ExcludeNodes
      • All the Nodes options (including the HSNodes) options, would make sense together
      • This is a big change, so it probably needs another ticket

      If we create a section for the *Nodes options, we could mark them as advanced options. They're all options that might compromise anonymity, by changing how tor behaves.

      Trac:
      Status: needs_review to needs_revision

    • C
      Taylor Yu @catalyst ·

      Replying to teor:

      Looks good! Thanks for the review! I updated the pull request.

      I've tried to prioritise the changes I'm suggesting. Feel free to mark as merge ready when the "must" and "useful" changes are done.

      Here are the things I think we must fix before merging:

      • GuardfractionFile should be moved to the directory authority section. It doesn't apply to clients. I agree. Moved.

      Here are some useful things that I think would help people read the manual:

      • ReducedConnectionPadding logically belongs immediately after ConnectionPadding.
      • ReducedCircuitPadding logically belongs immediately after CircuitPadding.
        • CircuitPadding splits the circuit timeout option group. If we're going to put it out of order anyway, maybe we should put it with ConnectionPadding. I agree, except with moving CircuitPadding. As a slightly more "top-level" option, leaving it in alphabetic order might be better. Maybe we should make a new ticket for moving the timeout options in a new subsection?
      • FascistFirewall* logically belongs after Reachable*Addresses. I left this one alone, because we clearly describe FascistFirewall as deprecated and refer to ReachableAddresses.
      • Reject and Warn PlaintextPorts belong together, probably near SafeSocks. They all do similar things. Done. Warn now precedes Reject because of how their descriptions are worded. Also moved TestSocks after SafeSocks because they're related and they were like that pre-alphabetization.

      Here are things we could fix now, but we could also do the fix in another ticket, possibly by creating a new subsection:

      • StrictNodes and GeoIPExcludeUnknown might logically belong after ExcludeNodes
      • All the Nodes options (including the HSNodes) options, would make sense together
      • This is a big change, so it probably needs another ticket

      If we create a section for the *Nodes options, we could mark them as advanced options. They're all options that might compromise anonymity, by changing how tor behaves. I agree; let's make a new ticket for these.

      Trac:
      Status: needs_revision to merge_ready

    • T
      teor @teor ·

      Replying to catalyst:

      Replying to teor:

      Here are some useful things that I think would help people read the manual:

      • CircuitPadding splits the circuit timeout option group. If we're going to put it out of order anyway, maybe we should put it with ConnectionPadding. I agree, except with moving CircuitPadding. As a slightly more "top-level" option, leaving it in alphabetic order might be better. Maybe we should make a new ticket for moving the timeout options in a new subsection?

      This is now #32928 (moved).

      Here are things we could fix now, but we could also do the fix in another ticket, possibly by creating a new subsection:

      • StrictNodes and GeoIPExcludeUnknown might logically belong after ExcludeNodes
      • All the Nodes options (including the HSNodes) options, would make sense together
      • This is a big change, so it probably needs another ticket

      If we create a section for the *Nodes options, we could mark them as advanced options. They're all options that might compromise anonymity, by changing how tor behaves. I agree; let's make a new ticket for these.

      This is now #32929 (moved).

    • Nick Mathewson
      Nick Mathewson @nickm ·

      Squashed and merged. Does this ticket close now?

    • Nick Mathewson
      Nick Mathewson @nickm ·

      I'm told it does. Closing.

      Trac:
      Resolution: N/A to implemented
      Status: merge_ready to closed

    • T
      teor @teor ·

      Trac:
      Cc: catalyst, teor to catalyst

    • Trac closed

      closed

    • Trac moved to tpo/core/tor#32846 (closed)

      moved to tpo/core/tor#32846 (closed)

    Please register or sign in to reply