Opened 9 months ago

Closed 9 months ago

Last modified 8 months ago

#29707 closed defect (fixed)

sbws does not scale consensus bandwidths to bytes before using them

Reported by: teor Owned by:
Priority: Medium Milestone: sbws: 1.0.x-final
Component: Core Tor/sbws Version:
Severity: Normal Keywords:
Cc: juga Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

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

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
  • 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?
  • all the bandwidths in this file should be in the same units?
  • sbws should work in bytes, until it formats the file?

Child Tickets

Change History (7)

comment:1 Changed 9 months ago by juga

Status: newneeds_review

Replying to 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.

comment:2 Changed 9 months ago by teor

Status: needs_reviewmerge_ready

Thanks, looks good!

I opened #29712 to use 1024 to convert kilobytes to bytes.
but the difference between 1000 and 1024 is not very important, because bandwidth measurements are only 20-59% accurate anyway.

comment:3 in reply to:  2 Changed 9 months ago by juga

Replying to teor:

Thanks, looks good!

I opened #29712 to use 1024 to convert kilobytes to bytes.

I'm quite sure Tor reports consensus bandwidth in kilobytes, not kibibytes: https://github.com/torproject/tor/blob/master/src/feature/dirauth/bwauth.c#L191, https://github.com/torproject/torspec/blob/master/dir-spec.txt#L2387

but the difference between 1000 and 1024 is not very important, because bandwidth measurements are only 20-59% accurate anyway.

comment:4 Changed 9 months ago by juga

I also created #29715, because stem says it is unit-less

comment:5 in reply to:  1 Changed 9 months ago by juga

Replying to juga:

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.

Created #29717.

comment:6 Changed 9 months ago by juga

Resolution: fixed
Status: merge_readyclosed

Merged.

comment:7 Changed 8 months ago by teor

Milestone: sbws: 1.0.x-final

Moving closed sbws tickets to sbws: 1.0.x-final.

Note: See TracTickets for help on using tickets.