builtin-actors icon indicating copy to clipboard operation
builtin-actors copied to clipboard

Cleanup baseline power check

Open LesnyRumcajs opened this issue 2 years ago • 4 comments

Baseline power checks come with a rounding error which is visible for epoch zero, like here (same in Rust here).

(copied from discussion with @ZenGround0 regarding reward actor invariant checks that were failing for prior_epoch == -1)

The issue is that BaselineInitialValue is not exactly equal BaselinePowerFromPrev(InitBaselinePower()) even though both are theoretically equal to Baseline(0).

Error is introduced because we use fixed point numbers InitBaselinePower() starts with initial value, divides by the exponent but then multiplying back precisely the same exponent does not give back the same number

I want to figure out exactly how error grows so we can have high confidence invariant that takes error into consideration Sketching this out -- st.BaselinePower = (init_val - epsilon) * x^(epoch) st.EffectiveBaselinePower = init_val*x^(effective_epoch) where effective_epoch <= epoch taking equality of epoch and effective_epoch as the worst case we have err = st.EffectiveBaselinePower - st.BaselinePower = init_valx^y - (init_val - epsilon)x^y err = epsilon*x^y

So we want

let epsilon = BASELINE_INITIAL_VALUE - baseline_power_from_prev(INIT_BASELINE_POWER)
let err = epsilon * exp(BASELINE_EXPONENT, st.epoch)

If you reimplement this in rust: https://github.com/filecoin-project/specs-actors/blob/0bcea9e3fa074b875135563cdaba6e786428c669/actors/util/math/exp.go#L6 we can do exp efficiently Now that I know what's going on I can confidently say it's a better use of our limited time for you to delete this baseline check entirely. If you can copy the above discussion into an issue for later so we can track this as a low priority cleanup that would be amazing. Instead of deleting a reasonable compromise hack is to compare st.EffectiveBaselinePower < baseline_power_from_prev(st.BaselinePower) under the assumption that errors are much smaller than per epoch updates. This won't work for very very large epochs but its more or less good enough forever especially with the above issue filed

Given the above, I'll go with the easy solution for now but it would be nice to improve the check with the suggestions above.

Related invariant check

LesnyRumcajs avatar Jun 10 '22 07:06 LesnyRumcajs

@ZenGround0 @LesnyRumcajs is this still relevant?

anorth avatar Aug 11 '22 23:08 anorth

@anorth I believe so: https://github.com/filecoin-project/builtin-actors/blob/master/actors/reward/src/testing.rs#L58

LesnyRumcajs avatar Aug 12 '22 08:08 LesnyRumcajs

@LesnyRumcajs and @ZenGround0 does this ticket track cleanup of https://github.com/filecoin-project/builtin-actors/blob/a600986f67ac5bd3f33edf544b3c50114045d03a/actors/reward/src/testing.rs#L60 from check_state_invariants only or is it something else?

sudo-shashank avatar Jan 18 '23 14:01 sudo-shashank

@sudo-shashank I believe it tracks only this particular piece of logic.

LesnyRumcajs avatar Jan 19 '23 07:01 LesnyRumcajs