Potential rounding issues (and updating comments)

[kirk-baird]

What has been Changed

The comments look like they belong a few lines higher up.

What might need to be changed - Units: Seconds vs Nanoseconds

        periodBits := math.Log2(float64(period))
	if round > (math.MaxUint64 >> int(periodBits)) {
		return TimeOfRoundErrorValue
	}
	delta := (round - 1) * uint64(period.Seconds())

Here math.Log2(float64(period)) will convert period into a float64 which is the nanoseconds for that duration. When we do the calculations we operate over uint64(period.Seconds()). So there are two different units being compares.

The implications are that periodBits will be about 30 bits (log2(10^9)) higher, as it is in nanoseconds rather than if it were based off seconds.

As an example, using period = 10 seconds gives a total of periodBits = log2(1,500,000,000) = 33.8.
math.MaxUint64 >> int(periodBits) ~= 2^(64 - 33)
Then the if statement will throw an error if round > 2^(64 - 33).
At 10 seconds periods that's about 608 years (2^31 * 10 seconds) worth of rounds.

This actually protects against the next two issues so it may be worth leaving but at least adding a comment to explain why.

Potential Issue 1: Rounding overflow

If periodBits actually represented the amount of bits of period in seconds e.g.
periodBits := math.Log2(float64(int(period.Seconds()) + 1)) assuming period >= 0
Then int(periodBits) is potentially trauncating a bit.

Example, say period = 10 uses 4 bits.
periodBits := math.Log2(float64(int(period.Seconds()) + 1)) = log2(10 + 1) = 3.4
int(periodBits) = 3.

So set round = math.MaxUint64 >> 3 thus the if statement does not error
round > (math.MaxUint64 >> int(periodBits))

We then have 10 * ((math.MathUint64 >> 3) - 1) which overflows.

Potential Issue 2

Carrying on from above, the cast int64(delta), where delta is uint64 may overflow. Overflow occurs if the most significant bit set to 1 i.e. delta >= 2^63 in which case int64(delta) would cast a large positive number to a negative number and be successfully returned.

Solution

 periodBits := math.Log2(float64(period.Seconds()) + 1) // can't have a value < 1 else log is negative
	if round > (math.MaxUint64 >> (int(periodBits) + 2)) { // +1 for multiplication overflow, +1 for casting to int64
		return TimeOfRoundErrorValue
	}
	delta := (round - 1) * uint64(period.Seconds())

Working on the assumption here that period >= 0 which should be fine for drand since the period wouldn't not be set to negative. But maybe, for extra safety, add a check to ensure period >= 0.

Or alternatively as mentioned above leave it as is and add a comment to explain why we use the nanoseconds not seconds.

[nikkolasg]

Nice - thanks for the detailled explanation. I would definitely support moving to float64(period.Seconds()) as we are treating everything in seconds. Could you push the suggested change to this PR ?

[kirk-baird]

Nice - thanks for the detailled explanation. I would definitely support moving to float64(period.Seconds()) as we are treating everything in seconds. Could you push the suggested change to this PR ?

Sure things, I'll update this PR :)