search

filecoin-project / specs-actors

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

Fix PoSt Dispute so snap deals upgrades can't be exploited #1521

Closed ZenGround0 closed 2 years ago

ZenGround0 commented 3 years ago

PoSt disputing relies on immutable SealedCIDs in sector infos between submission of optimistic window post and dispute but the current snap deals construction mutates this field. The consequence is that the PoSt proof covering the snap deals updated sector can always be successfully disputed until the updated sector is first PoSted with the new SealedCID.

A good way to fix this is to add a snapshot of sector infos to the DeadlineInfo. Then during the dispute this sector info tree is read instead of the current one. This is like the PartitionsSnapshot which exists for the same reason.

The migration should be simple. The v7 deadline struct has a new field and it is populated with whatever the current sector info root from the miner state is. Because SealedCid mutations only happen after the upgrade this should be fine.

BlocksOnAChain commented 3 years ago

@ZenGround0 who should this one be assigned to?

ZenGround0 commented 3 years ago

Description

The detailed description is above in the first issue comment.

Done Criteria

Non-criteria

Dependencies

Ongoing work

Not yet started

arajasek commented 3 years ago

We also need to decide what to do about sectors whose deadlines are currently open -- is that upgradeable?

BlocksOnAChain commented 3 years ago

@Kubuxu @lucaniz, as we discussed on the protocol call - what is your opinion on: https://github.com/filecoin-project/specs-actors/issues/1521#issuecomment-975683414

ZenGround0 commented 3 years ago

We also need to decide what to do about sectors whose deadlines are currently open -- is that upgradeable?

There shouldn't be any problems. In particular post should be unaffected. Did you have other concerns about this?

arajasek commented 3 years ago

@ZenGround0 The q (in my mind) is what happens if I Snap a sector whose deadline is currently open (a sector that i haven't posted for yet). Does the expected post become for the new sector cid or the old one?

The new sector probably makes more sense (and likely requires no code changes here), but might be something we need to explicitly handle in SP implementations like the lotus miner.

ZenGround0 commented 3 years ago

Ah I see this is a good point. I think this actually does warrant actors code changes. I think we should use a variation of the mutable deadlines idea that ProveCommitSector uses in order to avoid a lot of pain and suffering. Updating sectors in the current and next deadlines should be an error.

ZenGround0 commented 3 years ago

Actually I think the deadlineIsMutable function is exactly what we need. This pattern was designed to solve the totally analogous problem of partition contents changing during assignment.

arajasek commented 2 years ago

As part of this, add a test that you can't dispute good posts after a snap.

BlocksOnAChain commented 2 years ago

@arajasek - I will close the issue after we merge https://github.com/filecoin-project/specs-actors/pull/1544 since I pinged @Kubuxu and @Stebalien for a review, based on the talk I had with @jennijuju

ZenGround0 commented 2 years ago

Heads up #1544 is reviewed and merged, but no opposition on leaving this open to track additional review.