Opened 5 years ago

Closed 4 years ago

#9264 closed defect (fixed)

Problem with transport lines in BridgeDB's bridge pool assignment files

Reported by: karsten Owned by: isis
Priority: High Milestone:
Component: Obfuscation/BridgeDB Version:
Severity: Keywords: bridgedb-0.1.3, bridgedb-0.1.5
Cc: isis, aagbsn, sysrqb, asn Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

There seems to be a problem with BridgeDB writing transport lines to its bridge pool assignment files:

$ grep -c transport *
2013-07-13-00-01-22:123
2013-07-13-00-30-22:200
2013-07-13-01-00-23:275
2013-07-13-01-30-22:44
2013-07-13-02-00-25:121
2013-07-13-02-30-27:198
2013-07-13-03-00-33:281
2013-07-13-03-30-29:73

Numbers shouldn't go up and down so fast.

Also, I saw Isis mention this the other day, but I'm not sure if there's already a ticket for it:

1093d197b5eab404f30806948367483ad5c7482e email ip=4 flag=stable
112f02b648f19502831103fa50715ce31f8a4a2e email ip=4 flag=stable port=443
112f02b648f19502831103fa50715ce31f8a4a2e email ip=4 flag=stable port=443 transport=obfs3,obfs2
11b3a235c05843fbc57f29a848dee6dc6a6be118 unallocated

Note the duplicate entry for 112f02b648f19502831103fa50715ce31f8a4a2e. The first line shouldn't be there, I guess.

Setting priority to major, because the currently produced bridge pool assignments won't be useful for analysis.

Child Tickets

Attachments (1)

9264-sysrqb_rebased_0-BridgeDB · 2014-02-08 07-05-19.png (73.5 KB) - added by isis 4 years ago.

Download all attachments as: .zip

Change History (37)

comment:1 Changed 5 years ago by asn

This might also be related to #9174. We found that the bridge of #9174 sometimes published extrainfo descriptors that didn't include a transport line.

I will try look into this soon.

comment:2 in reply to:  1 Changed 5 years ago by hsn

Replying to asn:

We found that the bridge of #9174 sometimes published extrainfo descriptors that didn't include a transport line.

That bridge has AssumeReachable 1, because its always fails reachability check without it. I do not know if this could be related to not sometimes publishing transports.

comment:3 Changed 5 years ago by hsn

could be #8791 related?

comment:4 Changed 5 years ago by karsten

asn just found that BridgeDB might only parse cached-extrainfo.new and ignores cached-extrainfo. That would explain the pattern above where bridges with "transport" lines go up for two hours and then start at zero again.

The fix is to parse both cached-extrainfo* files in BridgeDB.

comment:5 in reply to:  4 ; Changed 5 years ago by sysrqb

Status: newneeds_review

Replying to karsten:

asn just found that BridgeDB might only parse cached-extrainfo.new and ignores cached-extrainfo. That would explain the pattern above where bridges with "transport" lines go up for two hours and then start at zero again.

The fix is to parse both cached-extrainfo* files in BridgeDB.

I have a patch for this in branch bug9264_extrainfo_files https://github.com/sysrqb/bridgedb.git

It's pretty simple.

comment:6 in reply to:  5 Changed 5 years ago by sysrqb

Replying to sysrqb:

parse both cached-extrainfo* files in BridgeDB.

I have a patch for this in branch bug9264_extrainfo_files https://github.com/sysrqb/bridgedb.git

Hm, do you think we should increase that log message, when an extrainfo file throws an exception on open, from warn to error?

comment:7 Changed 5 years ago by aagbsn

BridgeDB was incorrectly using only the new (cached-extrainfo.new) descriptors.
I have reverted to using the cached-extrainfo file, but as noted above we don't learn about the extra-info (and pluggable transports) till 2hrs later. sysrqb's fix looks like it should address this (thanks!).

From Karsten's response in the email thread, clarifying extrainfo behavior:

