[tor-bugs] #13125 [Tor]: Review the guardiness python script of #9321

[Tor Bug Tracker & Wiki]

#13125: Review the guardiness python script of #9321
------------------------+--------------------------------
     Reporter:  asn     |      Owner:
         Type:  defect  |     Status:  needs_review
     Priority:  normal  |  Milestone:  Tor: 0.2.7.x-final
    Component:  Tor     |    Version:
   Resolution:          |   Keywords:  tor-guard tor-auth
Actual Points:          |  Parent ID:  #9321
       Points:          |
------------------------+--------------------------------

Comment (by asn):

 Replying to [comment:6 NickHopper]:

 Thanks for the review!

 I pushed some new code, addressing most of your comments and some comments
 of Sebastian.

 The most important change is that now the guardfraction script doesn't
 delete old consensuses from the database. Instead it just ignores them by
 default. I added a new CLI switch to delete them.

 Also, I started using the sqlite datetime() functions which are a bit
 smarter about days of the month etc.


 > === consensus.py: ===
 > line 17: Are we sure all dirauths will run the parser at the same time
 offset? If some dirauths run just before the hour and some just after the
 hour there could be off-by-one differences even if the dirauths all have
 essentially the same view of the history.
 >

 You have a point here, but unfortunately I didn't find a way to round down
 the datetime() of sqlite down to the hour. I will look more into this.

 That said, I'm going to give a cron job to dirauth operators that they are
 supposed to use. The cron job will run at around 30 minutes in the hour,
 every hour. So that should not be that big of a problem.

 > line 34: probably best to use stem.Flag.GUARD instead of 'Guard'
 >

 Done.

 > line 56: I think your schema should do this, since you require the
 consensus dates to be unique
 >

 Done.

 > === guards_ds.py ===
 > write_output_file:
 > I think what you're trying to do here, by appending everything to f_str
 before writing is like an atomic write?  But I think file.write() could
 still fail, and in the meantime you've truncated the output file by
 opening it w+.  If you want to do an atomic replacement, you need to do a
 two-phase commit, e.g. write to a tempfile and then rename it after the
 write is complete.
 >

 Hm, I was not actually trying to do an atomic write. That style remained
 from when I started developing the script, and instead of writing to a
 file I logged in stdout to check that the output is correct.
 If you think I should try to do an atomic write there, tell me.

 > === guard_fraction.py: ===
 > lines 92-94: same comment as consensus.py; it would seem prudent to
 round to the nearest hour so that we don't get off-by-one errors with
 different dirauths having a database that is off by one consensus.

 Same reply as above.

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