[tor-bugs] #9264 [BridgeDB]: Problem with transport lines in BridgeDB's bridge pool assignment files

[Tor Bug Tracker & Wiki]

#9264: Problem with transport lines in BridgeDB's bridge pool assignment files
--------------------------+--------------------------------------------
     Reporter:  karsten   |      Owner:  isis
         Type:  defect    |     Status:  needs_review
     Priority:  major     |  Milestone:
    Component:  BridgeDB  |    Version:
   Resolution:            |   Keywords:  bridgedb-0.1.3, bridgedb-0.1.5
Actual Points:            |  Parent ID:
       Points:            |
--------------------------+--------------------------------------------
Changes (by sysrqb):

 * status:  needs_revision => needs_review


Comment:

 Replying to [comment:34 isis]:
 > Comments:
 >
 >  1. In
 [https://gitweb.torproject.org/user/sysrqb/bridgedb.git/commitdiff/ff3a2641ac3447238511ce3436386d9ee2553011
 commit ff3a2641ac3447238511ce3436386d9ee2553011]... somewhere a while ago
 (not in this commit specifically) I think the `COLLECT_TIMESTAMPS` option
 for #10724 got lost. I think I remember seeing it re-added in your #5232
 branch though.
 >

 Darn. It looks like it was dropped in
 967f1a6a4290154977ab7c0cc2cbe0560d7246ce :( not good. I'll fix this
 (unless you beat me to it).

 >  2. In 13fc557b8e4bd094457ca954129dceaa4445d744, the
 `test_splitterBridgeInsertion` is great!
 >
 >  3. In 92eb6fe9e7d4e1e7cbb72687b9f41f5f827fa182:
 >
 >     {{{
 > -        self.bridges.append(bridge)
 > +        index = 0
 > +        logging.debug("Inserting %s into %s ring" %
 (bridge.fingerprint, self.key))
 > +        for old_bridge in self.bridges:
 > +            if bridge.fingerprint == old_bridge.fingerprint:
 > +                self.bridges[index] = bridge
 > +                break
 > +            index += 1
 > +        else:
 > +            self.bridges.append(bridge)
 > }}}
 >
 >  Don't you want to use `Util.logSafely()`?
 Probably a good idea for the fingerprint. I removed the `key` because it
 was unhelpful.

 > And doesn't the `self.bridges[index] = bridge` mean that, if there were
 already duplicates in `self.bridges`, that only the first duplicate is
 replaced and the others remain in the list?
 >

 yes, however, 1) this should also prevent duplicates from being inserted
 in the first place, and 2) because we only insert bridges if they are
 defined in the NS we should never even try to insert the same bridge more
 than once (for each call to Main.load()). If we were still unconditionally
 parsing all descriptors then this would definitely be a concern. We only
 see this bug on SIGHUP when we create a new, distinct, instance of Bridge
 to represent a bridge in the splitter. If an instance of Bridge already
 exists in the splitter for that bridge, then, previously, the new instance
 was appended to the list (resulting in the splitter holding two instances
 for the same bridge), now the new instance should overwrite the old
 instance. They're distinct instances, so new != old, which is why we
 compare fingerprints.

 (sorry if you already understood this and I didn't understand the
 question)

 >  Also, it's a bit odd, but you might want to do `for old_bridge in
 self.bridges[:]:` instead of `for old_bridge in self.bridges:`, because of
 a list comprehension bug in Python (I don't recall off the top of my head
 what version it was fixed in...) where there is an off-by-one error in
 calculating the number of elements in the list (but only with really large
 lists like this one).
 >

 (4000 is really large? :)) Better to play it safe. Changed.

 >  4. In general, don't worry about the space taken up by splitting
 unittests up into many smaller tests, because the first thing that raises
 an exception means all the rest of the test doesn't get run.
 >

 True, true

 > ---------
 >
 > Other than the tiny things mentioned above, I think this should be
 merged immediately for version 0.1.5.

 Added two commits in
 [https://gitweb.torproject.org/user/sysrqb/bridgedb.git/shortlog/refs/heads/bug9264_rebased_3_r1
 bug9264_rebased_3_r1], which I think solve the above comments.

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