#28598 closed defect (fixed)

Should torflow scaling use the consensus bandwidth when it is measured?

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

Description

sbws is not using the consensus bandwidth, but it seems that torflow used the consensus bandwidth instead of the descriptors bandwidth, when it has the measured flag (https://gitweb.torproject.org/pytorctl.git/tree/TorCtl.py#n376).

Ratio might need some change too (https://gitweb.torproject.org/pytorctl.git/tree/TorCtl.py#n1636).

Child Tickets

Change History (15)

comment:1 Changed 10 months ago by juga

I can't remember if there was any reasons why we didn't use the consensus bandwidth, it was even in the spec (https://gitweb.torproject.org/torspec.git/tree/bandwidth-file-spec.txt#n335).
Maybe because we don't do that calculation at the time of measuring?.

comment:2 in reply to:  description ; Changed 10 months ago by teor

Replying to juga:

sbws is not using the consensus bandwidth, but it seems that torflow used the consensus bandwidth instead of the descriptors bandwidth, when it has the measured flag (https://gitweb.torproject.org/pytorctl.git/tree/TorCtl.py#n376).

Using the consensus bandwidth ignores the RelayBandwidthRate / MaxAdvertisedBandwidth, which is a bug in torflow.

Here's what I suggest:

  • when the relay is unmeasured, use the minimum of all descriptor bandwidths (like torflow)
  • when the relay is meansured, use the minimum of the measured bandwidth, and the RelayBandwidthRate/MaxAdvertisedBandwidth in the descriptor

Ratio might need some change too (https://gitweb.torproject.org/pytorctl.git/tree/TorCtl.py#n1636).

Yes, let's change the ratio to 1.0 for unmeasured relays. It's more secure that way.

comment:3 Changed 10 months ago by teor

Parent ID: #28588

comment:4 Changed 10 months ago by teor

Keywords: sbws-1.0-must-moved-20181128 added
Milestone: sbws 1.0 (MVP must)sbws 1.0.3

Moving all sbws 1.0 must bugs to 1.0.3.

comment:5 Changed 10 months ago by teor

Milestone: sbws 1.0.3sbws 1.0.x

Milestone renamed

comment:6 Changed 10 months ago by teor

Milestone: sbws 1.0.xsbws: 1.0.x

Milestone renamed

comment:7 Changed 10 months ago by teor

Milestone: sbws: 1.0.xsbws: 1.0.x-final

Milestone renamed

comment:8 Changed 10 months ago by juga

Status: newneeds_review

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

Replying to teor:

Ratio might need some change too (https://gitweb.torproject.org/pytorctl.git/tree/TorCtl.py#n1636).

Yes, let's change the ratio to 1.0 for unmeasured relays. It's more secure that way.

I realized that's a ratio to order relays, not the ratio used for torflow's scaling.

comment:10 Changed 10 months ago by teor

Reviewer: teor
Status: needs_reviewneeds_revision

I did a review on the pull request.

comment:11 Changed 10 months ago by teor

I did some more review on the pull request.

We can't leave the names for #28684, because:

  • #28684 might take a long time, and
  • this patch adds keys to the bandwidth file. Changing keys in the bandwidth file is a breaking change.

Here's what I think:

In sbws, "bw" means "The measured bandwidth of this relay." (for the next vote).
https://gitweb.torproject.org/torspec.git/tree/bandwidth-file-spec.txt
So we can't use "bandwidth" like stem does.

Here's what I suggest:

  • cons_bw: the bandwidth of this relay in the consensus
  • cons_bw_is_unmeasured: is this relay is unmeasured in the consensus?

comment:12 in reply to:  11 Changed 10 months ago by juga

Replying to teor:

Here's what I suggest:

  • cons_bw: the bandwidth of this relay in the consensus
  • cons_bw_is_unmeasured: is this relay is unmeasured in the consensus?

In the end i've used consensus_bandwidth and consensus_bandwidth_is_unmeasured.

comment:13 Changed 10 months ago by juga

Status: needs_revisionneeds_review

comment:14 Changed 10 months ago by teor

Status: needs_reviewmerge_ready

Thanks, that code is much more readable.

comment:15 Changed 10 months ago by juga

Resolution: fixed
Status: merge_readyclosed

Squashed in https://github.com/juga0/sbws/commits/bug28598_squashed.
Added one more commit to be able to parse old results. Didn't put it in needs_review again to don't create more work.

Note: See TracTickets for help on using tickets.