[tor-bugs] #31483 [Core Tor/Tor]: token_bucket_ctr_adjust() does not convert rate to step

[Tor Bug Tracker & Wiki]

#31483: token_bucket_ctr_adjust() does not convert rate to step
--------------------------+------------------------------------
 Reporter:  teor          |          Owner:  nickm
     Type:  defect        |         Status:  accepted
 Priority:  Very High     |      Milestone:  Tor: 0.4.2.x-final
Component:  Core Tor/Tor  |        Version:  Tor: unspecified
 Severity:  Normal        |     Resolution:
 Keywords:  042-must      |  Actual Points:
Parent ID:                |         Points:  0.5
 Reviewer:  asn           |        Sponsor:  Sponsor31-must
--------------------------+------------------------------------

Comment (by nickm):

 So, I don't think we have a real misbehavior here, but we do have a small
 piece of ugliness.

 The reason that I think we don't have a bug is that
 token_bucket_ctr_refill() doesn't use TICKS_PER_STEP, so it isn't
 necessary to convert the rate in "units per tick" into "units per step"
 when using a tocken_bucket_ctr_t.  So that computation is correct.

 To make matters more complicated, it's not clear to me that we want to
 harmonize both systems to use the same units. Right now, both
 "token_bucket_rw_t" and "token_bucket_ctr_t" are built to take their time
 in abstract ticks, and to track a resource measured in abstract units.
 For token_bucket_rw_t as it is used, the units are 1 byte, and the ticks
 are about 1 ms.  To avoid rounding errors, we consider time as in "steps"
 of 16 ticks.

 But for token_bucket_ctr_t as it is used for onion service DOS defenses,
 the units are 1 introduction cell, and the ticks are 1 second.  25 cells
 per second is the default rate.  There's no reason to group these ticks
 into a 16-tick step.  On the other hand, if we moved to a tick size that
 was smaller than 1 second, we'd have rounding errors with the cells-per-
 second calculation.

 I think that if we want to harmonize these two types, we should do it by
 having them both use the same step/tick logic, and by fixing ticks around
 1 ms.  We should also say that units have to be chosen so that they have a
 meaningful resolution with respect to the tick size: instead of introduce
 cells per tick, we would have to use units something like "hundredths of
 an introduce cell per tick".

 How much of this should we do in 0.4.2?

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