sbws does not scale consensus bandwidths to bytes before using them

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?

changed milestone to %sbws: 1.0.x-final in legacy/trac

By teor on 2020-06-13T09:19:58 (imported from GitLab project)

added component::core tor/sbws milestone::sbws: 1.0.x-final priority::medium resolution::fixed severity::normal status::closed type::defect labels

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 514671ef and removed in 79693446 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 legacy/trac#28282 (moved)

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

Trac:
Status: new to needs_review

By juga on 2019-03-09T13:06:47 (imported from GitLab project)

Thanks, looks good!

I opened legacy/trac#29712 (moved) 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.

Trac:
Status: needs_review to merge_ready

By teor on 2019-03-09T21:50:30 (imported from GitLab project)

Replying to teor:

Thanks, looks good!

I opened legacy/trac#29712 (moved) 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.

By juga on 2019-03-10T06:06:14 (imported from GitLab project)

I also created legacy/trac#29715 (closed), because stem says it is unit-less

By juga on 2019-03-10T06:28:07 (imported from GitLab project)

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 legacy/trac#29717 (moved).

By juga on 2019-03-10T06:48:08 (imported from GitLab project)

Merged.

Trac:
Status: merge_ready to closed
Resolution: N/A to fixed

By juga on 2019-03-11T07:41:37 (imported from GitLab project)

closed

By Trac on 2020-06-13T09:19:59 (imported from GitLab project)

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

Trac:
Milestone: N/A to sbws: 1.0.x-final

By teor on 2019-04-15T22:28:37 (imported from GitLab project)

mentioned in issue legacy/trac#29750 (moved)

By juga on 2020-06-13T09:20:56 (imported from GitLab project)

moved from legacy/trac#29707 (moved)

By Trac on 2020-06-13T16:15:32 (imported from GitLab project)

added Bug label and removed 1 deleted label

removed 1 deleted label