No.  Tonga writes descriptors to cached-extrainfo roughly every two
hours.  It then writes incoming descriptors to cached-extrainfo.new
without touching cached-extrainfo.  Only two hours later, it rewrites
cached-extrainfo and puts all descriptors in it that are still valid.
It also deletes cached-extrainfo.new.

comment:8 in reply to:  description Changed 5 years ago by aagbsn

Replying to karsten:

Also, I saw Isis mention this the other day, but I'm not sure if there's already a ticket for it:

1093d197b5eab404f30806948367483ad5c7482e email ip=4 flag=stable
112f02b648f19502831103fa50715ce31f8a4a2e email ip=4 flag=stable port=443
112f02b648f19502831103fa50715ce31f8a4a2e email ip=4 flag=stable port=443 transport=obfs3,obfs2
11b3a235c05843fbc57f29a848dee6dc6a6be118 unallocated

Note the duplicate entry for 112f02b648f19502831103fa50715ce31f8a4a2e. The first line shouldn't be there, I guess.

Setting priority to major, because the currently produced bridge pool assignments won't be useful for analysis.

Are those entries adjacent? Or was this output produced with sort? I noticed that for each assignment period a line is written 3 times for each bridge (but the lines are not consecutive). That suggests that the bug might have something to do with the function BridgeSplitter.dumpAssignments() in Bridges.py:923 ... but it isn't clear to me why that occurs yet.

comment:9 Changed 5 years ago by karsten

Good point. Lines are not adjacent in the original assignments.log file, I think. The sanitizer in metrics-db sorts lines, and what I pasted above is from sanitized assignments.

comment:10 Changed 5 years ago by karsten

Still no progress? Again, we're losing data here. The July pool assignment archives will be mostly useless for analysis, and Onionoo serves useless data to bridge operators. Please fix ASAP. Thanks.

comment:11 Changed 5 years ago by asn

As a hint on what might be going on, I read some BridgeDB logs with sysrqb and we noticed that when dumpAssignments() is run, we get multiple "%s supports %d transports" messages for each bridge.

This probably means that the list self.bridges contains duplicate bridges.

comment:12 Changed 5 years ago by isis

I read through some of the settings in the running instance of BridgeDB's bridgedb.conf file. There it specifies:

$ cat bridgedb.conf | grep descriptors
# Files from which we read bridge descriptors, on start and on SIGHUP.
BRIDGE_FILES = [ "../from-authority/bridge-descriptors" ]

and in Main.py, in the global CONFIG variable which is used for testing, these are:

lib/bridgedb/Main.py:    BRIDGE_FILES = [ "./cached-descriptors", "./cached-descriptors.new" ],

In the from-authority directory I found all three of these files, all with recent modification times, and the running BridgeDB process was not called with the '--testing' option, so it should only be reading the 'bridge-descriptors' file.

I'm fairly sure that if I change BRIDGE_FILES in the running instance's config to ["./cached-descriptors", "./cached-descriptors.new"] that it'll fix this, but I am nervous to change things without someone else who knows BridgeDB really well being available to help (I don't know how maintenance and deployments are done yet).

comment:13 Changed 5 years ago by karsten

Are you sure you're seeing recent cached-descriptors[.new] files on ponticum, not cached-extrainfo[.new]?

comment:14 Changed 5 years ago by sysrqb

It appears that whatever is causing this was put into production on 2013/08/14. Does ponticum have any sort of log that will be able to tell us what changed that day? I'm still going through commits that were made to bridgedb prior to that date. Does the bridgedb logfile go back that far?

I determined the date by doing a lovely binary search through the bridge-pool-assignments on metrics and running

