casper icon indicating copy to clipboard operation
casper copied to clipboard

Unnecessary assertion in `initialize_epoch`

Open djrtwo opened this issue 6 years ago • 2 comments

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

djrtwo avatar May 10 '18 13:05 djrtwo

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.

ChihChengLiang avatar May 13 '18 08:05 ChihChengLiang

I'm not sure what a reasonable lower bound on the sqrt_of_total_deposits would be. Can you expand upon your comment @ChihChengLiang ?

djrtwo avatar Jun 06 '18 16:06 djrtwo