feat(miner): add DDO-friendly StateMinerInitialPledgeForSector #12384

rvagg · 2024-08-14T05:49:36Z

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:

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.
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 · 2024-08-14T05:55:23Z

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 · 2024-08-14T05:56:46Z

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

rvagg · 2024-08-15T00:10:55Z

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

rvagg · 2024-08-15T06:49:22Z

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 · 2024-08-15T09:31:17Z

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 · 2024-08-15T11:07:47Z

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!

storage/pipeline/states_replica_update.go

rvagg · 2024-08-21T05:15:40Z

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 · 2024-08-22T03:50:43Z

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 · 2024-09-17T03:30:47Z

Calling for objections before I land this.

BigLep · 2024-09-17T16:08:54Z

Per 2024-09-17 FilOz team meeting, @ZenGround0 said he would take this one.

@ZenGround0 : see @rvagg 's offer for a sync discussion in https://filecoinproject.slack.com/archives/CP50PPW2X/p1726550218788619 if that is helpful

rvagg · 2024-09-17T23:53:50Z

Important note here is that one option (pointed out helpfully by @jennijuju) with this is to just not offer a pledge API at all and let miner software deal with those concerns themselves. If you look in storage/pipeline/pledge.go you can see essentially that going on now—that file has evolved as the existing API has proven less useful over time (and broken post-DDO). With access to raw state, you can calculate these things yourself, although it's not exactly trivial.

rvagg · 2024-09-17T23:55:12Z

And here is curio doing the same thing itself: https://github.com/filecoin-project/curio/blob/b2e2def1bce2ad5d18a9564c98af51d38d0cb1b0/tasks/snap/task_submit.go#L608-L658 so maybe it doesn't even need this API.

jennijuju · 2024-09-18T00:07:56Z

Important note here is that one option (pointed out helpfully by @jennijuju) with this is to just not offer a pledge API at all and let miner software deal with those concerns themselves. If you look in storage/pipeline/pledge.go you can see essentially that going on now—that file has evolved as the existing API has proven less useful over time (and broken post-DDO). With access to raw state, you can calculate these things yourself, although it's not exactly trivial.

Based on the assumption of there is no other users of this API - I.e no one else has flagged there might be a correctness issue of the api (even that I believe having wrong return is worse than not providing anything). We can, easily bring the API back if this deprecation caught anyone unknown users eye from the release note and patch to add it back base on request.

LexLuthr · 2024-09-18T09:29:56Z

From Curio and miner POV, I would request not to remove the API. The pledge calculations are less about storing data but more aligned with what chain expects when storing the data. The calculation will always come from how chain works and how FIPs affect it. I still think this API should be maintained as part of the chain node.

jennijuju · 2024-09-18T12:52:48Z

From Curio and miner POV, I would request not to remove the API. The pledge calculations are less about storing data but more aligned with what chain expects when storing the data. The calculation will always come from how chain works and how FIPs affect it. I still think this API should be maintained as part of the chain node.

Is your code using / depending on it at all, if so, can you please point me to where it’s used?

LexLuthr · 2024-09-18T13:22:40Z

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 · 2024-09-18T16:24:38Z

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 · 2024-09-18T17:04:02Z

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.

ZenGround0 · 2024-09-18T16:23:19Z

api/api_full.go

+	// deal space; therefore, this method is deprecated. Use StateMinerInitialPledgeForSector instead
+	// and pass in the verified deal space directly.
+	//
+	// Deprecated: Use StateMinerInitialPledgeForSector instead.
 	StateMinerInitialPledgeCollateral(context.Context, address.Address, miner.SectorPreCommitInfo, types.TipSetKey) (types.BigInt, error) //perm:read


I'd prefer to just delete unless there are known users of this one today. I know its possible to have precommits hold deal info with ProveCommitAggregate but my understanding is there is not practical usage.

@rjan90 what do you think? I don't think it's unreasonable to just completely remove it from a 1.30.0, but we shouldn't do it in a 1.29.x. If the target is 1.29 then let's mark it as deprecated now and remove it in a major version bump.

This will target v1.30.0, which will be the mandatory release for nv24, unless there is users blocked by it and it warrants a backport to the v1.29.x train.

We should mark it as deprecated in the v1.30.0 release notes and announce that it will be completely removed in the v1.31.0 release. The v1.31.0 release will be shipped alongside v1.30.0 for the nv24 upgrade but includes higher-risk items that we don't want to obligate users to upgrade to during a network upgrade.

ZenGround0 · 2024-09-18T16:36:47Z

chain/actors/builtin/verifreg/actor.go.template

