CheckBalanceInvariants loads state inconsistently across methods

[austinabell]

Most checks load state before checking invariants, for example:

specs-actors/actors/builtin/miner/miner_actor.go

Lines 477 to 478 in 2a62795

	rt.StateReadonly(&st)
	err := st.CheckBalanceInvariants(rt.CurrentBalance())

Where others do not and use the state from the transaction, such as:

specs-actors/actors/builtin/miner/miner_actor.go

Line 1710 in 2a62795

err := st.CheckBalanceInvariants(rt.CurrentBalance())

specs-actors/actors/builtin/miner/miner_actor.go

Line 1731 in 2a62795

err := st.CheckBalanceInvariants(rt.CurrentBalance())

When it seems like it has the same side effects as the others. Maybe there is a reason for this inconsistency, so feel free to close issue if so, but opening in case there isn't.

[anorth]

Thanks for opening the question. There are some inconsistencies here and it would be nice to figure out a single way of doing it.

• The check is not done inside the top-level method's transaction because, sometimes, the actor sends a message with funds after the transaction (usually to burn them), and we of course want to check balance invariants for the final balance.
  • ConfirmSectorProofsValid does it in the transaction, for no good reason
• Loading state is an unfortunate cost when redundant, which is almost always.
• We want to be really sure we're testing invariants on the right state, though.

Most of the time, I think the rt.StateReadonly(&st) is unnecessary, and the transaction state could be used.

• TerminateSectors needs it, or would need refactoring to avoid it
• OnDeferredCronEvent also needs it with the present structure (due to processing sector termination)

I think we can pick a policy that we can use almost everywhere, with the possible exception of the two prior places. So the question is do we want to always load state, redundantly, to be sure it's the correct one? Or trust in our patterns and use the post-transaction state without re-loading it?

Thoughts @Stebalien @ZenGround0 ?

[ZenGround0]

Without investigating too deeply I prefer consistently re-loading state redundantly because

1. Correctness >> Performance for invariant checking
2. I am pessimistic invariant checking will ever be performant enough for this optimization to move the needle noticeably

It would be great to prioritize performance of invariant checking if we have the bandwidth at which point we could profile and revisit this question. But I expect working on parallelizing as in the migration to be much higher leverage.

[Stebalien]

Given that the only two cases where we don't currently reload the state are RepayDebt and WithdrawBalance, neither of which are performance critical, I'd just reload the state to be consistent (even though we know that burning funds, changing pledge, etc. are not reentrent).

[ZenGround0]

From sync with other actors implementors the inconsistency here was explicitly called out as burdensome, making this now higher priority to do consistently.