[tor-bugs] #29707 [Core Tor/sbws]: sbws does not scale consensus bandwidths to bytes before using them

[Tor Bug Tracker & Wiki]

#29707: sbws does not scale consensus bandwidths to bytes before using them
---------------------------+------------------------------
 Reporter:  teor           |          Owner:  (none)
     Type:  defect         |         Status:  needs_review
 Priority:  Medium         |      Milestone:
Component:  Core Tor/sbws  |        Version:
 Severity:  Normal         |     Resolution:
 Keywords:                 |  Actual Points:
Parent ID:                 |         Points:
 Reviewer:                 |        Sponsor:
---------------------------+------------------------------
Changes (by juga):

 * status:  new => needs_review


Comment:

 Replying to [ticket:29707 teor]:
 > In this line, desc_bw is in bytes, but consensus_bandwidth is in
 kilobytes:
 > {{{
 > min_bandwidth = min(desc_bw, l.consensus_bandwidth)
 > }}}
 > https://github.com/torproject/sbws/blob/master/sbws/lib/v3bwfile.py#L977
 >

 PR just solving the bug (without tests):
 https://github.com/torproject/sbws/pull/346

 > Here's how we could avoid bugs like this in future:
 > * We should add some unit tests that only work when this bug is fixed

 This is hard to do rigth now in unit test because RelayList has controller
 has mandatory argument. [0]
 I started to mock it to create a Relay instance, then realized that
 creating stem's RouterStatusEntryV3 and RelayDescriptor accept different
 formats for fingerprint and signing key, so have to do several
 conversions.

 And leaving the raw data in bytes makes no difference for the V3BWFile
 tests.

 > * We should work out how to check that our test bandwidth scanners are
 producing numbers that are the right size
 >
 > Here are some things we might want to do. But they might not be worth
 the time:
 > * all the variables should be labelled with units?

 I started to do that in 514671efb4ccc4f9b63dad0f5373891f7f27b990 and
 removed in 79693446f53794d3ddab0681333640e3e50da7b0 because i thought `bs`
 was not a clear name, but i could have named it `bytes`.

 > * all the bandwidths in this file should be in the same units?

 Do you mean in v3bwfile?, that's what i thought was the case :/

 > * sbws should work in bytes, until it formats the file?

 This sounds good. It was also my idea to do refactoring to separate the
 scaling logic from generating the text. Part of #28282

 [0] If descriptors are passed, it doesn't need the controller.
 It has been my idea to change that as a refactor, but no need to wait for
 a big refactor, since it's enough to make that argument optional.
 That would also allow having real cached-descriptors and cached-consensus
 for the unit tests.
 I'll create a ticket for this.

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