-
Notifications
You must be signed in to change notification settings - Fork 176
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
server: Add suspending sessions that did not pass the pHash verification #2103
Merged
leszko
merged 7 commits into
livepeer:master
from
leszko:2097-suspend-untrusted-session
Nov 22, 2021
Merged
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
e2f94ca
server: Add suspending sessions that did not pass the pHash verification
leszko 9a5719f
Update CHANGELOG_PENDING.md
leszko 663f3ef
server: Add unit tests for suspending stream that returns invalid phash
leszko f2a4d7d
Update server/push_test.go
leszko c5c5a20
Remove glog.Errorf() from `push_test.go`
leszko e46591c
Merge branch '2097-suspend-untrusted-session' of https://github.com/l…
leszko 89fbc60
Merge branch 'master' of https://github.com/livepeer/go-livepeer into…
leszko File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not related to this exact issue or very high priority, so maybe we should open a new issue/PR for this:
Maybe we should reset our
verifiedSession
tonil
if the hash it sends doesn't match?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see one issue with adding this condition. If we add it, then a false-negative will cause
bsm.verifiedSession = nil
and in a result, we'll start looking for a new session. That can in turn cause bouncing between sessions if we encounter many false-negative scenarios.We decided that we'll suspend sessions only if it failed p-hash verification but at the same time the other untrusted sessions passed the verification. We could do the same and maybe set
verifiedSession
tonil
when we suspend the session, but technically it does not matter, because this condition takes into consideration bothbsm.verifiedSession = nil
and excluding suspended sessions.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point about false negatives causing swaps, and yeah we'd anyway suspend a verified session and look for a new one if the other untrusted session passes. LGTM feel free to merge 👍