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

Raise or remove SectorsMax #470

Closed
ZenGround0 opened this issue Jun 11, 2020 · 9 comments
Closed

Raise or remove SectorsMax #470

ZenGround0 opened this issue Jun 11, 2020 · 9 comments
Labels
cleanup Technical debt recovery and other cleanup work P3 Not urgent or important

Comments

@ZenGround0
Copy link
Contributor

From builtin/miner/policy.go:

// TODO raise this number, carefully
const SectorsMax = 32 << 20 // PARAM_FINISH

Unclear what carefully means or whether this TODO entails beyond the param finish but adding for completeness while flushing out todos.

@ZenGround0 ZenGround0 changed the title Raise SectorsMax ? Fix SectorsMax Jul 17, 2020
@ZenGround0
Copy link
Contributor Author

@anorth
Copy link
Member

anorth commented Jul 19, 2020

I disagree, the purpose of SectorsMax is a bound on maximum computation/storage of worst-case computations. It's actually almost 100% irrelevant since #648 - all the places its used could have a much tighter bound. It has nothing to do with Window PoSt.

If there are supposed to be some other limits on sector counts implied by the proof type, that is a new and distinct request that should be motivated on its own terms. @nicola I will close this issue unless you describe a new requirement (but that requirement will probably cause a new constant to be needed).

@anorth anorth changed the title Fix SectorsMax SectorsMax as a function of proof type Jul 19, 2020
@nicola
Copy link
Contributor

nicola commented Jul 20, 2020

From my PR:

This guarantees that actors operations per miner are bounded. Also, this reflects the limit of sectors that can be proven in a day via WindowPoSt.

Let's call the two requirements SectorsMaxActors (the current SectorsMax) and SectorMaxProof[ProofType]. The number of sector for a miner should be the minimum between the two.

The reason for this need is the following: NSE won't be able to support the current SectorsMax without having more than one SNARK proof per deadline. In other not enough challenges that can be done in 48 SNARKs to cover up for the challenges required by NSE.

So yes, this is a limitation of proofs that should be accounted for that we found during this Params Audit. (this invalidates this comment: #695 (comment))

@Stebalien
Copy link
Member

I'm pretty sure this is the wrong variable. I believe you're looking for "partitions per deadline max". That is, the maximum number of proofs a miner would need to submit in a single deadline. That will get rid of the direct dependency on the proof type as partition size already depends on the proof type. It will also be more accurate because partitions may not be evenly spread out across all deadlines.

@nicola
Copy link
Contributor

nicola commented Jul 20, 2020

This is the issue that I see: on NSE a miner cannot have 32 << 20 sectors since the total number of sectors proven per deadline is smaller - in other words, you will run out of deadlines with those many sectors.

If there is a system in place which guarantees that there is another implicit limit of maximum number of sectors per miner (e.g. if all deadlines are full, new porep is rejected), I am ok with that. I just have not found such check yet!

Let me know if this clarifies or if we need to sync on this one.

@Stebalien
Copy link
Member

This limit (SectorsMax) is a systems limit (how many sectors the chain can handle). It has nothing to do with proofs.

I'm saying that if you want to enforce a maximum number of sectors per deadline, we need a maximum number of sectors per deadline. That's a new limit that you'll need to define.We don't have currently have a concept of "full" deadlines.

@anorth
Copy link
Member

anorth commented Jul 21, 2020

I have discussed with @nicola and am re-titling this issue back to discuss raising (or removing) this limit.

@anorth anorth changed the title SectorsMax as a function of proof type Raise or remove SectorsMax Jul 21, 2020
@nicola
Copy link
Contributor

nicola commented Jul 21, 2020

Confirm that my digression has been resolved, this has no tight with proofs today.

@anorth anorth added cleanup Technical debt recovery and other cleanup work P3 Not urgent or important and removed info-requested discussion labels Oct 18, 2020
@ZenGround0
Copy link
Contributor Author

@anorth I recall you saying the referenced constant is distinct from AddressedSectorsMax leading me to believe it does not exist anymore and we can close this.

@anorth anorth closed this as completed Oct 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cleanup Technical debt recovery and other cleanup work P3 Not urgent or important
Projects
None yet
Development

No branches or pull requests

4 participants