uplink/piecestore: Check SN piece hash timestamp #3246
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Uplink must verify that every piece upload to a storage node return a hash whose timestamp isn't older than the maximum elapsed time allowed by the Satellite. We cannot leave this check only to the Satellite site, because if there is no error reported by this matter, the uplink cuts down the long tail. When uplink submits the result uploads including these invalid ones, the Satellite filters out the invalid ones and that can provoke that it gets less than the optimal threshold amount of valid upload results, so it rejects the request. Detecting the error at this stage will allow the uplink to detect these uploads as invalid and avoid to cut down the long tail prematurely.
ifraixedes
added
Request Code Review
ghost
requested review from
jenlij and
JessicaGreben
and removed request for
a team
navillasa
approved these changes
Oct 12, 2019
| @@ -14,13 +15,17 @@ import ( | |||
| "storj.io/storj/pkg/signing" | |||
| ) | |||
|
|
|||
| const pieceHashExpiration = 2 * time.Hour | |||
There was a problem hiding this comment.
should this be configurable or okay as const?
There was a problem hiding this comment.
It was and it still is a const in the satellite part, so I applied the same.
https://github.com/storj/storj/blob/master/satellite/metainfo/metainfo.go#L36
Which is used in https://github.com/storj/storj/blob/master/satellite/metainfo/validation.go#L378
|
Time window will be changed with #3255 |
@littleskunk I've seen the changes in that PR but I'm not sure in how that impacts to this PR considering that the links posted to @navillasa question #3246 (comment) If something has to be changed with those changes merged by #3255 I would need some guidance. |
VinozzZ
approved these changes
Oct 15, 2019
|
@littleskunk if there is anything to change regarding your comment, please open a new ticket with it. |
littleskunk
pushed a commit
that referenced
this pull request
Oct 16, 2019
Uplink must verify that every piece upload to a storage node return a hash whose timestamp isn't older than the maximum elapsed time allowed by the Satellite. We cannot leave this check only to the Satellite site, because if there is no error reported by this matter, the uplink cuts down the long tail. When uplink submits the result uploads including these invalid ones, the Satellite filters out the invalid ones and that can provoke that it gets less than the optimal threshold amount of valid upload results, so it rejects the request. Detecting the error at this stage will allow the uplink to detect these uploads as invalid and avoid to cut down the long tail prematurely. (cherry picked from commit 9caa318)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What: Uplink verifies that the timestamp of upload result piece hash got from a storage node is not older than the amount of time that the Satellite accepts (currently 2 hours).
Why: Because if uplink doesn't verify it, it counts as a valid result upload piece and cuts down the long tail of potential valid ones.
When uplinks count the upload results with a too old timestamp as valid, it may send a number of upload results equal to the optimal threshold, then the Satellite filters out the ones with a too old timestamp and then get only a total number of valid ones less than the optimal threshold, causing to reject the uplink request with an error response and aborting completely the upload operation.
Please describe the tests: No additional test, the current test should pass.
Please describe the performance impact: N/A
It should close the ticket https://storjlabs.atlassian.net/browse/V3-2474
Code Review Checklist (to be filled out by reviewer)