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

IndexerAnnounceAllDeals logic and intent is not clear #882

Open
hannahhoward opened this issue Oct 6, 2022 · 0 comments
Open

IndexerAnnounceAllDeals logic and intent is not clear #882

hannahhoward opened this issue Oct 6, 2022 · 0 comments
Labels

Comments

@hannahhoward
Copy link
Collaborator

What

A close read of the code in IndexerAnnounceAllDeals (https://github.com/filecoin-project/boost/blob/main/indexprovider/wrapper.go#L75) suggests a large discrepancy what this method does for Boost deals and what it does for Legacy deals, making the ultimate intent of this method unclear.

We should clarify our intent here, and make the code match it.

Deal Lifecycle and indexer announcements

The deal lifecycle as it relates to indexer announcements should probably look something like this

Deal Stage Indexer
Pre-publish
Publish
Hand off to sealing Announcements start here
Sealing
Active On Chain
Expiration of deal Announcements end here

Current operation

Currently IndexerAnnounceAllDeals does the following:

  • It announces boost deals that are pre-publish, publish, and hand-off-to-sealing
    • Presumably these generate an error when it actually goes looking for CIDs, as the CAR files for most of these states either don't exist or haven't been indexed
    • It doesn't announce deals that are failed, or in any of the sealing /active states
    • boost treats announcement as a seperate stage after handing off to sealing, so there's a potential duplicate announce if the deal stage is dealcheckpoints.AddedPiece (i.e. handed off but not announced). if the code in AnnounceAllDealsToIndexer runs first, the announcement in deal execution will fail and the deal will get stuck in the AddedPiece stage, as ErrAlreadyPublished is treated as a failure.
  • It calls go-fil-markets Provider.AnnounceAllDealsToIndexer
    • This announces deals that are Handoff to sealing, Sealing or active on chain
      • Although a comment suggests the intention is to avoid duplicate initial announcement, StorageDealStaged, which is where the both handoff and announcement happen for go-fil-markets is included in the list of acceptable stages. The proper "we're past initial announcement" state would be any ones after the Handoff stage
    • it doesn't announce Failed or expired deals.

Intent of IndexerAnnounceAllDeals

I would expect IndexerAnnounceAllDeals to force-reannounce all deals that are in a state where an announcement should be made. This would mean every deal that has been handed off to the sealer and potentially announced once, but not failed or otherwise expired.

Believing this is what this function does, I used it for #839 . But as the description above makes clear, that is what the code actually does.

If "force-reannounce" isn't the intent of the original code, that's fine -- forced re-announce could be a seperate function. However, what the intent of the code as it exists now is very unclear, as legacy deals largely follow the "forced-reannounce" intent but Boost deals definitely do not. I couldn't find from any of blames or original PR discussions what the intent of this function is.

Boost not tracking expiration of deals

Currently Boost deals that don't fail appear to simply stay at IndexedAndAnnounced forever.

A comment here reads:

	// TODO
	// Watch deal on chain and change state in DB and emit notifications.

Suggesting this functionality was going to be built but hasn't been.

We probably need this as it relates to the indexer, especially if we're going to start re-announcing on a regular basis as #839 does. Otherwise expired deals will get reannounced.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: No status
Development

No branches or pull requests

2 participants