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: #7869 sealing: Add more deal expiration checks during PRU pipeline #7871

Merged
merged 5 commits into from
Jan 12, 2022

Conversation

jennijuju
Copy link
Member

@jennijuju jennijuju commented Jan 4, 2022

Related Issues

resolves #7869

Proposed Changes

add more deal error handling during PRU pipeline so that we can fail early without wasting too much work

Additional Info

Checklist

Before you mark the PR ready for review, please make sure that:

  • All commits have a clear commit message.
  • The PR title is in the form of of <PR type>: <#issue number> <area>: <change being made>
    • example: fix: #1234 mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, INTERFACE BREAKING CHANGE, CONSENSUS BREAKING, build, chore, ci, docs, misc,perf, refactor, revert, style, test
    • area: api, chain, state, vm, data transfer, market, mempool, message, block production, multisig, networking, paychan, proving, sealing, wallet
  • This PR has tests for new functionality or change in behaviour
  • If new user-facing features are introduced, clear usage guidelines and / or documentation updates should be included in https://lotus.filecoin.io or Discussion Tutorials.
  • CI is green

Copy link
Contributor

@arajasek arajasek left a comment

Choose a reason for hiding this comment

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

  1. i don't know how long ProveReplicaUpdate1 takes, but it may be rerunning the check before calling ProveReplicaUpdate2?
  2. what we really want is probably to extend SectorInfo to have a deadline epoch set to its latest deal activation (can be updated along the way as precommit and so on happens). then, every state transition can check if the sector is doomed before investing any more time in it.

but that is outside the scope of this pr -- an issue would be nice to have, though.

@@ -80,6 +79,24 @@ func checkPieces(ctx context.Context, maddr address.Address, si SectorInfo, api
return nil
}

func checkDealExpiration(ctx context.Context, sector SectorInfo, api SealingAPI) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is sufficiently different or faster than checkPieces to warrant its own method?

Copy link
Member Author

@jennijuju jennijuju Jan 4, 2022

Choose a reason for hiding this comment

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

  1. i don't know how long ProveReplicaUpdate1 takes, but it may be rerunning the check before calling ProveReplicaUpdate2?

PRU1 computes the vanilia proofs so yah it should take a while, added another check before the PRU2 - so it doesnt take much time

Copy link
Member Author

Choose a reason for hiding this comment

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

tho i do think it makes sense to check before PRU2 instead of PRU1 - updated

@jennijuju
Copy link
Member Author

jennijuju commented Jan 4, 2022

  1. what we really want is probably to extend SectorInfo to have a deadline epoch set to its latest deal activation (can be updated along the way as precommit and so on happens). then, every state transition can check if the sector is doomed before investing any more time in it.

i understand what you are proposing wrt the adding to sector info, but the earliest deal start epoch should stay the same after the snap deal sector is packed, so it wont be updated once set? so i guess we can find that in checkPiece, assign to sector info, then check against it?

@BlocksOnAChain BlocksOnAChain added this to the v1.14.0 milestone Jan 4, 2022
@ZenGround0 ZenGround0 force-pushed the feat/snap-deals branch 2 times, most recently from e3cd18b to 61e1732 Compare January 5, 2022 08:10
@jennijuju jennijuju added this to In progress in Network v15 via automation Jan 5, 2022
Base automatically changed from feat/snap-deals to master January 11, 2022 17:46
@arajasek arajasek merged commit f955084 into master Jan 12, 2022
Network v15 automation moved this from In progress to DONE Jan 12, 2022
@arajasek arajasek deleted the jen/7869 branch January 12, 2022 23:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Fail early if the deals are expired during PRU handlings
3 participants