Skip to content
This repository has been archived by the owner on Jun 6, 2023. It is now read-only.

Miner Policy: Correct truncating division #1250

Open
wadealexc opened this issue Oct 13, 2020 · 2 comments
Open

Miner Policy: Correct truncating division #1250

wadealexc opened this issue Oct 13, 2020 · 2 comments
Labels
change-behaviour Changes behaviour or state interpretation, necessitating a network version upgrade P2 Medium priority, beneficial for network functionality and growth

Comments

@wadealexc
Copy link

QualityForWeight returns the average of weighted space time here:

// Average of weighted space time: (scaledUpWeightedSumSpaceTime / sectorSpaceTime * 10)
return big.Div(big.Div(scaledUpWeightedSumSpaceTime, sectorSpaceTime), builtin.QualityBaseMultiplier)

Dividing twice may introduce error in some cases. The return can be re-written to divide only once:

return big.Div(scaledUpWeightedSumSpaceTime, big.Mul(sectorSpaceTime, builtin.QualityBaseMultiplier))
@anorth
Copy link
Member

anorth commented Oct 14, 2020

IMO this is worth fixing up in the next major version update. This function is called quite frequently, so avoiding the extra division may make a just-noticeable performance improvement too. But the frequent use makes plumbing the network version through to do it in a minor upgrade rather painful.

@anorth anorth added the P2 Medium priority, beneficial for network functionality and growth label Oct 14, 2020
@anorth anorth added this to the v3 milestone Oct 18, 2020
@ZenGround0 ZenGround0 added the change-behaviour Changes behaviour or state interpretation, necessitating a network version upgrade label Oct 28, 2020
@anorth anorth modified the milestones: 🧡 v3 , v4 Jan 21, 2021
@ZenGround0 ZenGround0 mentioned this issue Apr 8, 2021
28 tasks
@ZenGround0 ZenGround0 modified the milestones: v6, QoL Improvements Aug 12, 2021
@ZenGround0
Copy link
Contributor

This needs analysis or a network upgrade to be safe in case power values change between the two calculations.

Approach 0: we could mathematically guarantee that the loss in precision is never enough to cause a difference between power values of sectors. Then this is purely an optimization trading a Div for a Mul. It's probably the case we can bound error (SectorQualityPrecision gives us 20 bits of room) and that Div's are more expensive than Muls so we should start here.

Approach 1: Write a migration traversing all partitions / deadlines and rerunning all power calculations over all sectors to recompute power value. This traverses essentially all state blocks (sector infos + deadline trees)

Approach2: Migration only recomputes partition power values if partition power is not the same as (constant power of cc sector) * (number of sectors) because this is the only case the power calculations will be different. This relies on the fact that QAPowerForWeight will always divide out exact multiples when there is no verified deal weight.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
change-behaviour Changes behaviour or state interpretation, necessitating a network version upgrade P2 Medium priority, beneficial for network functionality and growth
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants