Rewards sanity check

[cyberphysic4l]

Adds a sanity check that token supply bits count + poolCoefficientExponent is less that 64 to fit in the int64 range during rewards calculations.

[PhilippGackstatter]

Small improvement to use integers for the comparison rather than floats and print the actual values. We do this in the congestion control sanity check and it was already quite helpful when I set custom parameters..

Also would you mind adding another sanity check to ensure ValidationBlocksPerSlot <= 32? We currently have that assumption but it's not checked anywhere afaik, so in the protocol params would be good.

Edit: I think it's <= 32 because ValidationBlocksPerSlot equals the number of subslots and a uint32 can track 32 bits (=subslots) so it's fine to be equal to 32.

[PhilippGackstatter]

Suggested change

	if math.Log2(float64(protocolParams.TokenSupply()))+float64(protocolParams.RewardsParameters().PoolCoefficientExponent) > 63 {
	panic("Token supply bits count + poolCoefficientExponent must be less than or equal to 63")
	}
	tokenSupplyBitsCount := uint8(math.Log2(float64(protocolParams.TokenSupply())))
	if tokenSupplyBitsCount + protocolParams.RewardsParameters().PoolCoefficientExponent > 63 {
	message := fmt.Sprintf("Token supply bits count (%d) + PoolCoefficientExponent (%d) must be less than or equal to 63\n", tokenSupplyBitsCount, protocolParams.RewardsParameters().PoolCoefficientExponent)
	panic(message)
	}

[PhilippGackstatter]

Maybe @oliviasaa can comment on whether it should be > 63 or >= 63. I'm not sure right now.

Also I guess if we truncate the float result to integer it would have to be uint8(log2(tokenSupply)) + 1 in case there is a decimal place?

[cyberphysic4l]

If you use uint8 in this way, then you also need to check for overflows incase someone were to set PoolCoefficentExponent = 255 for example, then the sanity check would pass but this would obviously not be a sane value.

[cyberphysic4l]

I am actually also sceptical about one of the other assumptions in the comments/TIP, which is that the target reward is at most 42 bits. Where does this assumption come from and does it also need to be sanity checked?

[cyberphysic4l]

Regarding the >63 or >=63 point, I actually think the check can be >64 if, as you suggested, we use the actual bit count used by the token supply which is given by uint8(log2(tokenSupply))+1. So the pool stake takes up tokenSupplyBits, and that gets shifted left by poolCoefficientExponent, so that can be up to 64 bits without issue. Still happy to wait for @oliviasaa to check though.

[oliviasaa]

<64 :)

[oliviasaa]

and I agree with Andrew that using floats for the tests is simpler, because of a potential overflowing

[oliviasaa]

that the target reward is at most 42 bits

this was in the initial calculations, which used the iota supply, but you can just check if Computed Initial Rewards uses this amount of bits

[cyberphysic4l]

this was in the initial calculations, which used the iota supply, but you can just check if Computed Initial Rewards uses this amount of bits

@oliviasaa are you saying that the reference to 42 bits is outdated and should be updated to something else? Do we need another check that Computed Initial Rewards uses some amount of bits? The tip should be updated and so should the comment in the code to make this understandable either way.

[PhilippGackstatter]

Looks good to me, thanks for the Validation Blocks check!

[PhilippGackstatter]

Let's get this merged to fix iotaledger/iota-core#614 and address potential other checks in future PRs.