Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: remove DEAL_WEIGHT_MULTIPLIER #1573

Closed
rvagg opened this issue Aug 15, 2024 · 4 comments
Closed

Proposal: remove DEAL_WEIGHT_MULTIPLIER #1573

rvagg opened this issue Aug 15, 2024 · 4 comments

Comments

@rvagg
Copy link
Member

rvagg commented Aug 15, 2024

Currently, QAP is calculated based on 4 inputs: sector size, sector duration deal weight and verified weight. Both of these are on SectorOnChainInfo.

"Deal weight" and "verified weight" come in as QAP inputs as unverified_space and verified_space on DataActivationOutput. We multiply these by duration before passing them in to qa_power_for_weight (which we also pass in duration, along with sector size).

For ProveCommitAggregate and ProveReplicaUpdates, we calculate unverified_space based on actual deals and verified_space based on successful verified claims.

For ProveCommitSectors3 and ProveReplicaUpdates3, we calculate unverified_space based on pieces without a verified_allocation_key and verified_space based on successful verified claims (the pieces with verified_allocation_key but are not successfully claimed are not counted in this).

But, when we get down in to quality_for_weight, the "deal weight" is effectively ignored. We use the same duration we used to make these weights to calculate the total sector_space_time, but then the 3 parts we divide up to figure out QAP are:

  • weighted_deal_space_time - deal weight multiplied by 10 (DEAL_WEIGHT_MULTIPLIER)
  • weighted_verified_space_time - verified weight multiplied by 100 (VERIFIED_DEAL_WEIGHT_MULTIPLIER)
  • weighted_base_space_time - the remainder space, not accounted for by deal weight or verified weight, is multiplied by 10 (QUALITY_BASE_MULTIPLIER).

So, we're doing some complicated dances to get deal weight into this calculation, but it doesn't end up having any special impact on the calculation.

I propose we remove DEAL_WEIGHT_MULTIPLIER and simplify QAP calculation and inputs by removing the need to calculate and supply deal weight. All you need is verified size. This simplification applies here, and to go-state-types where QualityForWeight is a copy of the same function here, and in some Lotus APIs that are unnecessarily complex due to the need for full accounting (e.g. StateMinerInitialPledgeCollateral which really just needs 3 inputs, or ways to calculate them: size, duration and verified space. I'm proposing a new simpler one here, and it potentially could be simplified further: filecoin-project/lotus#12384).

As an aside: from what I can tell, deal weight doesn't really have any special impact other than being an interesting field on SectorOnChainInfo, even through sector update operations. In a post-DDO world, it seems that the only thing this now tells us with any certainty is that this sector had pieces in some form when onboarded. Even for partially full sectors, Lotus Miner fills out the sector with empty pieces before submitting a piece manifest, so it will still look full (perhaps other mining software does something cleverer for empty space?). So it's essentially a boolean: certain-CC vs maybe-not-CC. Is this correct?

@rvagg
Copy link
Member Author

rvagg commented Aug 15, 2024

@anorth
Copy link
Member

anorth commented Aug 15, 2024

Yes please, if we can, but there might be a challenge somewhere about accounting for older sectors that don't use the simple QAP calculation.

I think this complex construction existed in the first place to account for allocating QAP only for the deal duration (less than sector lifetime) associated with some non-zero data. There still exists sectors on the network that had their deal weight calculated as such.

@rvagg
Copy link
Member Author

rvagg commented Aug 19, 2024

From what I can see, as long as QUALITY_BASE_MULTIPLIER == DEAL_WEIGHT_MULTIPLIER, the deal_weight parameter will always be factored out of the result, regardless of how large or small it is. When we end up doing weighted_sum_space_time, we're cancelling it out.

Where WSS is weighted_sum_space_time, S is size, D is duration, W is deal_weight and V is verified_deal_weight, we end up doing this:

WSS = 10 * (S * D - W - V) + 10 * W + 100 * V
WSS = 10 * S * D - 10 * W - 10 * V + 10 * W + 100 * V
WSS = 10 * S * D - 10 * V + 100 * V
WSS = 10 * S * D + 90 * V

which leads to our result (without precision adjusting):

QAP = (S * D + 9 * V) / (S * D)

If I mess with the quality_is_independent_of_size_and_duration test and substitute various values for deal_weight, from 0, half of original, to a multiple of 2, 20, 2000, QAP always comes out the same.

It makes me wonder if the original intent was to have a DEAL_WEIGHT_MULTIPLIER that was different from QUALITY_BASE_MULTIPLIER, or at least the ability to tweak that particular knob like the 10x verified knob.

https://spec.filecoin.io/#section-systems.filecoin_mining.sector.sector-quality mentions this multiplier as if it's something to be tuned. I find it hard to imagine a world where "deal space" is going to be treated as anything different than CC space; especially now in a DDO world where the delta between CC and "deals" is just an ephemeral piece manifest.

I'll have a go at trying to remove it entirely and see where that gets us.

@anorth
Copy link
Member

anorth commented Aug 19, 2024

It makes me wonder if the original intent was to have a DEAL_WEIGHT_MULTIPLIER that was different from QUALITY_BASE_MULTIPLIER, or at least the ability to tweak that particular knob like the 10x verified knob.

Yes, that was an original intent, perhaps before the reality of being unable to prevent gaming it had been fully appreciated.

@rvagg rvagg closed this as completed in 92a7559 Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants