Skip to content

fix(bootnode): disable req/response sync on bootnode p2p #9298

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

Conversation

danyalprout
Copy link
Contributor

Description

We noticed a lot of panics in our production bootnode logs due to nil pointer errors. This happens due to following:

  1. When request/response sync is enabled it would setup a client & server for the request response flow
  2. The server would receive a sync request, and then use the stubbed l2 client to lookup the payload by number
  3. This will return a nil error and then attempt to write the nil payload.

This PR forces request/response to be disabled -- as the bootnode doesn't support it (and logs a warning if this is not the case). It also updates the stub PayloadByNumber method to return an unsupported error to ensure that if it's used anywhere else, the caller will receive an error vs. panicing.

@danyalprout danyalprout marked this pull request as ready for review February 1, 2024 04:08
@danyalprout danyalprout requested a review from a team as a code owner February 1, 2024 04:08
Copy link
Contributor

coderabbitai bot commented Feb 1, 2024

Walkthrough

Walkthrough

The recent update introduces a significant change where the PayloadByNumber method in the l2Chain type now reliably returns an error, enhancing error handling. Additionally, there's a proactive measure in the Main function where, upon a specific condition, it disables the EnableReqRespSync feature by setting its flag to false, accompanied by a warning log. This adjustment aims at improving system stability and responsiveness under certain operational scenarios.

Changes

File(s) Change Summary
op-bootnode/bootnode/entrypoint.go - PayloadByNumber in l2Chain now returns an error instead of nil.
- Sets EnableReqRespSync to false in Main if previously true, with a warning log.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit-tests for this file.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit tests for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository from git and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit tests.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

Copy link
Contributor

@trianglesphere trianglesphere left a comment

Choose a reason for hiding this comment

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

lgtm

@tynes tynes enabled auto-merge February 2, 2024 03:05
@tynes
Copy link
Contributor

tynes commented Feb 2, 2024

Maybe this needs a rebase for it to pick up shell-check as the name of the step in CI changed recently

auto-merge was automatically disabled February 4, 2024 03:53

Head branch was pushed to by a user without write access

@danyalprout danyalprout force-pushed the bootnode-disable-sync-req-resp branch from 609782b to 381d4f9 Compare February 4, 2024 03:53
Copy link

codecov bot commented Feb 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (06d0642) 27.86% compared to head (381d4f9) 40.15%.

Additional details and impacted files
@@             Coverage Diff              @@
##           develop    #9298       +/-   ##
============================================
+ Coverage    27.86%   40.15%   +12.28%     
============================================
  Files          167       79       -88     
  Lines         7357     5068     -2289     
  Branches      1271      735      -536     
============================================
- Hits          2050     2035       -15     
+ Misses        5186     2912     -2274     
  Partials       121      121               
Flag Coverage Δ
cannon-go-tests 62.56% <ø> (ø)
chain-mon-tests 27.14% <ø> (ø)
common-ts-tests 26.74% <ø> (ø)
contracts-bedrock-tests ?
contracts-ts-tests 12.25% <ø> (ø)
core-utils-tests 44.03% <ø> (ø)
sdk-next-tests 41.52% <ø> (ø)
sdk-tests 41.52% <ø> (ø)

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

see 88 files with indirect coverage changes

@danyalprout
Copy link
Contributor Author

Thanks @tynes, just rebased 👍

@mslipper mslipper added this pull request to the merge queue Feb 4, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 4, 2024
@mslipper mslipper added this pull request to the merge queue Feb 4, 2024
Merged via the queue into ethereum-optimism:develop with commit e4b6585 Feb 4, 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.

4 participants