[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_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