bug: TokenBucket update calculates wrong with lastUpdate timestamp

[NagyZoltanPeter]

I suspect a bug with the TokenBucket update calculation.
I think due to the lastUpdate field is updated wrong thus leads false calculation in update method and result in refilling the budget even if within time frame not elapsed.

Given the fact that lastUpdate is initialized at creation of TokenBucket instance and it can be far in time from the first consume event. Through the calculations of fill percentage will give wrong result if we compare current update time to the time of lastUpdate.
In my understanding lastUpdate shall be refreshed at the time when the budget is full and the first tokens are to be consumed.

TokenBucket.init (3, 500.millis)-> 5 sec -> consume, consume, consume (within 50ms), consume
last consume should fail but seems update refills the budget and sets lastUpdated ahead with 5sec.

nim-chronos/chronos/ratelimit.nim

Line 40 in 672db13

timeDelta = currentTime - bucket.lastUpdate

Some background, I started using TokenBucket as a request rate limiter in Waku's non relay protocols.
It should add cap on request count inside a certain amount of time.
So as I would use it is the within a certain time period max budgetCap number of request is allowed, when the period is over it shall refill the budget.

Now I'm not pretty sure I expect something different from TokenBucket as it is for or it is a bug.
Please let me know how do you see!

Update:
I checked the tests and found this one suspicious:

nim-chronos/tests/testratelimit.nim

Line 119 in 672db13

test "Short replenish":

I think it can be reproduced what's happening in CI if you add a 1ms wait after creating the TokenBucket

nim-chronos/tests/testratelimit.nim

Line 125 in 672db13

# var bucket = TokenBucket.new(15000, 1.milliseconds)

Than the replenish calculation will go wrong as descibed above, similar to my case.

CC: @arnetheduck, @cheatfate

[arnetheduck]

Given the fact that lastUpdate is initialized at creation of TokenBucket instance

indeed, the bucket could start empty and with a 0 timestamp - this situation should clear up after the first use though so I don't think it affects the "average".

Looking at the code, some other small issues stand out:

• partially full buffers don't contribute to tryConsume but get overwritten on replenish
• queued consumptions look a bit wonky for the same reason, ie the logic for what happens to lastUpdate / budget after a queued consume has been satisfied could be reviewed