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

block in Publish state when message sending fails due to lack of funds #638

Merged
merged 2 commits into from
Oct 11, 2021

Conversation

nonsense
Copy link
Member

@nonsense nonsense commented Oct 6, 2021

Should fix: filecoin-project/lotus#7102


Change in behaviour here is that if PublishDeals fails with not enough funds we don't fail permanently the deal, but consider this a transient error, and we just block the deal in the Publish state - waiting for an action from the user, or for a restart of the node to try again.

We opted against automatic retry, because it is hard to predict for how long a deal has to be retried.

@nonsense nonsense force-pushed the nonsense/retry-in-publish-deal branch from fdbd21f to 0131e1c Compare October 6, 2021 13:29
@nonsense nonsense force-pushed the nonsense/retry-in-publish-deal branch 2 times, most recently from 1622db3 to 86f01aa Compare October 6, 2021 13:39
@codecov-commenter
Copy link

Codecov Report

Merging #638 (1622db3) into master (36ed4dd) will increase coverage by 0.20%.
The diff coverage is 78.95%.

❗ Current head 1622db3 differs from pull request most recent head 86f01aa. Consider uploading reports for the commit 86f01aa to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #638      +/-   ##
==========================================
+ Coverage   65.22%   65.41%   +0.20%     
==========================================
  Files          62       62              
  Lines        4467     4472       +5     
==========================================
+ Hits         2913     2925      +12     
+ Misses       1273     1265       -8     
- Partials      281      282       +1     
Impacted Files Coverage Δ
...oragemarket/impl/providerstates/provider_states.go 84.49% <78.95%> (-0.88%) ⬇️
storagemarket/impl/client.go 21.33% <0.00%> (-0.05%) ⬇️
retrievalmarket/events.go 80.00% <0.00%> (+80.00%) ⬆️
retrievalmarket/dealstatus.go 80.00% <0.00%> (+80.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 36ed4dd...86f01aa. Read the comment docs.

@nonsense nonsense force-pushed the nonsense/retry-in-publish-deal branch from 86f01aa to eebce51 Compare October 7, 2021 10:16
@nonsense nonsense marked this pull request as ready for review October 7, 2021 10:25
@nonsense nonsense requested a review from dirkmc October 7, 2021 10:26
Copy link
Contributor

@dirkmc dirkmc left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@dirkmc dirkmc merged commit 679e33e into master Oct 11, 2021
@dirkmc dirkmc deleted the nonsense/retry-in-publish-deal branch October 11, 2021 13:21
@dirkmc dirkmc mentioned this pull request Oct 11, 2021
rvagg pushed a commit that referenced this pull request Oct 15, 2021
#638)

* PublishDeal: do not return an error when lacking funds

* unit test: make sure that we stay in StorageDealPublish state if PublishDeal errors with not enough funds error
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.

Allow manual retry of deal publishing
3 participants