Skip to content

feat(op-batcher): wait for pre-genesis RPC request loop#12383

Merged
protolambda merged 7 commits intoethereum-optimism:developfrom
jsvisa:op-batcher-pre-genesis
Oct 17, 2024
Merged

feat(op-batcher): wait for pre-genesis RPC request loop#12383
protolambda merged 7 commits intoethereum-optimism:developfrom
jsvisa:op-batcher-pre-genesis

Conversation

@jsvisa
Copy link
Copy Markdown
Contributor

@jsvisa jsvisa commented Oct 9, 2024

Description

closes #11630

Tests

Additional context

Metadata

jsvisa added 2 commits October 9, 2024 04:57
@jsvisa
feat(op-batcher): loop fetch sync status
e8256af 
Signed-off-by: jsvisa <delweng@gmail.com>
@jsvisa
feat(op-batcher): wait for l2 genesis time
152d7cb 
Signed-off-by: jsvisa <delweng@gmail.com>
@jsvisa jsvisa requested review from a team and bitwiseguy October 9, 2024 04:59
@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 9, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.14%. Comparing base (fb62380) to head (d8dcfc5).
Report is 57 commits behind head on develop.

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop   #12383      +/-   ##
===========================================
- Coverage    64.32%   64.14%   -0.19%     
===========================================
  Files           52       52              
  Lines         4348     4348              
===========================================
- Hits          2797     2789       -8     
- Misses        1376     1385       +9     
+ Partials       175      174       -1
Flag Coverage Δ
cannon-go-tests 64.14% <ø> (-0.19%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1 file with indirect coverage changes

@jsvisa
Copy link
Copy Markdown
Contributor Author

jsvisa commented Oct 9, 2024

@protolambda PTAL

@tynes
Copy link
Copy Markdown
Contributor

tynes commented Oct 11, 2024

Thank you for the PR @jsvisa !

tynes
tynes reviewed Oct 11, 2024
View reviewed changes
Comment thread op-batcher/batcher/driver.go Outdated
@jsvisa
feat(batcher): add remaining time for tick printing
2bb6959 
Signed-off-by: jsvisa <delweng@gmail.com>
protolambda
protolambda reviewed Oct 14, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@protolambda protolambda 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! Thank you for implementing this change. Two small nits, then happy to merge. In terms of testing I think it's covered well enough in op-e2e, almost every test already starts the batcher and awaits genesis (depending on genesis timing).

Comment thread op-batcher/batcher/driver.go Outdated
Comment thread op-batcher/batcher/driver.go Outdated
@jsvisa
apply code reviews
8fbea42 
Signed-off-by: jsvisa <delweng@gmail.com>
@protolambda protolambda added this pull request to the merge queue Oct 14, 2024
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 14, 2024
@protolambda protolambda added this pull request to the merge queue Oct 14, 2024
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 14, 2024
tynes
tynes reviewed Oct 14, 2024
View reviewed changes
Comment thread op-batcher/batcher/driver.go Outdated
@tynes
Copy link
Copy Markdown
Contributor

tynes commented Oct 14, 2024

small nit would be ok without it

@jsvisa
use min instead of if test
46fd51b 
Signed-off-by: jsvisa <delweng@gmail.com>
@ajsutton
Copy link
Copy Markdown
Contributor

ajsutton commented Oct 15, 2024

I think semgrep is failing when this gets updated with the latest changes from develop: https://app.circleci.com/pipelines/github/ethereum-optimism/optimism/68416/workflows/7f921a39-67d0-4347-a696-ed7d359aa0f4/jobs/2831634

@tynes
Copy link
Copy Markdown
Contributor

tynes commented Oct 15, 2024 

    op-batcher/batcher/driver.go
   ❯❯❱ dgryski.semgrep-go.timeafter.leaky-time-after
          Leaky use of time.After in for-select, see: https://groups.google.com/g/golang-
          nuts/c/cCdm0Ixwi9A/m/jMiJJScAEAAJ                                              
          Details: https://sg.run/dgrQ                                                   
                                                                                         
          310┆ for {
          311┆   cCtx, cancel := context.WithTimeout(ctx, l.Config.NetworkTimeout)
          312┆   syncStatus, err = rollupClient.SyncStatus(cCtx)
          313┆   cancel()
          314┆ 
          315┆   // Ensure that we have the sync status
          316┆   if err != nil {
          317┆           return eth.BlockID{}, eth.BlockID{}, fmt.Errorf("failed to get sync status: %w",
               err)                                                                                      
          318┆   }
          319┆ 
             [hid 17 additional lines, adjust with --max-lines-per-finding] 
                            
  BLOCKING CODE RULES FIRED:
    dgryski.semgrep-go.timeafter.leaky-time-after

@jsvisa
fix(batcher): reset timer
d8dcfc5 
Signed-off-by: jsvisa <delweng@gmail.com>
@jsvisa
Copy link
Copy Markdown
Contributor Author

jsvisa commented Oct 17, 2024

I think semgrep is failing when this gets updated with the latest changes from develop: https://app.circleci.com/pipelines/github/ethereum-optimism/optimism/68416/workflows/7f921a39-67d0-4347-a696-ed7d359aa0f4/jobs/2831634

thanks for the info, we did leaked the timer, now use timer.Reset to reuse the timer

@protolambda protolambda added this pull request to the merge queue Oct 17, 2024
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 17, 2024
@protolambda protolambda enabled auto-merge October 17, 2024 14:37
@protolambda protolambda disabled auto-merge October 17, 2024 14:52
@protolambda protolambda enabled auto-merge October 17, 2024 14:53
@protolambda protolambda added this pull request to the merge queue Oct 17, 2024
Merged via the queue into ethereum-optimism:develop with commit 7ce9165 Oct 17, 2024
@jsvisa jsvisa deleted the op-batcher-pre-genesis branch October 22, 2024 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tynes tynes tynes left review comments

@bitwiseguy bitwiseguy Awaiting requested review from bitwiseguy

+1 more reviewer

@protolambda protolambda protolambda approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

op-batcher: fix pre-genesis RPC request loop

4 participants

@jsvisa @tynes @ajsutton @protolambda