casper
casper copied to clipboard
Unnecessary assertion in `initialize_epoch`
There is an assertion in initialize_epoch
when updating reward_factor
that I'm pretty sure should not exist. It should either be a conditional assignment or should entirely be removed.
https://github.com/ethereum/casper/blob/dcf4caf35937800b04c6bac4a2ab13c1513b6147/casper/contracts/simple_casper.v.py#L388
Such an assertion that is out of the control of the caller via method params should not exist in this protocol level method. If this method fails, then the protocol will not be able to continue functioning.
It appears to me that reward_factor
can never be negative because of the logic to assign it:
adj_interest_base: decimal = self.BASE_INTEREST_FACTOR / self.sqrt_of_total_deposits()
self.reward_factor = adj_interest_base + self.BASE_PENALTY_FACTOR * (self.esf() - 2)
Neither adj_interest_base
nor self.BASE_PENALTY_FACTOR * (self.esf() - 2)
will never be negative. self.BASE_PENALTY_FACTOR * (self.esf() - 2)
can equal zero, but self.BASE_INTEREST_FACTOR / self.sqrt_of_total_deposits()
will always be greater than zero except in extreme cases where the denominator is extremely large (more ether than exists). So if these are the case, then reward_factor
will always be larger than zero.
If we are actually concerned with this value equalling zero in some cases, then we should have logic to recover gracefully rather than throwing with an assert. Something like the following:
if self.reward_factor == 0:
self.reward = DEFAULT_LOWEST_REWARD_FACTOR
removing that assert make perfect sense. Currently, the assertion is not giving useful information but introducing complexity when tracing bug.
Would also suggest assigning a minimum value for sqrt_of_total_deposits
, to prevent self.BASE_INTEREST_FACTOR / self.sqrt_of_total_deposits()
being too large.
I'm not sure what a reasonable lower bound on the sqrt_of_total_deposits
would be. Can you expand upon your comment @ChihChengLiang ?