feat: require bbr by default #3774
Conversation
evan-forbes
added
backport:v1.x
evan-forbes
requested review from
cmwaters and
ninabarbakadze
and removed request for
a team
WalkthroughWalkthroughThe recent changes enhance the Changes
Assessment against linked issues
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (2)
Files skipped from review as they are similar to previous changes (2)
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? TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Additionally, you can add CodeRabbit Configuration File (
|
| err := checkBBR(cmd) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
besides adding imports and not exporting a few functions that previously were, this is the only change to the copy paste forked code 🙃
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (2)
cmd/celestia-appd/cmd/start.go (2)
3-4: Clarify the rationale for copying and forking code.The note mentions that the file was copied and forked from the SDK. Ensure this decision is documented clearly, and consider the long-term maintainability.
- // NOTE: This file was copy paste forked from the sdk in order to modify the + // NOTE: This file was copied and forked from the SDK to modify the
55-55: Document the new flagFlagForceNoBBR.The
FlagForceNoBBRallows bypassing the BBR requirement. Ensure this is well-documented for users who might need to disable the BBR check.+ // This flag allows users to bypass the BBR requirement if necessary.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
I think this will need a corresponding PR to https://github.com/celestiaorg/docs to instruct users to enable BBR or provide the --force-no-bbr flag.
This is a breaking change with respect to how user's interact with the binary. It's fine to include in v3.x but we should consider whether we want to backport to v1.x or v2.x.
| ) | ||
| } | ||
|
|
||
| // checkBBR checks is bbr is configured to be used as a congestion control algo. |
There was a problem hiding this comment.
| // checkBBR checks is bbr is configured to be used as a congestion control algo. | |
| // checkBBR checks if BBR is configured to be used as a congestion control algorithm. |
| // The BBR congestion control algorithm does not appear to be enabled in this | ||
| // system's kernel. This is important for the p2p stack to be performant. | ||
| // | ||
| // to enable bbr call: |
There was a problem hiding this comment.
| // to enable bbr call: | |
| // To enable BBR call: |
Co-authored-by: Rootul P <rootulp@gmail.com>
|
I think this should just be in V3. We could do a patch release for v2 but it will require extra communication to tell node operators of the change and hopefully v3 will shortly be out |
celestia-bot
requested review from
a team and
staheri14
and removed request for
a team
evan-forbes
removed
backport:v1.x
This PR adds a requirement to use bbr by default. This requirement can be bypassed by using a flag. Warnings are returned when installing, and there is a makefile command that will configure the system to use bbr. Most of this PR is copying over the start command from the sdk. Alternatively, we could simply modify the sdk. That would not require copy past forking, but the trend has been that every change that we make to the sdk eventually gets moved here, so I figured we should just skip to doing that. This also gives us finer grained control in the future. I'm definitely not tied to this approach, and if we'd rather change our fork, then I'm happy go do that instead. closes #3745 --------- Co-authored-by: Rootul P <rootulp@gmail.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This reverts commit b948c08.
This reverts commit 2bed8f8.
| @@ -102,7 +102,8 @@ startCelestiaApp() { | |||
| --api.enable \ | |||
| --grpc.enable \ | |||
| --grpc-web.enable \ | |||
| --v2-upgrade-height 3 | |||
| --v2-upgrade-height 3 \ | |||
| --force-no-bbr // no need to require BBR usage on a local node | |||
There was a problem hiding this comment.
I believe it's not a valid bash syntax the actual behavior is not what's expected here
I assume it was meant to be a comment, so, # for comments
| // figure out how to enable bbr for your system. | ||
| // | ||
| // While this node will get worse performance using something other than bbr, | ||
| // If you need to bypass this block use the "--force-no-bbr true" flag. |
There was a problem hiding this comment.
depending on the parser, --x true can give different results, i.e. it will probably be parsed as --x and true as a positional argument
if you want to keep explicit value for it, then it should be --force-no-bbr=true, if it's supported by the args parser
Overview
This PR adds a requirement to use bbr by default. This requirement can be bypassed by using a flag. Warnings are returned when installing, and there is a makefile command that will configure the system to use bbr.
~~~ Note for Reviewers ~~~
Most of this PR is copying over the start command from the sdk. Alternatively, we could simply modify the sdk. That would not require copy past forking, but the trend has been that every change that we make to the sdk eventually gets moved here, so I figured we should just skip to doing that. This also gives us finer grained control in the future. I'm definitely not tied to this approach, and if we'd rather change our fork, then I'm happy go do that instead.
closes #3745