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

Miner: CommitBatcher#processBatchV2 should check allocations can be claimed before executing (DDO UX) #11837

Closed
rvagg opened this issue Apr 5, 2024 · 3 comments · Fixed by #11841
Assignees

Comments

@rvagg
Copy link
Member

rvagg commented Apr 5, 2024

CommitBatcher#processBatchV2 populates params.SectorActivations with whatever activation manifest is ready for the sector, including its piece manifest which may include one or more verified allocations (VerifiedAllocationKey != nil).

There are a bunch of reasons a claim against one of these allocations may fail, and unfortunately the information is deep within an actor call stack such that the information can't be returned from there to tell the user why the claim failed. The execution fails when simulating gas costs (see #11836 for related concern) and the error output only has the params from that execution along with this kind of error: 00: f081660 (method 34) -- all data activations failed (16). The user doesn't know why it may have failed.

The failure will almost always arise out of this check: https://github.com/filecoin-project/builtin-actors/blob/a0e34d22665ac8c84f02fea8a099216f29ffaeeb/actors/verifreg/src/lib.rs#L1071-L1086

It should be straightforward to check each of these conditions before executing a message:

  • Client & provider match
  • Piece match
  • Expiration of allocation not too late
  • Sector lifetime within terms of allocation

Each of these cases could print its own error message and it would help debug data onboarding problems which are likely to be many for DDO.

@LexLuthr LexLuthr assigned LexLuthr and unassigned LexLuthr Apr 5, 2024
@LexLuthr
Copy link
Contributor

LexLuthr commented Apr 5, 2024

@rvagg The first two

  • Client & provider match
  • Piece match

are checked as part of the deal onboarding. The miner really does the sector side of things, maybe we should just check the sector expiration with allocation limits? This would cleanly separate out the checks responsibility between miner and market (Boost in this case). WDYT?

@rvagg
Copy link
Member Author

rvagg commented Apr 19, 2024

@LexLuthr sounds reasonable, the problem that a user saw was related to expirations anyway so I guess that was the one falling through the cracks here.

@LexLuthr
Copy link
Contributor

@rvagg The PR is already in place. Please review it and let me know if you require further changes to it.

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

Successfully merging a pull request may close this issue.

2 participants