feat(miner): add DDO-friendly StateMinerInitialPledgeForSector

[rvagg]

Fixes: #12369

• Deprecate StateMinerInitialPledgeCollateral since it only accounts for deals in PCI, which aren't present in a DDO world

Here's the QAP calculation in actors that we have to work around for pledge calculation: https://github.com/filecoin-project/builtin-actors/blob/82d02e58f9ef456aeaf2a6c737562ac97b22b244/actors/miner/src/lib.rs#L5530-L5539

When PreCommits contained deal information, the QAP calculation works out the ~same because the deals can tells us how much verified vs unverified space there is because the deals contain that information. This calculation is still done in actors for ProveCommitAggregate and ProveReplicaUpdates, but from what I can tell we don't even use them anymore in lotus-miner (I'd like confirmation of this from someone that knows for sure beyond my simple grepping). So currently the only path to QAP calculation that we take is with piece manifests, which don't have deal information, they only have (1) size and (2) verified or not.

In Actors, the piece manifest path goes through here: https://github.com/filecoin-project/builtin-actors/blob/82d02e58f9ef456aeaf2a6c737562ac97b22b244/actors/miner/src/lib.rs#L5784-L5798 where we're simply adding up piece size and verified claim size (the caveat here is that we're actually doing the claim in actors so it must be successful, here are don't know for sure it will work out). If we're using filler pieces to pad out an un-full sector then that would come in to play here too I suppose, the piece manifest doesn't care about deals.

So, for pledge calculation, we only need the current reward state and info about the sector being onboarded: it's size, duration and the piece list. We don't actually need the PreCommit for that sector to work this out, although we could use it to figure out duration and size if we wanted. But without the piece manifest, which isn't on the PreCommit for DDO, we don't get an accurate result: there are no deals, so both "unverified space" and "verified space" end up being zero, hence the under-calculation of pledge. We at least need to supply the piece manifest, which has the information we need.

I've opted here for introducing a new API, StateMinerInitialPledgeForSector, which does this, but I've made some choices here which could be debated and I'd like feedback please:

1. I've opted to accept a "duration" and a "sector size" parameter, so the caller can figure those out. They could come from a PreCommit, which we could load from the sector number. So, we could reduce the flexibility of this by either requiring a sector number, or requiring a SectorActivationManifest which contains the piece manifests and the sector number, and then we can look up the precommit. It would make the args simpler, but make the method less flexible.
2. I don't do as much validation as StateMinerInitialPledgeCollateral did. It does some basic checks on deals to make sure they can activate: https://github.com/filecoin-project/go-state-types/blob/ee0897a7c86ea81d6a0d3b4ca3cc90cb81a1a99d/builtin/v14/market/market_state.go#L207-L219 - do we want to do something like this for claims?

Additionally, storage/pipeline/pledge.go does some of this work already. It's used for PRU3 collateral diff calculation: https://github.com/filecoin-project/lotus/blob/release/v1.28.1/storage/pipeline/states_replica_update.go#L134-L150, and I think that's wrong. I think we should be able to use this same API to supply the new piece manifest. But I'd like confirmation and help from @filecoin-project/curio team on that please. Note that in pledge.go it counts deals, not pieces, so it's going to get messed up on filler pieces, which would make partially-full sectors problematic.

[rvagg]

Ugh, catching up to #12341, I was working on an old master and didn't have that. But I think this new API makes #12341 unnecessary and we should be able to wind that back and ditch pledge.go entirely.

Rebased and reconciled on current master.

[rvagg]

Also needs some testing .. gotta figure out a good path to testing this API.

[rvagg]

More background as I dig further on this: filecoin-project/builtin-actors#1573

[rvagg]

As per filecoin-project/builtin-actors#1573 I think we can simplify the API even further; here are the options as I see them:

• StateMinerInitialPledgeForSector(size, duration, pieceManifests) so you can use it arbitrarily to do the calculation with arbitrary params, or
• StateMinerInitialPledgeForSector(sectorManifest) so you can load the precommit and figure out size and duration from that, and potentially do some checking on the claims to confirm if they could be successfully activated (provider, term, etc.).
• StateMinerInitialPledgeForSector(duration, size, verifiedSize) because ultimately those are the only inputs we need, the rest comes from the power actor.

[LexLuthr]

3rd would be the best implementation in my opinion. It disconnects the API from how sector metadata is handled by any implementation. Finding verified size and size are easy enough. This would also allow external parties (not miner implementations) to find collateral requirements without looking at the chain history.

[rvagg]

Still not sure how to test this, I started writing a unit test but got hung up on trying to build a usable state tree to run this against!

[rvagg]

I came up with a way to test this in an itest, described in there like so:

// TestPledgeCalculations tests the pledge calculations for sectors with different piece combinations
// and verified deals.
// We first verify that the deprecated pledge calculation that uses PreCommit with Deal information
// matches the new one that just uses sector size, duration and a simple verified size.
// Then we compare the pledge calculations for sectors with different piece combinations and
// verified deals according to expected rules about verified pieces requiring 10x pledge.
// Most of the complication in this test is for setting up verified deals and allocations so we can
// run the deprecated pledge calculation on a valid precommit.

We onboard and test the following types of sectors:

• CC
• FullPiece
• HalfPiece
• TwoHalfPieces
• FullPieceVerified
• HalfPieceVerified
• TwoHalfPiecesVerified
• HalfPieceUnverifiedHalfPieceVerified

Then we compare the outputs of StateMinerInitialPledgeCollateral (old, that I've deprecated in this PR) and StateMinerInitialPledgeForSector (new, which only takes duration and verified size).

Then we compare our expectations on the pledge outputs from them:

• all sectors without verified pieces should have the same pledge
• fully verified sectors should have the same pledge
• half verified sectors should have the same pledge
• full verified sectors should be 10x CC
• half verified sectors should be 5x CC + 1/2 CC

[LexLuthr]

I haven't reviewed the calculation itself as I don't know how that is supposed to work. Rest of it looks good. Thank you doing the miner side as well. I love the test. Simple and efficient.

[rvagg]

Added some more documentation both in the itest and in the API so a future confused maintainer will understand what's going on with this DealID thing.

[rvagg]

Calling for objections before I land this.

[LexLuthr]

Not at the moment because we don't have a choice. Existing method is broken. If this method created/fixed both Curio and miner will depend on it instead of doing calculations themselves.
This was the reason, I requested fixing this method in the first place. Any change in calculation can have drastic impact on storage pipeline.

[jennijuju]

Not at the moment because we don't have a choice. Existing method is broken. If this method created/fixed both Curio and miner will depend on it instead of doing calculations themselves. This was the reason, I requested fixing this method in the first place. Any change in calculation can have drastic impact on storage pipeline.

• one point @rvagg didn't capture in his earlier comment with our convo was that I personally think that State APIs should only return direct read-off actor state (return data structure changes are okay imho). This API is doing too many "off actor" work to be considered as a state API for my taste -> error prune to reflect true state
• Is there an open issue in curio and lotus miner for these code bases to consume this API? and how high of priority / how likely is it for your team to move away from the existing code base you have and use this api?

[LexLuthr]

There are no open issues because we are tracking this one. Once this is merged, we will be switching immediately to this method instead of doing the calculation on miner side. This PR already has changes for lotus-miner courtesy Rod. I will be switching to latest lotus dependency in Curio as soon as this is merged and get rid the manual calculation there.

This API is not only useful for storage implementations but will also allow external parties to calculate pledges as discussed before. This will open up a whole new set of use cases.