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

Initialize round by any B/O who has the initializeRound flag set to true #3029

Merged
merged 3 commits into from
Apr 24, 2024

Conversation

leszko
Copy link
Contributor

@leszko leszko commented Apr 22, 2024

AFAIU the current logic for the -initializeRound flag has the assumption that every O has this flag set to true. Then, it uses some calculation to decide which O sends the tx to initialize the round.

The assumption is not true and with most O having -initializeRound false we end up with the round not being initialized. This PR removes all this logic for checking who should intialize the round and just any B/O that has this flag enabled will try to initialize the round. The worst that can happen is that a lot of B/Os will try to initialize the round, but the gas price is low, so that's not too bad.

After this PR gets merged I'll set it to true in all Bs in staging for Livepeer Inc.

@leszko leszko requested a review from victorges April 22, 2024 14:54
Copy link

codecov bot commented Apr 22, 2024

Codecov Report

Attention: Patch coverage is 12.50000% with 42 lines in your changes are missing coverage. Please review.

Project coverage is 57.32866%. Comparing base (9305333) to head (5ae9efe).

Additional details and impacted files

Impacted file tree graph

@@                 Coverage Diff                 @@
##              master       #3029         +/-   ##
===================================================
- Coverage   57.48217%   57.32866%   -0.15351%     
===================================================
  Files             92          92                 
  Lines          15704       15685         -19     
===================================================
- Hits            9027        8992         -35     
- Misses          6077        6091         +14     
- Partials         600         602          +2     
Files Coverage Δ
cmd/livepeer/livepeer.go 50.00000% <100.00000%> (+0.33557%) ⬆️
eth/roundinitializer.go 75.40984% <35.71429%> (-17.36124%) ⬇️
cmd/livepeer/starter/starter.go 7.17897% <0.00000%> (-0.01455%) ⬇️

... and 1 file with indirect coverage changes


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 9305333...5ae9efe. Read the comment docs.

Files Coverage Δ
cmd/livepeer/livepeer.go 50.00000% <100.00000%> (+0.33557%) ⬆️
eth/roundinitializer.go 75.40984% <35.71429%> (-17.36124%) ⬇️
cmd/livepeer/starter/starter.go 7.17897% <0.00000%> (-0.01455%) ⬇️

... and 1 file with indirect coverage changes

Copy link
Member

@victorges victorges left a comment

Choose a reason for hiding this comment

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

LGTM

I have a suggestion for a logic to avoid so many failed transactions onchain every day, but if you prefer to continue with the current version it's fine with me. I'd just request for us not to enable the -initializeRound flag on all of our Bs, otherwise it's gonna be a lot of pollution onchain.

eth/roundinitializer.go Show resolved Hide resolved
Copy link
Member

@victorges victorges left a comment

Choose a reason for hiding this comment

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

LGTM!

If you're merging this without squash keep in mind that there's a duplicate commit (msg) in the history (I just prefer squash myself)

eth/roundinitializer.go Show resolved Hide resolved
@leszko leszko merged commit e9cbadb into master Apr 24, 2024
18 checks passed
@leszko leszko deleted the rafal/intialize-round-on-b branch April 24, 2024 12:49
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