@@ -20,14 +20,58 @@ import (
 	"github.com/filecoin-project/lotus/chain/actors/adt"
 	"github.com/filecoin-project/lotus/chain/actors"
 	"github.com/filecoin-project/lotus/chain/types"
-    verifregtypes "github.com/filecoin-project/go-state-types/builtin/v9/verifreg"
+	verifregtypes "github.com/filecoin-project/go-state-types/builtin/v9/verifreg"
+	verifregtypes13 "github.com/filecoin-project/go-state-types/builtin/v14/verifreg"


I'm confused why we need two separate types. My impression is that the templating meta language, i.e. here should encode this sort of thing. And versions generated from the template at lower verifreg actor versions should not be using verifregtype13.

Ok the templating only generates 13 different versions for the state accessors and this is the actor template so this is not too bad. In that case can't we just call verifregtypes13 verifregtypes and use only one dependency?

Also it's confusing why its verifregtypes13 but we import v14, I guess these were introduced in 13 but latest is 14? This would go away if we had a single import

should be 12 .. not 13 or 14, and I'm not sure how I ended up with those numbers :chinscratch:

MinimumVerifiedAllocationTerm is the main thing we need, it only showed up in 12; we can't bump everything to 12 without breaking the entire verifreg.State API which requires v9 properties, most notably AllocationId. So either we break them all to require v12 properties, or import both just to expose a stable MinimumVerifiedAllocationTerm.

I've removed the rest of the properties, I was just trying to be complete instead of just the random grab-bag of properties currently exposed.

actually I need a bunch more than just MinimumVerifiedAllocationTerm, I've added those back in, but now I recall my original thinking - as long as I'm bringing in random pieces, why not be comprehensive

documentation/en/api-v1-unstable-methods.md

chain/actors/builtin/verifreg/actor.go.template

ZenGround0 · 2024-09-18T17:13:46Z

node/impl/full/state.go

+		return types.EmptyInt, err
+	}
+
+	deposit, err := rewardState.PreCommitDepositForPower(*powerSmoothed, sectorWeight)


TIL about this weirdness. rewardState should just return ThisEpochRewardSmoothed -- its already a function on the reward state. Then you can return this from pledgeCalculationInputs. I guess maybe you need to keep it this way so that the reward version can select miner code version but it would be ideal to clean it up.

sorry @ZenGround0 I'm struggling to follow this comment or figure out how to act on it, can you explain further and demonstrate how it might lead to a refactor of what's in here already? I'm not really up to speed on reward smoothed and power smoothed, so this is just monkey-typey refactoring without changing that logic.

ZenGround0 · 2024-09-18T17:14:08Z

