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 price per session #2892

Merged
merged 3 commits into from Oct 19, 2023
Merged

Conversation

leszko
Copy link
Contributor

@leszko leszko commented Oct 17, 2023

fix #2891

@leszko leszko requested a review from thomshutt October 17, 2023 12:40
@codecov
Copy link

codecov bot commented Oct 17, 2023

Codecov Report

Merging #2892 (2d8c94b) into master (32d5d45) will increase coverage by 0.01121%.
The diff coverage is 100.00000%.

❗ Current head 2d8c94b differs from pull request most recent head 0f621ac. Consider uploading reports for the commit 0f621ac to get more accurate results

Impacted file tree graph

@@                 Coverage Diff                 @@
##              master       #2892         +/-   ##
===================================================
+ Coverage   56.45344%   56.46465%   +0.01121%     
===================================================
  Files             89          89                 
  Lines          19416       19421          +5     
===================================================
+ Hits           10961       10966          +5     
  Misses          7849        7849                 
  Partials         606         606                 
Files Coverage Δ
server/broadcast.go 77.97571% <100.00000%> (+0.01785%) ⬆️
server/rpc.go 72.10682% <ø> (ø)
server/segment_rpc.go 78.48837% <100.00000%> (+0.12580%) ⬆️

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 32d5d45...0f621ac. Read the comment docs.

Files Coverage Δ
server/broadcast.go 77.97571% <100.00000%> (+0.01785%) ⬆️
server/rpc.go 72.10682% <ø> (ø)
server/segment_rpc.go 78.48837% <100.00000%> (+0.12580%) ⬆️

Copy link
Contributor

@thomshutt thomshutt left a comment

Choose a reason for hiding this comment

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

Looks good but this will mean every time an O increases their prices, they lose sessions right? I think this is good as a patch for the vulnerability, but does somewhat limit the ability of the Os to do dynamic pricing changes without losing out on work

@0xspeedybird
Copy link

This approach is not comprehensive and prevents the market dynamics promoted during the recent selection algo updates. Please review the comments on #2891.

@Titan-Node
Copy link

I would rather see a comprehensive fix than just a patch to fix the "vulnerability" today. Technically it's not actually costing the B any more funds than they would have prior to the algo change, Os can still only increase it to less than the maxPrice the B sets.

If we implement this as it stands all Os started by default that do not include the autoPriceAdjust flag will fail to work.

@eliteprox
Copy link
Contributor

I agree, we should keep the same price during the entire stream. Dropping session when the price changes isn't an acceptable solution imho.

@Titan-Node
Copy link

The Orch would need to cache the price per orchSessionID and maintain that price until that session ends

@leszko
Copy link
Contributor Author

leszko commented Oct 18, 2023

Thanks for all the comments. I think you're right. Let's wait with merging this PR until I send a fix for O to keep the price the same during the whole session.

@leszko leszko changed the title Disallow increasing O price during the streaming session Fix price per session Oct 18, 2023
@leszko
Copy link
Contributor Author

leszko commented Oct 18, 2023

Updated the PR. Now:

  • Orchestrator fixes the price per session
  • Broadcaster checks if the price was not increased

@Titan-Node @eliteprox @0xspeedybird @thomshutt PTAL.

@leszko leszko requested a review from thomshutt October 18, 2023 10:21
Copy link
Contributor

@thomshutt thomshutt left a comment

Choose a reason for hiding this comment

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

LGTM!

@leszko leszko merged commit d5cc5c8 into master Oct 19, 2023
16 checks passed
@leszko leszko deleted the rafal/fix-price-based-selection-vulnerability branch October 19, 2023 08:09
eliteprox pushed a commit to eliteprox/go-livepeer that referenced this pull request Feb 21, 2024
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.

Selection algo vulnerability + graceful shutdown feature
5 participants