[tor-bugs] #33631 [Circumvention/BridgeDB]: BridgeDB doesn't allow bridges to change their distribution mechanism

[Tor Bug Tracker & Wiki]

#33631: BridgeDB doesn't allow bridges to change their distribution mechanism
------------------------------------+-------------------------------
 Reporter:  phw                     |          Owner:  phw
     Type:  defect                  |         Status:  needs_review
 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 phw):

 * status:  needs_revision => needs_review


Comment:

 Replying to [comment:4 cohosh]:
 > - 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,))}}}
 [[br]]
 Good catch! I fixed it
 [https://github.com/NullHypothesis/bridgedb/commit/164552a070c59c6a00066fe618aee12f3bf3df59
 here].
 [[br]]
 > - 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?
 [[br]]
 That's a good observation, also
 [https://github.com/NullHypothesis/bridgedb/commit/94b3e63188c9b2b5136700347c0db20ee48c1586
 fixed].
 [[br]]
 > - Do the tests also check that bridges were removed from all subrings?
 Can we do that easily?
 [[br]]
 Actually, no. Fixed
 [https://github.com/NullHypothesis/bridgedb/commit/ef71bf152cb7d1b45e1048f37af6b0d2ce1e6eca
 here].

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/33631#comment:5>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online