Re: [tor-bugs] #8494 [Core Tor/Tor]: Does MaxAdvertisedBandwidth do anything useful and if not, can we deprecate it?

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#8494: Does MaxAdvertisedBandwidth do anything useful and if not, can we deprecate
it?
-------------------------------------------------+-------------------------
 Reporter:  alphawolf                            |          Owner:  juga
     Type:  defect                               |         Status:
                                                 |  assigned
 Priority:  Low                                  |      Milestone:  Tor:
                                                 |  0.3.5.x-final
Component:  Core Tor/Tor                         |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  tor-spec, consensus, bandwidth,      |  Actual Points:
  MaxAdvertisedBandwidth tor-relay tor-dirauth   |
  needs-insight tor-bwauth                       |
Parent ID:  #25925                               |         Points:
 Reviewer:                                       |        Sponsor:
-------------------------------------------------+-------------------------

Comment (by teor):

 Replying to [comment:16 juga]:
 > Replying to [comment:15 teor]:
 > > This spec tells us what a bandwidth generator needs to do to be
 compatible with the Tor network.
 > > Sometimes it is useful to give examples of how sbws or torflow
 implements a feature.
 > > But first we need to say what *any* generator must do. Then we can
 give examples.
 >
 > ACK
 >
 > >
 > > So let's start with a sentence like this:
 > >
 > >     If a relay sets MaxAdvertisedBandwidth, generators MUST give it a
 similar weight to relays with that bandwidth capacity.
 > > Replying to [comment:13 juga]:
 > > > Bandwidth terms are different in `dir-spec.txt` and the code.
 > > > If i'm not mistaken, what is called `bandwidthrate` [0] in the code
 when creating the descriptor, is called `bandwidth-avg` in `dir-spec.txt`
 [1].
 > > > And it is calculated as the min(BandwidthRate,
 MaxAdvertisedBandwidth, RelayBandwidthRate) [2].
 >
 > Is this correct or i'm mistaken?
 >

 bandwidth-avg is min(BandwidthRate, MaxAdvertisedBandwidth,
 RelayBandwidthRate, BandwidthBurst, RelayBandwidthBurst)

 > > > If this is correct, the paragraph to add in `bandwidth-file-
 spec.txt` could be:
 > > >
 > > >     sbws limits the relay's measured bandwidth to the bandwidth-avg
 advertised
 > >
 > > Please say "Bandwidth generators MUST limit the relay's weight based
 on", not "sbws limits the relay's measured bandwidth to".
 > >
 > > >     in the relay's descriptor, which is the minimum between
 BandwidthRate,
 > > >     MaxAdvertisedBandwidth and RelayBandwidthRate in the relay's
 configuration.
 > > >
 > > > Though probably the last sentence not needed.
 > >
 > > I think we need the last sentence, because we need to explain why
 bandwidth generators need to implement MaxAdvertisedBandwidth.
 > >
 > > Let's also say:
 > >
 > >     Relays limit their bandwidth when BandwdithRate or
 RelayBandwidthRate are set. These options reduce a relay's bandwidth
 capacity. But MaxAdvertisedBandwidth doesn't change the relay's bandwidth
 capacity. Instead, it asks the bandwidth generator to limit the weight of
 the relay as if the relay's measured bandwidth was min(measured bandwidth,
 MaxAdvertisedBandwidth).
 > >     Generators SHOULD NOT limit weights based on bandwidth-observed,
 because that penalises new relays.
 > >
 >
 > In all your suggestions, i would not use the word `weight` to do not
 confuse it with the `weight` generated in the consensus.

 When I say "weight", I mean "the weight generated in the consensus",
 rather than "the bandwidth measured for the relay".

 Some generators don't measure relay bandwidth. For example, peerflow
 aggregates a relay's observed bandwidths from other relays. That's why I
 said "consensus weight".

 But we are not making that distinction in the spec, so we can just say
 "measured bandwidth".

 > I also would not use the word `capacity` to do not confuse it with the
 `bandwidth-observed` in the descriptors that it's called
 `bandwidthcapacity` in the code and comes from relays' self-tests.

 When I said "capacity", I mean "the total amount of bandwidth the relay
 can handle". I didn't want to say "measured bandwidth", because that's
 "the extra amount of bandwidth the relay can handle". But we're not making
 that distinction in the spec either, so we can just say "measured
 bandwidth" here as well.

 >
 > > Then we can give an example of what sbws does:
 > >
 > >     sbws measures relay bandwidths, then caps the measured bandwidth
 using bandwidth-avg (MaxAdvertisedBandwidth).
 >
 > as mentioned above, isn't bandwidth-avg = min(MaxAdvertisedBandwidth,
 MaxAdvertisedBandwidth, RelayBandwidthRate)?

 No, see above.

 > > > If Torflow is using also `bandwidth-avg`, then it could also be
 added in `bandwidth-file-spec.txt`:
 > > >
 > > >     Torflow does not need to limit the relay's measured bandwidth
 since it
 > > >     partions relays to be measured by bandwidth-avg
 > > >
 > > > Is this correct?.
 > >
 > > No, Torflow partitions based on:
 > > * bandwidth-consensus, if available, or
 > > * min(bandwidth-avg, bandwidth-burst, bandwidth-observed), or
 > > * 1, if either bandwidth is zero.
 > >
 > > https://gitweb.torproject.org/pytorctl.git/tree/TorCtl.py#n376
 > > https://gitweb.torproject.org/pytorctl.git/tree/TorCtl.py#n459
 > >
 > > Torflow also calculates weights based on the descriptor bandwidth to
 measured bandwidth ratio.
 > >
 > > So let's say:
 > >
 > >     Torflow partitions relays based on their bandwidth. For unmeasured
 relays, Torflow uses the minimum of all descriptor bandwidths, including
 bandwidth-avg (MaxAdvertisedBandwidth)
 >
 > as mentioned above, isn't bandwidth-avg = min(MaxAdvertisedBandwidth,
 MaxAdvertisedBandwidth, RelayBandwidthRate)?

 See above.

 > > and bandwidth-observed. Then Torflow measures the relays in each
 partition against each other, which implicitly limits a relay's measured
 bandwidth to the bandwidths of similar relays.
 > >
 > >     Torflow also generates consensus weights based on the ratio
 between the measured bandwidth and the minimum of all descriptor
 bandwidths (at the time of the measurement). So when an operator reduces
 the MaxAdvertisedBandwidth for a relay, Torflow reduces that relay's
 measured bandwidth.
 > >
 >
 > So, sumarizing your 4 sentences suggestions with my comments, plus what
 you clarify about Torflow:

 Remember, the spec says *what* a bandwidth generator needs to do. For
 complicated tasks, we also say *why* and *how*.

 Because MaxAdvertisedBadwidth is complicated, I don't want to leave these
 details out.

 >     Bandwidth generators MUST limit the relays' measured bandwidth based
 on the descriptors' bandwidth-avg.

 I want to say "MaxAdvertisedBandwidth" in the first sentence, because I
 want to say *why* we need to take these extra steps to limit the consensus
 weight. Then I want to give the details of *how* MaxAdvertisedBandwidth
 works in descriptors right now. If we say "bandwidth-avg", we lose the
 connection between generator bandwidth limits and MaxAdvertisedBandwidth.

 Then I want to explain why MaxAdvertisedBandwidth needs to be implemented
 on bandwidth generators, rather than in Tor itself.

 >     Descriptors' bandwidth-avg is the minimum between BandwidthRate,
 MaxAdvertisedBandwidth and RelayBandwidthRate in the relays'
 configuration, when these options are set.

 I hunk we should rewrite this sentence to say:

     A relay's MaxAdvertisedBandwidth affects the bandwidth-avg in its
 descriptor. bandwidth-avg is the minimum of MaxAdvertisedBandwidth,
 BandwidthRate, RelayBandwidthRate, BandwidthBurst, and
 RelayBandwidthBurst.


 >     Generators SHOULD NOT limit measured bandwidths based on
 descriptors' bandwidth-observed, because that penalises new relays.

 I agree.

 >     When generators does not limit the relays' measured bandwidth on the
 descriptors' bandwidth-avg, they MUST give a similar measured bandwidth to
 the consensus bandwidth or the minimum between the descriptor's bandwidth-
 avg, bandwidth-burst or bandwidth-observed.

 This sentence allows the Torflow behaviour, which is buggy, because it
 penalises new relays. And it conflicts with the "SHOULD NOT" in the
 sentence before. if you delete this sentence, the spec will be less
 confusing.

 You can give the sbws and Torflow implementations as examples if you want.

 > Though i'm still not sure this is correct. I could just copy what you
 propose, but not sure about `capacity` and `weight` terms, as commented
 aboved.

 You're right, we're not being that specific.

 > As a different issue to solve at some point in the future, i think we
 should unify terms for bandwidth in Tor code and `dir-spec.txt` to make it
 less confusing. It'd be also helpful to add formulae or more detailed
 descriptions on how the different bandwidth terms are generated in `dir-
 spec.txt`

 Please open another ticket for changes to other specs.

 What do you think about these subsections?

 > >     No Zero Bandwidths
 > >     Bandwidth Aggregation
 > >     Bandwidth Scaling
 > >     MaxAdvertisedBandwidth

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

_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs