filecoin-project / specs-actors

DEPRECATED Specification of builtin actors, in the form of executable code.
Other
86 stars 102 forks source link

Fix unnecessary sector termination failure #1411

Closed ZenGround0 closed 3 years ago

ZenGround0 commented 3 years ago

As part of processing terminations miner actors call requestTerminateDeals which requires success on the market OnMinerSectorsTerminate method.

Terimination calls OnMinerSectorsTerminate with all deal ids covered by terminated sectors. OnMinerSectorsTerminate fails with ErrIllegalArgument if any input deal id's deal state is not found in the market actor's deal states map. However this deal state is removed when the deal expires so this error case is possible.

If a miner calls TerminateSectors with fewer than the max per epoch allowed processed terminations and at least one sector with at least one deal already expired then the whole call will fail which is not ideal. Worse if a miner calls TerminateSectors with more than the max per epoch allowed to be processed, or if the sector expires two weeks after faulting through cron, then the failure will be hit during cron. This will cause the miner actor to lose its cron job and therefore its power claim, kicking it out of the system until an upgrade fixes state.

This is not a security concern because while the miner's storage will no longer be checked for valid proofs and faulted during deadline cron the miner will be totally disallowed from winning block elections without a power claim.

I believe the fix is as simple as ignoring not found deals in OnMinerSectorsTerminate instead of throwing an error.

anorth commented 3 years ago

I'm fairly confident that the described situation can't happen, and there's no bug.

The code in question loads the deal proposal before the deal state. If the proposal is not found, it is skipped (no abort). The error could only happen if the proposal is found but the deal state is absent.

Basically we have invariants that the keys of deal states are a subset of proposals, and once a state is created, it exists until the proposal is deleted simultaneously.

The "no state for deal" abort could be triggered for a deal that has a proposal but no state because it has not been activated yet. But then it should not be part of a terminating sector. So this check is detecting invalid miner behaviour. We could remove it and skip the deal, but then we'd miss this misbehaviour.