Opened 4 weeks ago

Closed 3 weeks ago

Last modified 4 days ago

#29853 closed defect (fixed)

sbws should show relays for diagnostics, even when MIN_REPORT has not been reached

Reported by: teor Owned by: juga
Priority: Medium Milestone: sbws: 1.1.x-final
Component: Core Tor/sbws Version:
Severity: Normal Keywords: tor-bwauth, sbws-1.0-must-moved-20181128, sbws-11x-final-removed-20190312, sbws-110-proposed, changes-version-minor
Cc: juga Actual Points:
Parent ID: #28563 Points:
Reviewer: nickm Sponsor:

Description

Hi juga,

There's a serious bug in #28563: sbws is not reporting any relays until MIN_REPORT has been reached.

But we want sbws to always report every relay for diagnostics. If the bandwidth file doesn't have minimum_number_eligible_relays, they should all be marked as "under_min_report=1 vote=0".

https://github.com/torproject/sbws/blob/6070d36022dc2130518dd0c68332166b2bf76c73/sbws/lib/v3bwfile.py#L1352

Child Tickets

Change History (14)

comment:2 Changed 4 weeks ago by juga

Status: assignedaccepted

Sorry, i forgot about this case, will work on it soon.

comment:3 Changed 4 weeks ago by juga

Status: acceptedneeds_review

comment:4 Changed 4 weeks ago by juga

Added a last commit to include the bandwidth file generated by the testnet that could be checked in #29727

comment:5 Changed 4 weeks ago by asn

Reviewer: nickm

comment:6 Changed 4 weeks ago by nickm

Status: needs_reviewmerge_ready

LGTM. Before merging, a question: -- the last time that we marked anything as vote=0, we also marked it as unmeasued=1, bw=1. Do we need to do that in this case?

comment:7 in reply to:  6 ; Changed 4 weeks ago by teor

Status: merge_readyneeds_revision

Replying to nickm:

LGTM. Before merging, a question: -- the last time that we marked anything as vote=0, we also marked it as unmeasued=1, bw=1. Do we need to do that in this case?

But the relays are measured. There just aren't enough of them to have a reasonable set of weighted bandwidths across the network.

So the spec says vote=0 under_min_report=1 bw=1 real_bw=N:
https://github.com/torproject/torspec/pull/68/commits/bce5edc7382bfe229f9b56124574b97df1939c1b
Which means that authorities without #29806 will vote bandwidth 1 for these relays.

But we might want legacy authorities to vote bandwidth=(the measured bandwidth) or bandwidth=(the reported bandwidth) for these relays.

If 3 or more bandwidth authorities are already voting measured bandwidths for most relays, it doesn't matter what we choose. Even if we choose bw=1, the unmeasured relays just have a slightly lower median in their vote.

But if we're just re-starting the bandwidth authorities after many of them have failed, here's what happens when we vote bw=

  • 1: measured relays get a consensus weight of 1 once 3 authorities know about them
    • then there is a big change in weight once 2 authorities have measured 60% of relays, and start voting measured bandwidths
  • the reported bandwidth: measured relays get a consensus weight of their reported bandwidth once 3 authorities have measured them
    • then there is a smaller change to the measured weight once 2 authorities have measured 60% of relays, and start voting measured bandwidths
    • relays can lie about their bandwidth more easily
  • the measured bandwidth:measured relays get a consensus weight of their measured bandwidth once 3 authorities have measured them
    • there is no change once 2 authorities have measured 60% of relays, and start voting measured bandwidths

So I think we should put measured bandwidths in the bandwidth file in this case.
(And we don't need real_bw.)
I'll do a draft spec update,
What do you think, juga, nickm?

comment:8 in reply to:  7 Changed 4 weeks ago by juga

Replying to teor:
[...]

So I think we should put measured bandwidths in the bandwidth file in this case.
(And we don't need real_bw.)
I'll do a draft spec update,
What do you think, juga, nickm?

Yeah, i think it's makes more sense to start voting with the actual measured (and filtered and scaled) bandwidth.

comment:9 Changed 4 weeks ago by juga

Status: needs_revisionneeds_review

nickm said by irc that also agrees with teor's comment in https://trac.torproject.org/projects/tor/ticket/29853#comment:7.

2 fixups to change that, 1 fixup on i mistake i made, 1 fixup to update the test.
Waited for CI which passes.

comment:10 Changed 4 weeks ago by nickm

I don't understand why the changes here didn't cause/require changes in the test files?

comment:11 in reply to:  10 Changed 3 weeks ago by juga

Replying to nickm:

I don't understand why the changes here didn't cause/require changes in the test files?

Yeah, sorry, this code is quite confusing, it really needs refactor/renaming and better comments.

So, we have the following cases:

  • case 0: the number of eligible relays is over the 60% of the consensus, nothing is set.
  • case 1: after filtering, there are not eligible relays, they will be set "vote=0 under_min_report = 1" and they will also have "unmeasured=1 bw=1" because they are not eligible.
  • case 2: after scaling, the number of relays is less than 60% in the consensus, they will be set "vote=0 under_min_report =1", the won't have "unmeasured=1", but some of them may have "bw=1" just because that's the bandwdith after scaling

I added a test to check the 3 cases.

The changes in this branch, *did* require changes in other tests, in concrete:

  • test_torflow_scale: i mocked consensus to pretend the network has only 1 relay so that the filtered relays are over the 60% and the scaling expected results continue working. Nothing regarding under_min_report changes here.
  • test_from_results_read i hardcoded under_min_report because update_progress would be None and if not min_perc will set under_min_report

comment:12 Changed 3 weeks ago by nickm

Status: needs_reviewmerge_ready

Ah. That makes sense. It looks okay to me now.

comment:13 Changed 3 weeks ago by juga

Resolution: fixed
Status: merge_readyclosed

squashed rebased and force pushed, then merged.

comment:14 Changed 4 days ago by teor

Milestone: sbws: 1.1.0sbws: 1.1.x-final

Move sbws 1.1.0 tickets into 1.1.x-final, to match Tor's milestone scheme.

Note: See TracTickets for help on using tickets.