for i in ./*/*; do echo -n "$i: "; cat $i | awk '{ print $1; }' \
   | uniq -d | wc -l; done | grep -v 0$ | head

over the month. August 14 was the first day that contained duplicates.

./14/2012-08-14-05-00-11: 1
./14/2012-08-14-05-30-11: 1
./14/2012-08-14-06-00-11: 1
./14/2012-08-14-06-30-12: 1
./14/2012-08-14-07-00-12: 1
./14/2012-08-14-07-30-12: 1
./14/2012-08-14-08-00-04: 1
./14/2012-08-14-08-30-04: 1
./14/2012-08-14-09-00-05: 1
./14/2012-08-14-09-30-06: 1

This isn't an answer, but at least it gives us another avenue to attack it.

comment:15 Changed 5 years ago by sysrqb

I've written an ugly hack that should solve this problem temporarily until we're able to determine what is causing it. It's branch bug9264_temporary_fix on https://github.com/sysrqb/bridgedb.git

Did I mention this is a *temporary* hack?

comment:16 in reply to:  13 Changed 5 years ago by isis

Replying to karsten:

Are you sure you're seeing recent cached-descriptors[.new] files on ponticum, not cached-extrainfo[.new]?

The latter, oops, 5AM hacking #FTL.

comment:17 in reply to:  15 Changed 5 years ago by isis

Owner: set to isis
Status: needs_reviewaccepted

Replying to sysrqb:

I've written an ugly hack that should solve this problem temporarily until we're able to determine what is causing it. It's branch bug9264_temporary_fix on https://github.com/sysrqb/bridgedb.git

Did I mention this is a *temporary* hack?

Got it. Looks good, thanks, sysrqb! I merged this here into my master branch, to go into the next redeploy that I am about to do.

comment:18 Changed 5 years ago by isis

Status: acceptedneeds_information

Deployed from branch deploy/2013-08-09-isis-5332-9156-9264.

comment:19 Changed 5 years ago by karsten

Resolution: fixed
Status: needs_informationclosed

I can confirm that bridge pool assignment files now contain sane numbers of bridges with transports:

$ grep -c transport *
2013-08-12-00-04-05:474
2013-08-12-00-34-08:475
2013-08-12-01-03-51:475
2013-08-12-01-34-01:475
2013-08-12-02-04-01:490
2013-08-12-02-34-03:494
2013-08-12-03-04-09:493
2013-08-12-03-33-57:494
2013-08-12-04-03-56:493
2013-08-12-04-33-55:493
2013-08-12-05-03-53:492
2013-08-12-05-34-08:492
2013-08-12-06-03-57:490
2013-08-12-06-33-57:491
2013-08-12-07-03-51:490
2013-08-12-07-34-32:459
2013-08-12-08-05-22:465
2013-08-12-08-34-03:476
2013-08-12-09-03-52:491
2013-08-12-09-33-56:490
2013-08-12-10-03-38:490
2013-08-12-10-33-59:490
2013-08-12-11-05-10:510
2013-08-12-11-33-38:509

So, I *think* this concludes the ticket. Optimistically closing. If this was wrong, please re-open. Thanks everyone!

comment:20 Changed 5 years ago by asn

Resolution: fixed
Status: closedreopened

I'm not sure if this should be closed. We still don't know why there were duplicate bridges in that self.bridges list. There must be some weirdness somewhere -- we should either fix it, or document it.

I'll reopen this ticket till we do some more digging on what sysrqb's branch hotfixed. Otherwise, we will never get to it.

comment:21 in reply to:  20 Changed 5 years ago by sysrqb

Status: reopenedneeds_information

Replying to asn:

I'm not sure if this should be closed. We still don't know why there were duplicate bridges in that self.bridges list. There must be some weirdness somewhere -- we should either fix it, or document it.

We should definitely fix it...unless it is "impossible".

I'll reopen this ticket till we do some more digging on what sysrqb's branch hotfixed. Otherwise, we will never get to it.

Yup. We can also open a new ticket for "why does ponticum import duplicate bridges", but the history here may help, but the Summary isn't exactly truthful anymore.

Basically, we knew BridgeDB was importing a single bridge multiple times when it parsed the descriptors. This hotfix simply checks if the bridge we just parsed was already imported and skips it if we already know about it, thus preventing duplicates.

comment:22 Changed 4 years ago by sysrqb

possible breakthrough on this by isis. Overall, this issue appears to have become worse over the last few months but due to the workaround we implemented it wasn't noticable. It appears that tonga is including old bridge descriptors in the file it sends us, as many as 4 from last 24 hours it seems.

To get an idea, here are the number of bridges descriptors sent by tonga and unique bridges contained within that set. Also included is a slightly-nasty command to check this.

$ grep -c fingerprint bridge-descriptors
11588
$ grep fingerprint bridge-descriptors | sort -u | wc -l
4300

$ grep -A 1 ^published bridge-descriptors | sed -e 's/opt //g' -e 's/fingerprint/ fingerprint /g' | tr -d '\n' | sed 's/\-\-/\n/g' | sort -k 4 | less

So, next stop bridge auth.

comment:23 in reply to:  22 ; Changed 4 years ago by karsten

Replying to sysrqb:

So, next stop bridge auth.

This is expected behavior by the bridge authority. It simply sends you its local descriptor caches, and those may contain older descriptors. This isn't something to fix, it's supposed to be like that.

Here's how you'd filter out the latest descriptors of currently running bridges:

  • Go through the bridge network status and note down descriptor digests of entries with the Running flag.
  • Go through server descriptors by descriptor digest and note down the extra-info digests of corresponding extra-info descriptors.
  • Go through extra-info descriptors by extra-info digest and learn the information you wanted to learn.

Does BridgeDB already use stem? This is something where stem would be pretty helpful.

comment:24 in reply to:  23 Changed 4 years ago by sysrqb

Replying to karsten:

Replying to sysrqb:

So, next stop bridge auth.

This is expected behavior by the bridge authority. It simply sends you its local descriptor caches, and those may contain older descriptors. This isn't something to fix, it's supposed to be like that.

Here's how you'd filter out the latest descriptors of currently running bridges:

I missed the fact that you posted this. I was about to say exactly the same thing - I realize this while working on #10680. This should be fixed in the next release, if possible.

Does BridgeDB already use stem? This is something where stem would be pretty helpful.

Not yet, maybe someday soon but the bridge parsers that are scattered around need to be integrated into it before bridgedb can use it.

comment:25 Changed 4 years ago by sysrqb

Wrote a patch for this. branch bug9264_rebased_0 in my repo. It could use a lookover by another set of eyes.

[0] https://travis-ci.org/sysrqbci/bridgedb/builds/18092876

comment:26 Changed 4 years ago by isis

Status: needs_informationneeds_revision

This was already stated on IRC, but for the record:

Thanks for adding those unittests! :) Have you looked at sure? It's perfect for testing the parsers; I'vehttp://falcao.it/sure/reference.html used it in a couple test files already, i.e.:

this(port).should.be.ok
this(port).should.be.a(networkstatus.addr.PortList)

It's pretty terse and readable, plus the crazy syntax made writing tests more entertaining. (You don't have to use it, this is just a general "hey dude! check out this neat thing!" :D )

sysrqb, in your bug9264_rebased_0 patches, I think something broke Bridge.getConfigLine() or another related function. Testing the email server and HTTPS interface resulted in:

Feb 08 06:57:12 [DEBUG] fa25ed11b3a7cf918de82460959b81d50b40a4df supports 2 transports
Feb 08 06:57:12 [DEBUG] 590e93e84ac43a994392d402a7ca103ab2e2ad5c supports 2 transports
Feb 08 06:57:12 [DEBUG] 83158cbfb8e76f9f782c706cd88f0d519a270867 supports 2 transports
Feb 08 06:57:12 [DEBUG] Saving state to: 	'/home/isis/code/torproject/bridgedb/run-manual/bridgedb.state'
Feb 08 06:57:13 [DEBUG] Opening GPG keyfile gnupghome/TESTING.subkeys.sec...
Feb 08 06:57:13 [INFO] GPG Key with fingerprint 318A83A975C91BD4538C317C4A792354FA079716 imported
Feb 08 06:57:13 [INFO] Testing signature created with GnuPG key...
Feb 08 06:57:13 [INFO] Starting reactors.
Feb 08 07:03:02 [DEBUG] > Received: BridgeDB
Feb 08 07:03:02 [DEBUG] > From: isis@127.0.0.1
Feb 08 07:03:02 [DEBUG] > To: bridges@127.0.0.1
Feb 08 07:03:02 [DEBUG] > Subject: testing
Feb 08 07:03:02 [DEBUG] > 
Feb 08 07:03:02 [DEBUG] > get bridges
Feb 08 07:03:02 [INFO] Got a completed email; deciding whether to reply.
Feb 08 07:03:02 [INFO] Attempting to return for 3 bridges for isis@127.0.0.1...
Feb 08 07:03:02 [DEBUG] Cache hit frozenset([<function filterBridgesByIP4 at 0x17f58c0>])
Feb 08 07:03:02 [DEBUG] Returning 3 bridges from ring of len: 356
Feb 08 07:03:02 [DEBUG] Got duplicate bridge 'fa179ed482ff4096a2548db2fbdbc7046abe093d' in main hashring for position 'f8f6fd62d22b6d29e684b0149eff7ef7a7aaf177'.
Feb 08 07:03:02 [DEBUG] Email body:
Content-Type: text/plain
From: bridges@torproject.org
To: isis@127.0.0.1
Message-ID: <20140208070302.8823.222990448.0@wintermute.patternsinthevoid.net>
Subject: Re: testing
Date: Sat, 08 Feb 2014 07:03:02 +0000

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

[This is an automated message; please do not reply.]

Here are your bridge relays: 

  None
  None
  None

Bridge relays (or "bridges" for short) are Tor relays that aren't listed
in the main directory. Since there is no complete public list of them,
even if your ISP is filtering connections to all the known Tor relays,
they probably won't be able to block all the bridges.

To use the above lines, go to Vidalia's Network settings page, and click
"My ISP blocks connections to the Tor network". Then add each bridge
address one at a time.

Configuring more than one bridge address will make your Tor connection
more stable, in case some of the bridges become unreachable.

The following commands are also supported:

  ipv6 : request ipv6 bridges.
  transport NAME : request transport NAME. Example: 'transport obfs2'

Another way to find public bridge addresses is to visit
https://bridges.torproject.org/. The answers you get from that page
will change every few days, so check back periodically if you need more
bridge addresses.

-----BEGIN PGP SIGNATURE-----

iQMhBAEBCgELBQJS9damBYMB4TOAVhSAAAAAACUAKGlzaXMrc2lnbnN1YmtleUBw
YXR0ZXJuc2ludGhldm9pZC5uZXRDRDY2NkI2MTIwOTZFQjM5OUZGMzQ3RURFOUYy
NjY5RTREQjZEMDUySxSAAAAAABoAKGlzaXNAcGF0dGVybnNpbnRoZXZvaWQubmV0
MzE4QTgzQTk3NUM5MUJENDUzOEMzMTdDNEE3OTIzNTRGQTA3OTcxNi4aaHR0cHM6
Ly9ibG9nLnBhdHRlcm5zaW50aGV2b2lkLm5ldC9wb2xpY3kudHh0LJhodHRwczov
L2Jsb2cucGF0dGVybnNpbnRoZXZvaWQubmV0L2lzaXMudHh0AAoJEOnyZp5NttBS
CXcP/1DFrDbrkUqDk+fgJPiXcLGdOGUA1NlJ6PWZwCUi9dHl6e4lD0c9G2o8U/LX
L39/UIebh/ywr+xOu5dZ6SE7aIG36qrhVvyTPqWa8JT8epbAsK5PmyXAGN6/vjnZ
+CW0jFKmXPICdRAb3G5mBLlt7hzwpMoNYtuWsZT7B6Wz20fJoHDp5eRJKTxGoWKr
knVUMq0qURzB9SwGGJijM8OcosPVrNuNFQCcaEbgPxm1wd2gf+moHbSelQu2T+bs
LqPhbs5rzCIDxtW+foHxi7ieOBXozlQF605dgt+We7yRl2ttomZmaf0G1S/kYBMg
botAXiS+DjIiUd821qWUUZKRtTOMS3w6robtp+PIdQ+IFTg2yv51K3EEBFf4sSY7
alXbG+uJlPV+eIY2SdOdXwFbzfh3PbEw1XDE0Xj6PHkcPLqsHVEejrF3Ga3mjyM4
IV4Sj9jSySMnBg6qQiLxEdB0J1wjU0bKL5d8iJK0/CiinshPwcdKACp+urPS+EoB
U6sRrBmEEHF8Msmga/WbHEoVU2L8fSohseo9XO2VHom0ZNpdvY42QygixjA0xOR2
pf9w1Rvfd0ocMwRxRsJZ8Oh8P1B9LhxwFz96SvBxv7hsYmmX4QkektAYKQMOfCYK
dQ6egFqnK1D9vromVCFjzA5nMMAZwAlnJUxHa7ZMnMr+CpNV
=8j/+
-----END PGP SIGNATURE-----

Feb 08 07:03:02 [INFO] Sending reply to 'isis@127.0.0.1'

https://trac.torproject.org/projects/tor/raw-attachment/ticket/9264/9264-sysrqb_rebased_0-BridgeDB%20%C2%B7%202014-02-08%2007-05-19.png

Last edited 4 years ago by isis (previous) (diff)

comment:27 in reply to:  26 Changed 4 years ago by isis

Currently reviewing sysrqb's bug9264_rebased_1 branch.

A couple comments:

In 350341c991d3, in lib/bridgedb/Bridges.py, this line forgets to take into account that descDigest might be None:

@@ -540,6 +540,7 @@ def parseStatusFile(networkstatusFile):
                 logging.debug("  ORAddress:  {0}".format(ORaddr))
                 logging.debug("  ORport:     {0}".format(ORport))
                 logging.debug("  dirport:    {0}".format(dirport))
+            descDigest = toHex(descDigest)
 
         elif ID and line.startswith("a "):
             try:

(depending on how descriptor parsing went), which raises:

  File "/home/isis/.virtualenvs/bridgedb/local/lib/python2.7/site-packages/bridgedb-0.1.2_19_g59ec642-py2.7.egg/bridgedb/Bridges.py", line 649, in parseStatusFile
    descDigest = toHex(descDigest)
TypeError: must be string or buffer, not None

We could instead change the logic in networkstatus.parseRLine(); I prioritised pulling the bridge's ID digest from the descriptor so that we could at the very least try to find its corresponding extrainfo and relay descriptors to get more information, and therefore I decided that other information could be permitted to be returned as None if it was for some reason unparseable.

Though I do agree that using the NS info is better than using the server descriptor info. Any thoughts on how we should behave if the NS info doesn't make sense?


For 'a94787aef9e0c'... What do you think about moving the newer parsers/parsing functions out of lib/bridgedb/Bridges.py? It's a bit stuffy in there. :) (Also not necessary if you think they should live in Bridges.)

And did you decide to use StringIO for speed reasons? Or because we shouldn't chunk up the extrainfo descriptors twice?

This code looks really nice.


In eed51c457939d5d, these getters and setters might make nice @propertys:

+    def setVerified(self):
+        """Call when the bridge's descriptor is parsed"""
+        self.verified = True
+
+    def isVerified(self):
+        """Returns the truthiness of ``verified``"""
+        return self.verified
+

Hooray! Pretty documentation and unittests! :D


to be continued

Last edited 4 years ago by isis (previous) (diff)

comment:28 Changed 4 years ago by sysrqb

yup, I already pushed a fix to bug9264_rebased_1.

comment:29 Changed 4 years ago by isis

Keywords: bridgedb-0.1.3 added
Resolution: fixed
Status: needs_revisionclosed

I merged sysrqb's bug9264_rebased_1 branch into develop for version 0.1.3. The bug where the distributors didn't have bridges in their hashrings was caused by a commit to make fingerprint string capitalisation consistent; I reverted it.

I also merged the hotfix/9264-descdigest-none branch, which fixes a problem where, if the nickname was not to spec (longer than 19 characters), then the nickname and descriptor digest are both set to None and bridgedb.Bridges.parseStatusFile() attempts to format the null descriptor digest into hexadecimal.

@sysrqb: Thanks!

@karsten: let me know if this does/does't fix the assignments.log files!

comment:30 in reply to:  29 ; Changed 4 years ago by karsten

Resolution: fixed
Status: closedreopened

Replying to isis:

@karsten: let me know if this does/does't fix the assignments.log files!

I'm afraid it doesn't. The latest assignments bridge-pool-assignment 2014-02-22 21:02:20 contain up to 46 duplicate entries. :(

comment:31 in reply to:  30 Changed 4 years ago by isis

Replying to karsten:

Replying to isis:

@karsten: let me know if this does/does't fix the assignments.log files!

I'm afraid it doesn't. The latest assignments bridge-pool-assignment 2014-02-22 21:02:20 contain up to 46 duplicate entries. :(

Ugh. I suppose only 46 is good news, compared to the 16,000,000 duplicates we were seeing for a while. It seems that something is indeed quite wrong:

bridgedb@ponticum:/srv/bridges.torproject.org/log$ du -sh assignments.log-2014022{1,2,3,4,5,6,7,8}.gz
179M    assignments.log-20140221.gz
230M    assignments.log-20140222.gz
90M     assignments.log-20140223.gz
248M    assignments.log-20140224.gz
283M    assignments.log-20140225.gz
129M    assignments.log-20140226.gz
281M    assignments.log-20140227.gz
368M    assignments.log-20140228.gz

comment:32 Changed 4 years ago by sysrqb

I pushed bug9264_rebased_2 to my repo. The short description of the bug is that when we reparsed the descriptors we never checked if we already had the bridge, so we appended it to a list every time. With this patch we'll check if we already have a bridge with the same fingerprint and overwrite it if we do, otherwise we append as before.

Another option is to change the list to a dict, but O(n) complexity of scanning a list linearly should be fairly quick in this case.

I added a unit test for this which successfully tests this duplicate-bridge-insertion case, however travis-ci is choking some other parts of the tests.
https://travis-ci.org/sysrqbci/bridgedb/builds/20051728

comment:33 Changed 4 years ago by sysrqb

And I just pushed bug9264_rebased_3 which is based on develop. bug9264_rebased_2 was accidently based on master.

https://travis-ci.org/sysrqbci/bridgedb/builds/20054603

comment:34 Changed 4 years ago by isis

Keywords: bridgedb-0.1.5 added
Status: reopenedneeds_revision

Reviewing. I also merged #11127 and #10809's fix/11127-recaptcha-ssl_10809r1_r1 branch into it, in order to test your changes on top of those other ones.

Comments:

  1. In 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.


  1. In 13fc557b8e4bd094457ca954129dceaa4445d744, the test_splitterBridgeInsertion is great!


  1. 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()? 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?


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).


  1. 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.

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

comment:35 in reply to:  34 Changed 4 years ago by sysrqb

Status: needs_revisionneeds_review

Replying to isis:

Comments:

  1. In 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).

  1. In 13fc557b8e4bd094457ca954129dceaa4445d744, the test_splitterBridgeInsertion is great!


  1. 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.

  1. 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 bug9264_rebased_3_r1, which I think solve the above comments.

comment:36 Changed 4 years ago by isis

Resolution: fixed
Status: needs_reviewclosed

Reviewed and merged bug9264_rebased_3_r1 into develop.

Note: See TracTickets for help on using tickets.