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

feat: dealpublisher: check for duplicate deals before adding #9365

Merged
merged 2 commits into from
Sep 26, 2022

Conversation

arajasek
Copy link
Contributor

@arajasek arajasek commented Sep 23, 2022

Related Issues

TestDealsRetryLackOfFunds fails flakily (see eg here https://app.circleci.com/pipelines/github/filecoin-project/lotus/23264/workflows/87a70d59-974d-46d8-971a-54e1bba3afd3/jobs/595498)

This test sets up a SP with insufficient $$ to publish a deal (with the expectation, but not an assertion that the first try will fail), then after 3 seconds funds the publish key and triggers MarketRetryPublishDeal, and waits for the deal to succeed.

The cause of the flakiness is that the funding of the publish key and triggering MarketRetryPublishDeal can happen at a few different points wrt the first attempt to publish the deal (which involves adding to the publisher, waiting a second, and then validating and pushing to mempool).

Proposed Changes

There seem to be a few different things at play here:

  • 3 seconds doesn't seem enough to test intended behaviour here.
    • This PR increases that time to 30. Overkill in the extreme, but still much better than the time lost dealing with the flakiness of this test.
  • If MarketRetryPublishDeal is triggered on a deal that's in the publish queue, we seem to add the deal a second time. This is funky, but not ultimately a cause of failure.
    • This PR adds a dedup check when processNewDealing.
  • It seems like when MarketRetryPublishDeal is triggered on a deal in the StorageDealPublish stage, we "do nothing" except retrigger the handler for StorageDealPublish. We should:
    • confirm I'm right about this behaviour
    • confirm that's intentional?
    • if so, think about more edge-cases for potentially trying to republish a deal

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>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, INTERFACE BREAKING CHANGE, CONSENSUS BREAKING, build, chore, ci, docs,perf, refactor, revert, style, test
    • area: api, chain, state, vm, data transfer, market, mempool, message, block production, multisig, networking, paychan, proving, sealing, wallet, deps
  • 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

@arajasek arajasek requested a review from a team as a code owner September 23, 2022 16:49

// Sanity check that new deal isn't already in the queue
for _, pd := range p.pending {
pdPropCid, err := pd.deal.Proposal.Cid()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't fast, but shouldn't be perf critical. I also considered just comparing the bytes of the signature, but that feels hacky?

Opinions welcome.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, it's not performance critical, this approach LGTM 👍


// Sanity check that new deal isn't already in the queue
for _, pd := range p.pending {
pdPropCid, err := pd.deal.Proposal.Cid()
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, it's not performance critical, this approach LGTM 👍

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 this pull request may close these issues.

2 participants