node/impl/full/state.go

 	}

 	return types.BigDiv(types.BigMul(deposit, initialPledgeNum), initialPledgeDen), nil
 }

 func (a *StateAPI) StateMinerInitialPledgeCollateral(ctx context.Context, maddr address.Address, pci miner.SectorPreCommitInfo, tsk types.TipSetKey) (types.BigInt, error) {
-	// TODO: this repeats a lot of the previous function. Fix that.


ZenGround0 · 2024-09-18T17:14:52Z

node/impl/full/state.go

+		return types.EmptyInt, xerrors.Errorf("getting circulating supply: %w", err)
+	}
+
+	initialPledge, err := rewardState.InitialPledgeForPower(


Same thing here as with PCDForPower.

? are you referring to the ThisEpochRewardSmoothed comment you made above? you're going to have to explain further so I can turn this into something actionable, sorry for my lack of context

ZenGround0 · 2024-09-18T18:22:00Z

storage/pipeline/commit_batch.go

@@ -96,7 +90,6 @@ func NewCommitBatcher(mctx context.Context, maddr address.Address, api CommitBat
 		feeCfg:    feeCfg,
 		getConfig: getConfig,
 		prover:    prov,
-		pledgeApi: pa,


What's the thinking behind removing this separate concept?

it was only introduced recently to deal with the ddo breakage this PR is trying to fix: #12341 - so, cruft removal

ZenGround0 · 2024-09-18T18:23:57Z

storage/pipeline/commit_batch.go

-func (b *CommitBatcher) getSectorCollateral(sn abi.SectorNumber, tsk types.TipSetKey) (abi.TokenAmount, error) {
-	pci, err := b.api.StateSectorPreCommitInfo(b.mctx, b.maddr, sn, tsk)
+func (b *CommitBatcher) getSectorCollateral(sn abi.SectorNumber, pieces []miner.PieceActivationManifest, ts *types.TipSet) (abi.TokenAmount, error) {
+	pci, err := b.api.StateSectorPreCommitInfo(b.mctx, b.maddr, sn, ts.Key())


No idea how NI PoRep pipeline does collateral calculation but it can't use this function since there's no precommit on chain. It might be worth refactoring so we don't need to get PCI at all.

I did a draft (hacky) ni-porep version of the pipeline a few weeks back, it's just a matter of excluding pcd from the total collateral calculation, same thing applies to the changes here, you'd just skip the pcd part of this function: https://github.com/filecoin-project/lotus/pull/12398/files#diff-8ce3fb3c298a4e0a8970016def04e58a3c7811776ac9d3ad3140b999a1b84d53

itests/sector_miner_collateral_test.go

Fixes: #12369 deprecate StateMinerInitialPledgeCollateral since it only accounts for deals in PCI, which aren't present in a DDO world

rvagg requested review from LexLuthr and magik6k August 14, 2024 05:49

rvagg force-pushed the rvagg/StateMinerInitialPledgeForSector branch from d37b243 to 16ba134 Compare August 14, 2024 05:55

rvagg force-pushed the rvagg/StateMinerInitialPledgeForSector branch from 16ba134 to c57daac Compare August 14, 2024 06:19

rvagg mentioned this pull request Aug 15, 2024

Proposal: remove DEAL_WEIGHT_MULTIPLIER filecoin-project/builtin-actors#1573

Closed

rvagg mentioned this pull request Aug 15, 2024

Update StateMinerInitialPledgeCollateral method to be DDO aware #12369

Open

11 tasks

rvagg force-pushed the rvagg/StateMinerInitialPledgeForSector branch 2 times, most recently from 4357d93 to 53ab3da Compare August 15, 2024 11:03

rvagg marked this pull request as ready for review August 15, 2024 11:04

ribasushi reviewed Aug 20, 2024

View reviewed changes

storage/pipeline/states_replica_update.go Outdated Show resolved Hide resolved

rvagg force-pushed the rvagg/StateMinerInitialPledgeForSector branch 2 times, most recently from 7c23132 to 9bb78b0 Compare August 21, 2024 05:12

LexLuthr approved these changes Aug 21, 2024

View reviewed changes

rvagg force-pushed the rvagg/StateMinerInitialPledgeForSector branch from 759a6e8 to a032c1b Compare August 22, 2024 00:29

rvagg mentioned this pull request Aug 22, 2024

Common Node API filecoin-project/FIPs#1027

Draft

rvagg force-pushed the rvagg/StateMinerInitialPledgeForSector branch 2 times, most recently from bfbb4fa to 6f0cd11 Compare September 11, 2024 01:51

rvagg mentioned this pull request Sep 11, 2024

chore(deps): update go-state-types, adapt to QAP API changes #12445

Draft

BigLep assigned rvagg Sep 11, 2024

rvagg force-pushed the rvagg/StateMinerInitialPledgeForSector branch from 6f0cd11 to a0d2ae8 Compare September 17, 2024 03:30

BigLep requested a review from ZenGround0 September 17, 2024 16:07

ZenGround0 reviewed Sep 18, 2024

View reviewed changes

ZenGround0 approved these changes Sep 18, 2024

View reviewed changes

rvagg added 8 commits September 19, 2024 19:51

feat(miner): add DDO-friendly StateMinerInitialPledgeForSector

2454f20

Fixes: #12369 deprecate StateMinerInitialPledgeCollateral since it only accounts for deals in PCI, which aren't present in a DDO world

feat: simplify StateMinerInitialPledgeForSector arguments

4ee431a

feat(miner): use StateMinerInitialPledgeForSector for collateral calcs

4e2c8ae

docs: add StateMinerInitialPledgeForSector to CHANGELOG

8de5ba3

fix: undo unnecessary docsgen changes

96f71a0

chore(state): use types.EmptyInt as err return instead of big.Zero()

fe5b8d9

chore(miner): add tests for pledge calculations

d0b3b57

doc(miner): more docs about pledge calculation delta

955ea3e

rvagg force-pushed the rvagg/StateMinerInitialPledgeForSector branch from a0d2ae8 to c55777c Compare September 19, 2024 10:31

chore(miner): tweaks to pledge api from feedback

b714b82

rvagg force-pushed the rvagg/StateMinerInitialPledgeForSector branch from c55777c to b714b82 Compare September 19, 2024 10:37

feat(miner): add DDO-friendly StateMinerInitialPledgeForSector #12384

Are you sure you want to change the base?

feat(miner): add DDO-friendly StateMinerInitialPledgeForSector #12384

Conversation

rvagg commented Aug 14, 2024

rvagg commented Aug 14, 2024

rvagg commented Aug 14, 2024

rvagg commented Aug 15, 2024

rvagg commented Aug 15, 2024

LexLuthr commented Aug 15, 2024

rvagg commented Aug 15, 2024

rvagg commented Aug 21, 2024

LexLuthr left a comment

Choose a reason for hiding this comment

rvagg commented Aug 22, 2024

rvagg commented Sep 17, 2024

BigLep commented Sep 17, 2024

rvagg commented Sep 17, 2024

rvagg commented Sep 17, 2024

jennijuju commented Sep 18, 2024

LexLuthr commented Sep 18, 2024

jennijuju commented Sep 18, 2024

LexLuthr commented Sep 18, 2024

jennijuju commented Sep 18, 2024

LexLuthr commented Sep 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment