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

Miners cannot activate some sectors with verified deals #1138

Closed
geoff-vball opened this issue Jan 31, 2023 · 7 comments
Closed

Miners cannot activate some sectors with verified deals #1138

geoff-vball opened this issue Jan 31, 2023 · 7 comments
Labels
wontfix This will not be worked on

Comments

@geoff-vball
Copy link
Contributor

geoff-vball commented Jan 31, 2023

Summary: Miners cannot activate a sector if it contains a verified deal for which 60 days have passed from when the PublishStorageDeals message landed on chain for that deal.

Since FIP-0045, PublishStorageDeals creates an allocation with an expiration 60 days in the future. FIP-0045 states:

The market will accept verified deals with a StartEpoch beyond the maximum allowed allocation Expiration. If such a deal is not activated before the allocation expires, it will not confer QA power. However, the deal may still be activated and sector commitment should proceed (without QA power).

Right now we cannot activate sectors with such deals.

The params for ClaimAllocations are:

pub struct ClaimAllocationsParams {
pub sectors: Vec,
pub all_or_nothing: bool,
}

Right now, only the miner actor can call ClaimAllocations and always calls with all_or_nothing as true. This causes the entire sector activation to fail if the allocation for any verified deal in the sector is not able to be claimed. Since allocations expire 60 days after PSD, this is currently a hard window for sector activation.

I think the fix here is just changing the miner actor to call ClaimAllocations with all_or_nothing = false.

I talked this over with @arajasek and we don't think there is any attack vector here, so making the issue public.

@anorth

@anorth
Copy link
Member

anorth commented Jan 31, 2023

@ZenGround0 we added and set the all_or_nothing parameter during development, specifically in order to set it true. Can you recall the reasoning? It was something about preserving existing behaviour w.r.t partial failures. Like someone had suggested that if the SP is expecting 10x QAP, they'd rather fail the sector than accidentally be committed to a sector without the QAP (maybe it would be loss-making).

The PR is #715, but the discussion may have been in person.

Future APIs associated with filecoin-project/FIPs#298 will probably expose this choice to the operator, or at least be different enough that it's reasonable to change the semantics.

@ZenGround0
Copy link
Contributor

Like someone had suggested that if the SP is expecting 10x QAP, they'd rather fail the sector than accidentally be committed to a sector without the QAP (maybe it would be loss-making).

Yes as I remember it lotus TSEs asserted that this was the better behavior than allowing commitment to pass without the expected QAP, and the argument made sense to me. @TippyFlitsUK @rjan90

@ZenGround0
Copy link
Contributor

However, the deal may still be activated and sector commitment should proceed (without QA power).

@geoff-vball we can do this but check in with TSEs first. My guess is we want to keep it this way.

@geoff-vball
Copy link
Contributor Author

geoff-vball commented Jan 31, 2023

I can see the rationale there, and I do think most SPs would rather fail. The fact that the SP currently cannot commit if they want to seems problematic though.

Could we set the flag to false and move the decision making for this to the client?

@Reiers
Copy link

Reiers commented Feb 1, 2023

I don't think I was part of the initial discussion, but here is my two cents.

The concern was/is that if a Storage Provider expected 10x QAP and only received a portion of that QAP, they would rather have the sector activation fail than accidentally commit to a sector without the full amount of QAP.

SP's wants/needs the QAP, and if it turns into regular deal sector (that is most likely free since it was intended as a FIL+ deal.) That's just taking up space, even CC sector would be better because then you can at least upgrade it.

( If it turns into a CC sector then I don't see the issue. )

lotus TSEs asserted that this was the better behavior than allowing commitment to pass without the expected QAP

  • I can see that it would mess up the sealing pipeline and create other issues. But personally I would rather that the deal failed all together.

It seems very unlikely that a SP would hold a deal in PublishStorageDeals for 60 days...

@TippyFlitsUK
Copy link

I agree with Nicklas that I think the chances of this cropping up are very unlikely.

It would also make sense to have an env var/config available that would allow the client/SP to specify behaviour in niche use cases.

If the default is to fail the sector activation, I would hope that there would be clear messaging in client/SP logs that explains exactly why.

@github-project-automation github-project-automation bot moved this to 📋 Backlog in Network nv19 Feb 24, 2023
@anorth
Copy link
Member

anorth commented Mar 2, 2023

I think we can close this issue. We should add a parameter to sector activation that allows an SP to specify which behaviour they want, but that's an API change, and we generally try to minimise how often we do that (hence the current implementation which picks a policy). APIs will change anyway with future programmable markets stuff, and we can improve this at that point.

@anorth anorth removed this from Network nv19 Mar 2, 2023
@anorth anorth closed this as completed Jul 24, 2023
@anorth anorth added the wontfix This will not be worked on label Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

5 participants