dealpublisher: Fully validate deals before publishing #7234

magik6k · 2021-08-31T11:57:56Z

Should fix #7206

magik6k · 2021-08-31T12:03:24Z

This validation seems to be pretty fast, on my mainnet miner:

2021-08-31T14:02:02.435+0200    INFO    storageadapter  storageadapter/dealpublisher.go:370     validating deal {"took": 0.015638289, "proposal": "bafy..."}

codecov · 2021-08-31T12:07:06Z

Codecov Report

Merging #7234 (42135ec) into master (212400b) will increase coverage by 0.01%.
The diff coverage is 65.51%.

@@            Coverage Diff             @@
##           master    #7234      +/-   ##
==========================================
+ Coverage   39.09%   39.10%   +0.01%     
==========================================
  Files         614      614              
  Lines       64895    64923      +28     
==========================================
+ Hits        25369    25391      +22     
- Misses      35127    35132       +5     
- Partials     4399     4400       +1

Impacted Files	Coverage Δ
markets/storageadapter/dealpublisher.go	`81.46% <65.51%> (-2.72%)`	⬇️
markets/loggers/loggers.go	`89.28% <0.00%> (-10.72%)`	⬇️
extern/sector-storage/manager_calltracker.go	`57.70% <0.00%> (-4.85%)`	⬇️
chain/stmgr/call.go	`67.87% <0.00%> (-3.64%)`	⬇️
itests/kit/blockminer.go	`93.65% <0.00%> (-1.59%)`	⬇️
chain/vm/vm.go	`59.47% <0.00%> (-1.12%)`	⬇️
chain/messagepool/selection.go	`80.12% <0.00%> (-0.41%)`	⬇️
chain/store/store.go	`63.49% <0.00%> (-0.18%)`	⬇️
storage/wdpost_sched.go	`77.22% <0.00%> (ø)`
chain/sync_manager.go	`67.70% <0.00%> (+0.31%)`	⬆️
... and 7 more

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 212400b...42135ec. Read the comment docs.

arajasek

Should do the thing, test would be good to see

arajasek · 2021-08-31T14:40:42Z

markets/storageadapter/dealpublisher.go

@@ -315,6 +318,13 @@ func (p *DealPublisher) publishReady(ready []*pendingDeal) {
 // validateDeal checks that the deal proposal start epoch hasn't already


update comment (it's doing more now)

@magik6k can we add tests like @arajasek suggested before we take things further?

FYI - @BigLep

Thanks for adding the test and holding the standard for ourselves and others.

ZenGround0

LGTM

It would be ideal if this also had an integration test setting up a batch of two deals one failing because of client insufficent funds and asserting that only the good deal made it through. Idk how difficult that is to do though.

magik6k · 2021-09-01T10:05:48Z

The test still isn't exactly non-flaky, after a bunch of runs it hit:

https://app.circleci.com/pipelines/github/filecoin-project/lotus/17038/workflows/3663f40c-6018-47d2-a4fa-fc496585241a/jobs/255358/tests#failed-test-0

2021-08-31T18:00:09.321Z	INFO	markets	loggers/loggers.go:15	storage client event	{"name": "ClientEventFailed", "proposal CID": "bafyreibhep3h5wcixnm6lgixl5zfuv7dkwpbhnuw5urfucjdylzeyz5qyy", "state": "StorageDealError", "message": "unexpected deal status while waiting for data request: 11 (StorageDealFailing). Provider message: deal rejected: node error fetching verified data cap: data cap missing -- client not verified"}
    deals_publish_test.go:296: didn't expect this now

https://app.circleci.com/pipelines/github/filecoin-project/lotus/17035/workflows/293293f6-a104-4af8-a347-e9b326a3e3a4/jobs/255010/tests#failed-test-0

    deals_publish_test.go:303: 
        	Error Trace:	deals_publish_test.go:303
        	Error:      	Not equal: 
        	            	expected: 2
        	            	actual  : 1
        	Test:       	TestPublishNonVerifClient

BlocksOnAChain · 2021-09-06T09:29:06Z

@magik6k - I don’t know if it’s directly related to the tests we added for DealPublisher, but I see we are experiencing some flaky tests for this in our CI: https://app.circleci.com/insights/github/filecoin-project/lotus/workflows/ci/tests?branch=master&reporting-window=last-30-days.
Maybe we can take a look?
FYI: @jennijuju

BlocksOnAChain · 2021-09-06T09:30:35Z

Also @Stebalien created an issue with has logs related to this: #7252

jennijuju · 2021-09-06T17:12:07Z

@magik6k - I don’t know if it’s directly related to the tests we added for DealPublisher, but I see we are experiencing some flaky tests for this in our CI: https://app.circleci.com/insights/github/filecoin-project/lotus/workflows/ci/tests?branch=master&reporting-window=last-30-days.
Maybe we can take a look?
FYI: @jennijuju

The last commit should resolve it.

magik6k requested a review from a team as a code owner August 31, 2021 11:57

magik6k changed the title ~~dealpublisher: Fully validate deals xbefore publishing~~ dealpublisher: Fully validate deals before publishing Aug 31, 2021

magik6k force-pushed the feat/dealpub-validation branch from a12fdf3 to 6c6c5e3 Compare August 31, 2021 11:58

arajasek approved these changes Aug 31, 2021

View reviewed changes

ZenGround0 approved these changes Aug 31, 2021

View reviewed changes

magik6k force-pushed the feat/dealpub-validation branch from dc78e04 to 62a2f25 Compare August 31, 2021 17:46

jennijuju added P1 P1: Must be resolved and removed P1 P1: Must be resolved labels Sep 1, 2021

magik6k force-pushed the feat/dealpub-validation branch from 62a2f25 to 22ce0ec Compare September 2, 2021 21:06

dealpublisher: Fully validate deals before publishing

3869105

magik6k force-pushed the feat/dealpub-validation branch 3 times, most recently from d55f551 to 8093f52 Compare September 3, 2021 17:12

dealpublisher: Test deal verification

42135ec

magik6k force-pushed the feat/dealpub-validation branch from 8093f52 to 42135ec Compare September 3, 2021 17:40

BlocksOnAChain linked an issue Sep 6, 2021 that may be closed by this pull request

TestDealPublisher fails waiting for StateMinerInfo to be called #7252

Closed

jennijuju added this to the v1.11.3 milestone Sep 6, 2021

jennijuju merged commit 52cb19c into master Sep 6, 2021

jennijuju deleted the feat/dealpub-validation branch September 6, 2021 17:12

magik6k mentioned this pull request Sep 11, 2021

Single verified deal with insufficient cap makes all published deals fail #5661

Closed

3 tasks

nonsense mentioned this pull request Sep 30, 2021

Retry PSD with valid deals if the message failed #7368

Closed

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dealpublisher: Fully validate deals before publishing #7234

dealpublisher: Fully validate deals before publishing #7234

magik6k commented Aug 31, 2021

magik6k commented Aug 31, 2021

codecov bot commented Aug 31, 2021 •

edited

Loading

arajasek left a comment

arajasek Aug 31, 2021

BlocksOnAChain Aug 31, 2021

BlocksOnAChain Aug 31, 2021

BigLep Aug 31, 2021

ZenGround0 left a comment

magik6k commented Sep 1, 2021

BlocksOnAChain commented Sep 6, 2021

BlocksOnAChain commented Sep 6, 2021

jennijuju commented Sep 6, 2021

		@@ -315,6 +318,13 @@ func (p DealPublisher) publishReady(ready []pendingDeal) {
		// validateDeal checks that the deal proposal start epoch hasn't already

dealpublisher: Fully validate deals before publishing #7234

dealpublisher: Fully validate deals before publishing #7234

Conversation

magik6k commented Aug 31, 2021

magik6k commented Aug 31, 2021

codecov bot commented Aug 31, 2021 • edited Loading

Codecov Report

arajasek left a comment

Choose a reason for hiding this comment

arajasek Aug 31, 2021

Choose a reason for hiding this comment

BlocksOnAChain Aug 31, 2021

Choose a reason for hiding this comment

BlocksOnAChain Aug 31, 2021

Choose a reason for hiding this comment

BigLep Aug 31, 2021

Choose a reason for hiding this comment

ZenGround0 left a comment

Choose a reason for hiding this comment

magik6k commented Sep 1, 2021

BlocksOnAChain commented Sep 6, 2021

BlocksOnAChain commented Sep 6, 2021

jennijuju commented Sep 6, 2021

codecov bot commented Aug 31, 2021 •

edited

Loading