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

broadcaster introspection: update verification sigverification #2844

Merged

Conversation

ad-astra-video
Copy link
Contributor

What does this pull request do? Explain your changes. (required)
Update verification to exit early if OrchestratorInfo.Address is nil rather than OrchestratorInfo.TicketParams.Recipient.

This is a PR that will allow to log the on chain Orchestrator eth address with broadcaster introspection.

For additional discussion: #2840 (comment)

Specific updates (required)

  • Change sigVerification function to exit early if OrchestratorInfo.Address is nil rather than OrchestratorInfo.TicketParams.Recipient.
  • Update tests to

How did you test each of these updates (required)

  • Ran test suite github action
  • Tested transcoding 4 segments

Does this pull request close any open issues?
No

Checklist:

@ad-astra-video
Copy link
Contributor Author

Logs from github action test run on my repo
logs_691.zip

Logs from local test of transcoding (cpu transcoding :))
broadcaster.log
orchestrator.log

@@ -184,7 +184,7 @@ func IsRetryable(err error) bool {
}

func (sv *SegmentVerifier) sigVerification(params *Params) error {
if params.Orchestrator == nil || params.Orchestrator.TicketParams == nil {
if params.Orchestrator == nil || params.Orchestrator.Address == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it actually exit early in both cases, like this?

if params.Orchestrator == nil || params.Orchestrator.Address == nil  || params.Orchestrator.TicketParams == nil {
    return nil
}

Otherwise, I guess we can get panic in the next lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is better. I will update.

@codecov
Copy link

codecov bot commented Aug 23, 2023

Codecov Report

Merging #2844 (a7d76fe) into master (a231510) will not change coverage.
The diff coverage is 100.00000%.

Impacted file tree graph

@@              Coverage Diff              @@
##              master       #2844   +/-   ##
=============================================
  Coverage   56.70668%   56.70668%           
=============================================
  Files             88          88           
  Lines          19160       19160           
=============================================
  Hits           10865       10865           
  Misses          7699        7699           
  Partials         596         596           
Files Changed Coverage Δ
verification/verify.go 89.16667% <100.00000%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a231510...a7d76fe. Read the comment docs.

Files Changed Coverage Δ
verification/verify.go 89.16667% <100.00000%> (ø)

@leszko
Copy link
Contributor

leszko commented Aug 23, 2023

@ad-astra-video Please ping me when your PR is ready to review again. Thanks.

@ad-astra-video
Copy link
Contributor Author

@leszko ready to review. Had to update a couple tests to account for having three exit early tests in sigVerification.

@leszko
Copy link
Contributor

leszko commented Aug 23, 2023

@ad-astra-video Approved, thanks for the PR. I'll merge it as soon as all the CI builds are completed.

@leszko leszko merged commit f350fba into livepeer:master Aug 23, 2023
15 of 16 checks passed
eliteprox pushed a commit to eliteprox/go-livepeer that referenced this pull request Feb 21, 2024
…eer#2844)

* update verify.go sigVerification to return early if sess.Address() is nil instead of TicketParams

* update verify_test.go for change in early exit of sigVerification

* update test broadcastsession to add TicketParams.Recipient

* update sigVerification early exit per comment

* update tests for early exit change in sigVerification

---------

Co-authored-by: Rafał Leszko <[email protected]>
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

2 participants