FIP Discussion: In ProveCommit[Sector | Aggregate] a single bad deal doesn't ruin the sector
ZenGround0 opened this issue · 2 comments
Summary
In the current protocol a single expired deal id in the dealIDs field of an on chain precommit info will cause market deal activation to fail and cause the sector to be dropped from the batch or aggregate of sectors being committed. Instead this FIP proposes that the provider of a sector with such expired deals provide enough information for the commitment verification even when deals have expired.
Motivation
Occasionally deals expire between PreCommit and ProveCommit and in these cases sectors containing these deals will fail to commit. This is bad for miners and bad for the non-expired deals that would become active if the sector were committed.
Design
The ProveCommit and method takes an array of pieceCids and two associated arrays of PieceSizes and dealIDs as arguments. The ProveCommitAggregate method takes an array of such arrays. In the common case these fields are empty. In the case a miner has an expired deal they can specify the missing pieceCids their size and their DealIDs.
ComputeDataCommittment is now extended to take in associated arrays of dealIDs, pieceCids and PieceSizes so that the market actor has all the information needed to call the syscall for computing the sector's commD. This method will also include a validation that all provided overwrite pieceCids belong to a dealID with a proposal no longer on chain and that this dealID is below the current max dealID.
The market ActivateDeals method will now return a bitfield of the indexes of dealIDs that are valid and will skip over expired deals instead of raising an error (similar to the approach proposed in #142).
In the confirmSectorProofsValid subroutine the return to the market ActivateDeals method will be validated to match with the input expired dealIDs. Expired dealIDs will then be removed from their sector's PrecommitInfo and the DealWeight and VerifiedDealWeight fields will be recomputed with a call to the market actor. The created SectorInfo will be given these modified fields before being written to actor state.
Considerations
This proposal will increase the input parameter size for the common case of ProveCommit and ProveCommitAggregate messages with no expired dealIDs. We should investigate wrapping ProveCommit params in a cbor array or pointer so that we can represent empty expired dealIDs as compactly as possible.
There is no significant risk of the pieceCids of expired deals being set to different data than that in the expired proposal because deal existence is checked in precommit and so there is no cost savings between expiring a deal and writing an arbitrary cid to the sector vs making a deal with the correct pieceCid in the first place.
We could optionally use this opportunity of changing the ActivateDeals signature to make this method work with batches of sector-deals.
We could optionally use this opportunity of introducing sector weight recalculations in error cases to redo more accurate sector weight calculations for all sectors.
This would be really nice to have -- a good step towards improving the useability of Filecoin
Possible future work to break (or at least invert) the coupling between the miner and market actors may make this redundant. I don't have concrete details yet, but likely miners would prove sectors first and then activate deals in a subsequent call.