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

Fix incorrect processing of VerificationFreq parameter #2740

Merged
merged 3 commits into from
Jan 30, 2023

Conversation

cyberj0g
Copy link
Contributor

About
VerificationFreq parameter of the transcoding job controls how often fast verification is re-run on an already verified O session. Fast verification involves transcoding segment with 1 trusted and 2 untrusted Os, instead of using a single O. This PR fixes incorrect processing of this parameter. Valid values are 0 and positive integers. If VerificationFreq = 0, fast verification logic is fully disabled. If VerificationFreq=N>0, then, after initial verification run for a new O session, it will run again for a segment with a probability of 1 / VerificationFreq.

Issue
The shouldSkipVerification function used VerificationFreq parameter in an inverted way. VerificationFreq=1 caused fast verification to run only once, as long as verified session is available, and higher values caused fast verification to run more and more often, with a value of e.g. 40 giving a 97.5% chance of running fast verification for each new segment of the stream.

@@ -397,7 +397,7 @@ func (bsm *BroadcastSessionsManager) shouldSkipVerification(sessions []*Broadcas
if !includesSession(sessions, bsm.verifiedSession) {
return false
}
return common.RandomUintUnder(bsm.VerificationFreq) == 0
return common.RandomUintUnder(bsm.VerificationFreq) != 0
Copy link
Member

Choose a reason for hiding this comment

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

Ooof good catch!

IIUC this method generates a random integer x s.t. 0 <= x < VerificationFreq and there would be a 1 / VerificationFreq probability for the random integer to be 0. Since this method is checking if verification should be skipped, if the check on this line is whether the integer is equal to 0 then this method would return true 1 / VerificationFreq of the time which is the opposite of what we want i.e. this method should return true (VericationFreq - 1) / VerificationFreq) of the time.

@codecov
Copy link

codecov bot commented Jan 30, 2023

Codecov Report

Merging #2740 (1e15f5d) into master (d8ad5c7) will not change coverage.
The diff coverage is 100.00000%.

Impacted file tree graph

@@              Coverage Diff              @@
##              master       #2740   +/-   ##
=============================================
  Coverage   56.34825%   56.34825%           
=============================================
  Files             88          88           
  Lines          19147       19147           
=============================================
  Hits           10789       10789           
  Misses          7767        7767           
  Partials         591         591           
Impacted Files Coverage Δ
server/broadcast.go 77.46835% <100.00000%> (ø)

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 d8ad5c7...1e15f5d. Read the comment docs.

Impacted Files Coverage Δ
server/broadcast.go 77.46835% <100.00000%> (ø)

@cyberj0g cyberj0g merged commit 6ec62a9 into master Jan 30, 2023
@cyberj0g cyberj0g deleted the ip/fix-fv-frequency-logic branch January 30, 2023 10:13
eliteprox pushed a commit to eliteprox/go-livepeer that referenced this pull request Feb 3, 2023
* fix semantics of shouldSkipVerification

* go fmt

* Fix test to align with the meaning of verificationFreq parameter
@thomshutt thomshutt mentioned this pull request Feb 10, 2023
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.

None yet

3 participants