#28655: If a bridge supports obfs4, don't give out its other flavors ------------------------------------+-------------------------------- Reporter: arma | Owner: phw Type: defect | Status: needs_revision Priority: High | Milestone: Component: Circumvention/BridgeDB | Version: Severity: Normal | Resolution: Keywords: bridgedb | Actual Points: Parent ID: | Points: 2 Reviewer: | Sponsor: Sponsor19 ------------------------------------+-------------------------------- Changes (by sysrqb):
* status: needs_review => needs_revision Comment: Sorry this took so long to review. It looks good! I only have a few small nitpicks. - [https://gitweb.torproject.org/user/phw/bridgedb.git/tree/bridgedb/test/test_https_distributor.py?h=bug28655&id=ad034c358a38dcea98948d4b2c50e0758f15ce13#n197 Reusing] `bridges` is a little confusing (used first as a method parameter), can you use a different variable name? - Ugh, [https://gitweb.torproject.org/user/phw/bridgedb.git/tree/bridgedb/filters.py?h=bug28655&id=ad034c358a38dcea98948d4b2c50e0758f15ce13#n157 python]. Sure, I guess that's correct. (no changes needed) - I agree extending `SUPPORTED_TRANSPORTS` or creating a new `list` config option like `PROBING_RESISTANT_TRANSPORTS` is a good idea. Hard-coding the list of probing resistant PTs in one place is not great, but hard-coding them in two places is asking for bugs :) Overall, I think the bridgedb patch is good. I worry about making `SUPPORTED_TRANSPORTS` more complex. I think a simple `str` -> `boolean` mapping is easy to understand. The two ways I see doing this are not as obvious: `dict` -> `tuple` as `str` -> (`boolean`, `boolean`) `dict` -> `dict` -> `boolean` as `str` -> `str` ({supported, probing resistant}) -> `boolean`) Maybe you had another idea for this, as well :) but with these options, I think adding a new config variable would be more readable. For the leekspin patch, I think it looks good. My only concern is in the [https://gitweb.torproject.org/user/phw/leekspin.git/tree/leekspin/util.py?id=3bc9c660e8df80fe89693c8e4fad38955011bf20#n65 description] of the new argument. It says `m` out of `n`, but it's not immediately obvious what `m` is here. `n` is an actual argument (`-n`, `--descriptors`), but `m` is not a valid argument. Replacing `<m>` with `<xp>` would make it more readable, or somehow note `m` **is** `xp`: "make `<m>` (`xp`) out of all `<n>`". -- Ticket URL: <https://trac.torproject.org/projects/tor/ticket/28655#comment:17> Tor Bug Tracker & Wiki <https://trac.torproject.org/> The Tor Project: anonymity online
_______________________________________________ tor-bugs mailing list tor-bugs@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs