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

fix(miner)!: remove DEAL_WEIGHT_MULTIPLIER and its input to QAP calc #1574

Merged
merged 1 commit into from
Sep 9, 2024

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Aug 19, 2024

Ref: #1573

As outlined in #1573, as long as QUALITY_BASE_MULTIPLIER == DEAL_WEIGHT_MULTIPLIER then we factor out deal_space in QAP calculations. It appears to me from the language of the spec that it was imagined that the "DealWeightMultiplier (DWM)" was something that could be tuned but unless anyone imagines a world where we tinker with anything other than the "VerifiedDealWeightMultiplier (VDWM)" then we should just do away with deal_weight entirely in the calculations to avoid confusion and overhead.

In terms of what this does to quality_for_weight, here's what I see in that code as it is currently. 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 QAP result (without precision adjusting):

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

i.e. W appears nowhere in the simplified version. We just do lots of unnecessary calculations with it.

You can even see this in the test for the QAP calculation quality_is_independent_of_size_and_duration:

    // Quality of space with no deals. This doesn't depend on either the sector size or duration.
    let empty_quality = BigInt::from(1 << SECTOR_QUALITY_PRECISION);
    // Quality space filled with non-verified deals.
    let deal_quality = &empty_quality * (DEAL_WEIGHT_MULTIPLIER.clone() / QUALITY_BASE_MULTIPLIER.clone());

Substitute * (DEAL_WEIGHT_MULTIPLIER.clone() / QUALITY_BASE_MULTIPLIER.clone()) with * (10/10).

It would be nice to take this simplification further and remove all "unverified space" calculations, but SectorOnChainInfo still needs it and I guess we should keep it accurate unless we FIP to remove it or set it to zero.

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks correct but... it would be nice to run a test on mainnet where we verify that it doesn't change power calculations in any way (if that's not too horribly difficult...).

@Stebalien
Copy link
Member

My concern is mostly due to potential rounding/scaling issues...

Copy link
Member

@anorth anorth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thank you.

I think it would be prudent to add a few tests that spell out the old calculation and verify it gets exactly the same results as the newly modified function.

@rvagg rvagg force-pushed the rvagg/remove-deal-space-multiplier branch 2 times, most recently from af6f03f to 7685ae3 Compare August 21, 2024 05:40
@rvagg
Copy link
Member Author

rvagg commented Aug 21, 2024

@anorth I added back in original_quality_for_weight which retains the original form (minus the constant) and pulled it in to quality_is_independent_of_size_and_duration to verify that it gets the same result.

@rvagg rvagg force-pushed the rvagg/remove-deal-space-multiplier branch 2 times, most recently from 464f051 to a47f724 Compare August 21, 2024 06:18
@rvagg
Copy link
Member Author

rvagg commented Sep 6, 2024

Updated with even more tests against the original form. Also matching the same tests in filecoin-project/go-state-types#308 but over there we get to test against the actual v14 form of it, which is nice.

@rvagg rvagg merged commit 92a7559 into master Sep 9, 2024
12 checks passed
@rvagg rvagg deleted the rvagg/remove-deal-space-multiplier branch September 9, 2024 03:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ☑️ Done (Archive)
Development

Successfully merging this pull request may close these issues.

3 participants