[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #33631 [Circumvention/BridgeDB]: BridgeDB doesn't allow bridges to change their distribution mechanism
#33631: BridgeDB doesn't allow bridges to change their distribution mechanism
------------------------------------+--------------------------------
Reporter: phw | Owner: phw
Type: defect | Status: needs_revision
Priority: Medium | Milestone:
Component: Circumvention/BridgeDB | Version:
Severity: Normal | Resolution:
Keywords: s30-o23a1 | Actual Points:
Parent ID: #31280 | Points: 0.5
Reviewer: cohosh | Sponsor: Sponsor30-can
------------------------------------+--------------------------------
Changes (by cohosh):
* status: needs_review => needs_revision
Comment:
Hey! Looks good, I like the general structure of the changes. Some
feedback:
- Since we're ignoring the returned distributor from the SQL query
[https://github.com/NullHypothesis/bridgedb/compare/develop...defect/33631?expand=1
#diff-f5e72cabd3c2e29288c80ecdbfc51cabL185 here], it makes more sense to
modify the query to return only what we need:
{{{cur.execute("SELECT id FROM Bridges WHERE hex_key = ?", (h,))}}}
- The logic for removing bridges by finding their index
[https://github.com/NullHypothesis/bridgedb/compare/develop...defect/33631?expand=1
#diff-db025301d124ca0c9fe5cfeea36077c8R626 here] seems a bit unwieldy to
me. I noticed another, similar function of the code
[https://github.com/NullHypothesis/bridgedb/blob/21cf51c227d8e53ca70ff7e4831e8b770d6eeb9a/bridgedb/Bridges.py#L658
here] that does the following:
{{{
index = 0
logging.debug("Inserting %s into hashring..." % bridge)
for old_bridge in self.bridges[:]:
if bridge.fingerprint == old_bridge.fingerprint:
self.bridges[index] = bridge
break
index += 1
}}}
Is there a better way to handle this? Or a way to be more consistent?
Perhaps we could refactor this logic into its own function that takes a
bridge and returns its index in `self.bridges`. Would making
`self.bridges` a map help?
- Do the tests also check that bridges were removed from all subrings? Can
we do that easily?
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/33631#comment:4>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs