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

remove wrong peer check in push deal validation #585

Merged
merged 3 commits into from
Jul 23, 2021

Conversation

whyrusleeping
Copy link
Member

We should not have this check, restricting data transfer to only the specific peer who made the deal cuts out a lot of potential architectures (and is silly anyways given the ephemeral nature of libp2p peer IDs). Knowledge of the proposal CID and the miners desire of the data being pushed in the first place should be validation enough.

@whyrusleeping
Copy link
Member Author

Not sure if this is a flaky test or what:

Failed
=== RUN   TestRestartClient/ClientEventDealActivated
    integration_test.go:565: storage deal proposed
    integration_test.go:533: event ClientEventDealActivated has happened on client, shutting down client and provider
    integration_test.go:568: both client and provider have been shutdown the first time
    integration_test.go:572: client state after stopping is StorageDealActive
    integration_test.go:581: provider state after stopping is StorageDealActive
    integration_test.go:606: ------- client and provider have been restarted---------
    integration_test.go:608: ---------- finished waiting for expected events-------
    testutil.go:109: 
        	Error Trace:	testutil.go:109
        	            				integration_test.go:612
        	Error:      	Not equal: 
        	            	expected: 0x8
        	            	actual  : 0x7
        	Test:       	TestRestartClient/ClientEventDealActivated
        	Messages:   	Unexpected deal status
        	            	expected: StorageDealExpired (8)
        	            	actual  : StorageDealActive (7)
    --- FAIL: TestRestartClient/ClientEventDealActivated (0.23s)

Failed in CI, but passes locally.

@Stebalien
Copy link
Member

  1. Are we able to recover if the wrong peer sends us something and then fails?
  2. What happens if two peers send us the deal at the same time?

(just making sure there's no attack surface here, this change is definitely correct in principle).

@whyrusleeping
Copy link
Member Author

@Stebalien yeah, failed data transfers have no bearing on the deal itself. And this doesnt let any random peer send data for my transfer, they could just start their own transfer for the same data/deal

If we are both sending data for the same deal, the first one to succeed will trigger the deal machinery to move forward, the second one seems like it will send a TransferCompleted event to the fsm, which will throw an error message and ignore the event because thats not a valid transition from the state that it will be in at that time.

@ribasushi
Copy link
Contributor

Not sure if this is a flaky test or what:

Hrm... go test -count=1 ./... passes locally for me too

Copy link
Contributor

@ribasushi ribasushi left a comment

Choose a reason for hiding this comment

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

Seems correct, the explanation what happens on clashing transfers sounds reasonable

@whyrusleeping nit: fix the comments on top of the validators to match new reality

@codecov-commenter
Copy link

Codecov Report

Merging #585 (11d1f29) into master (b9e514c) will decrease coverage by 0.23%.
The diff coverage is n/a.

❗ Current head 11d1f29 differs from pull request most recent head 1380c95. Consider uploading reports for the commit 1380c95 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #585      +/-   ##
==========================================
- Coverage   66.77%   66.55%   -0.22%     
==========================================
  Files          56       56              
  Lines        4227     4223       -4     
==========================================
- Hits         2822     2810      -12     
- Misses       1155     1163       +8     
  Partials      250      250              
Impacted Files Coverage Δ
storagemarket/impl/requestvalidation/common.go 85.19% <ø> (-1.91%) ⬇️
retrievalmarket/events.go 0.00% <0.00%> (-80.00%) ⬇️
retrievalmarket/dealstatus.go 0.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 b9e514c...1380c95. Read the comment docs.

@whyrusleeping whyrusleeping merged commit 9ef0c55 into master Jul 23, 2021
@whyrusleeping whyrusleeping deleted the fix/remove-wrong-peer-check branch July 23, 2021 18:37
aarshkshah1992 pushed a commit that referenced this pull request Jul 26, 2021
* remove wrong peer check in push deal validation (#585)

* remove wrong peer check in push deal validation

* kill the pull side of peer id checking as well

* fixup comments

* Do not hex-encode CIDs in logs (#561)

* On overloaded CI 10 seconds just isn't enough (#587)

* support padding out smaller files (#536)

* support padding out smaller files

* Actually write out the padding - AP depends on it

Co-authored-by: Peter Rabbitson <ribasushi@protocol.ai>

Co-authored-by: Whyrusleeping <why@ipfs.io>
Co-authored-by: Raúl Kripalani <raul@protocol.ai>
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.

4 participants