feat: add script to configure v3 #4022
Conversation
evan-forbes
added
WS: V3 3️⃣
evan-forbes
requested review from
rootulp,
cmwaters,
staheri14,
rach-id and
ninabarbakadze
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
docs/release-notes/release-notes.md (4)
11-18: Improve clarity and flow of BBR instructions.The instructions for enabling BBR could be more polished. Consider this revision:
-Consensus node operators must enable the BBR (Bottleneck Bandwidth and Round-trip propagation time) congestion control algorithm. See [#3774](https://github.com/celestiaorg/celestia-app/pull/3774). -if using linux in docker, kubernetes, a vm or baremetal, this can be done by calling - -```sh -make enable-bbr -``` - -command on the host machine. +Consensus node operators must enable the BBR (Bottleneck Bandwidth and Round-trip propagation time) congestion control algorithm. See [#3774](https://github.com/celestiaorg/celestia-app/pull/3774). + +For Linux environments (Docker, Kubernetes, VM, or bare metal), run the following command on the host machine: + +```sh +make enable-bbr +```🧰 Tools
🪛 Markdownlint
12-12: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
28-38: Add cautionary note about configuration impact.The configuration instructions should mention the impact of these changes on node operation.
Consider adding a note:
+> Note: These configuration changes will affect your node's network performance and resource usage. Ensure you have adequate system resources before applying these changes. +
40-46: Improve manual configuration section structure and grammar.The manual configuration section needs better formatting and grammar fixes.
-**Alternatively**, the configurations can be changed manually. This involves updating the mempool TTLs and the send and the receive rates. - -- Configuring Bandwidth Settings - - update `recv_rate` and `send_rate` in your TOML config file to 10MiB (10485760). -- Extend TTLs - - update `ttl-num-blocks` in your TOML config file to 12. +**Manual Configuration Alternative** + +The following settings must be updated in your TOML configuration file: + +1. Bandwidth Settings: + - Set `recv_rate` to 10MiB (10485760) + - Set `send_rate` to 10MiB (10485760) + +2. Mempool Settings: + - Set `ttl-num-blocks` to 12🧰 Tools
🪛 LanguageTool
[grammar] ~40-~40: Did you mean the noun “reception” (= the act of receiving) or “receipt” (= invoice)?
Context: ...g the mempool TTLs and the send and the receive rates. - Configuring Bandwidth Setting...(PREPOSITION_VERB)
47-47: Enhance upgrade signaling instructions.The upgrade signaling section could be more informative.
Consider these improvements:
#### Signaling Upgrades + +> Important: Only validators in the active set can participate in upgrade signaling. The upgrade requires approval from validators representing at least 83.33% (5/6) of the total voting power. - Upgrades now use the `x/signal` module to coordinate the network to an upgrade height. The following command can be used, if you are a validator in the active set, to signal to upgrade to v3 ```bash -celestia-appd tx signal signal 3 <plus transaction flags> +celestia-appd tx signal signal 3 --from=<validator-key> --chain-id=<chain-id> --gas=auto --gas-adjustment=1.3</blockquote></details> <details> <summary>app/default_overrides.go (2)</summary><blockquote> `277-279`: **LGTM! Consider extracting the rate constant.** The P2P rate limiting configuration is a good addition for network stability and DoS prevention. However, consider extracting the magic number `10` into a named constant for better maintainability. ```diff const mebibyte = 1048576 +const defaultP2PRate = 10 // MiB/s - cfg.P2P.SendRate = 10 * mebibyte - cfg.P2P.RecvRate = 10 * mebibyte + cfg.P2P.SendRate = defaultP2PRate * mebibyte + cfg.P2P.RecvRate = defaultP2PRate * mebibyte
277-279: Document the rationale for the chosen rate limits.Consider adding a comment explaining why 10 MiB/s was chosen as the default rate limit. This helps future maintainers understand the reasoning behind these values and when they might need adjustment.
const mebibyte = 1048576 +// defaultP2PRate defines the default send/receive rate limits for P2P communication. +// 10 MiB/s provides a balance between network performance and resource consumption. cfg.P2P.SendRate = 10 * mebibyte cfg.P2P.RecvRate = 10 * mebibyte
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
Makefile(1 hunks)app/default_overrides.go(1 hunks)docs/release-notes/release-notes.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/release-notes/release-notes.md
[grammar] ~40-~40: Did you mean the noun “reception” (= the act of receiving) or “receipt” (= invoice)?
Context: ...g the mempool TTLs and the send and the receive rates. - Configuring Bandwidth Setting...
(PREPOSITION_VERB)
🪛 Markdownlint
docs/release-notes/release-notes.md
12-12: Expected: 0 or 2; Actual: 1
Trailing spaces
(MD009, no-trailing-spaces)
🔇 Additional comments (2)
Makefile (2)
313-313: LGTM!
The target is correctly marked as PHONY since it doesn't produce a file output.
304-305: Add validation for config file accessibility
The variables are well-defined, but the script should validate:
- If HOME environment variable is set
- If the config file exists
- If we have write permissions to modify the file
Consider adding these checks to the configure-v3 target:
CONFIG_FILE ?= ${HOME}/.celestia-app/config/config.toml
SEND_RECV_RATE ?= 10485760 # 10 MiB
+
+# Helper to check config file
+check-config-file:
+ @if [ -z "$(HOME)" ]; then \
+ echo "ERROR: HOME environment variable is not set"; \
+ exit 1; \
+ fi
+ @if [ ! -f "$(CONFIG_FILE)" ]; then \
+ echo "ERROR: Config file not found at: $(CONFIG_FILE)"; \
+ exit 1; \
+ fi
+ @if [ ! -w "$(CONFIG_FILE)" ]; then \
+ echo "ERROR: No write permission for: $(CONFIG_FILE)"; \
+ exit 1; \
+ fi
📝 WalkthroughWalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
docs/release-notes/release-notes.md (4)
12-12: Remove trailing space.There is a trailing space at the end of this line.
-if using linux in docker, kubernetes, a vm or baremetal, this can be done by calling +if using linux in docker, kubernetes, a vm or baremetal, this can be done by calling🧰 Tools
🪛 Markdownlint
12-12: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
20-24: Consider adding explanation for MCTCP.While BBR is well explained with a link to the PR, MCTCP lacks context. Consider adding a brief explanation of what MCTCP is and its benefits, along with a reference PR if available.
40-40: Improve sentence structure.The current wording is slightly awkward.
-**Alternatively**, the configurations can be changed manually. This involves updating the mempool TTLs and the send and the receive rates. +**Alternatively**, the configurations can be changed manually. This involves updating the mempool TTLs, send rates, and receive rates.🧰 Tools
🪛 LanguageTool
[grammar] ~40-~40: Did you mean the noun “reception” (= the act of receiving) or “receipt” (= invoice)?
Context: ...g the mempool TTLs and the send and the receive rates. - Configuring Bandwidth Setting...(PREPOSITION_VERB)
42-45: Consider adding rationale for configuration values.While the specific values are clearly stated, it would be helpful to explain why these particular values (10MiB for bandwidth settings and 12 blocks for TTL) were chosen. This context would help operators understand the impact of these changes.
app/default_overrides.go (1)
277-279: LGTM! Consider adding documentation for the rate limits.The P2P rate configuration of 10 MiB/s for both send and receive is reasonable for a blockchain network. The symmetric configuration will help maintain balanced network performance.
Consider adding a comment explaining:
- Why these specific rate limits were chosen
- The impact on node operators (bandwidth requirements)
- Whether these limits are adjustable post-deployment
Makefile (1)
304-305: Consider improving variable declarations for robustnessA few suggestions to enhance the variable declarations:
- Add a fallback for CONFIG_FILE when HOME is not set
- Update the comment to specify "10 MiB/s" since it's a rate
- Add validation to ensure SEND_RECV_RATE is positive
-CONFIG_FILE ?= ${HOME}/.celestia-app/config/config.toml -SEND_RECV_RATE ?= 10485760 # 10 MiB +CONFIG_FILE ?= ${HOME:-${USERPROFILE}}/.celestia-app/config/config.toml +SEND_RECV_RATE ?= 10485760 # 10 MiB/s + +# Validate SEND_RECV_RATE is positive +ifeq ($(shell echo "$(SEND_RECV_RATE) <= 0" | bc),1) + $(error SEND_RECV_RATE must be positive) +endif
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
Makefile(1 hunks)app/default_overrides.go(1 hunks)docs/release-notes/release-notes.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/release-notes/release-notes.md
[grammar] ~40-~40: Did you mean the noun “reception” (= the act of receiving) or “receipt” (= invoice)?
Context: ...g the mempool TTLs and the send and the receive rates. - Configuring Bandwidth Setting...
(PREPOSITION_VERB)
🪛 Markdownlint
docs/release-notes/release-notes.md
12-12: Expected: 0 or 2; Actual: 1
Trailing spaces
(MD009, no-trailing-spaces)
🔇 Additional comments (3)
docs/release-notes/release-notes.md (1)
47-47: Verify the documentation link.
The relative link to the signal module documentation (../../x/signal/README.md) should be verified to ensure it resolves correctly in the published documentation.
app/default_overrides.go (1)
Line range hint 290-291: Verify state sync parameters and enhance documentation.
The snapshot configuration looks reasonable, but let's ensure it's optimal for v3:
- SnapshotInterval of 1500 blocks provides good availability without excessive resource usage
- Keeping 2 recent snapshots balances disk usage with redundancy
Let's verify if these values align with the network's parameters:
Consider adding comments explaining:
- The estimated time between snapshots based on block time
- Disk space requirements for keeping 2 snapshots
- Recommendations for nodes with different resource constraints
✅ Verification successful
Based on the shell script results, I can now provide a final response:
State sync parameters are well-aligned with network parameters
The snapshot interval of 1500 blocks is appropriate given:
- Network's
GoalBlockTimeis 15 seconds (pkg/appconsts/consensus_consts.go) - V3's
TimeoutCommitis 4.2 seconds (pkg/appconsts/v3/app_consts.go) - This means snapshots occur approximately every 6.25 hours, providing a good balance between availability and resource usage
The configuration values are consistently used across the codebase and properly tested (app/default_overrides_test.go).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if snapshot interval is appropriate given the block time
# Expected: Should find block time references to confirm if 1500 blocks is a reasonable interval
# Search for block time constants
echo "Checking block time constants:"
rg -A 2 "GoalBlockTime|TimeoutCommit|TimeoutPropose" --type go
# Search for any existing state sync configurations
echo "Checking existing state sync configurations:"
rg "SnapshotInterval|SnapshotKeepRecent" --type go
Length of output: 12454
Makefile (1)
313-313: LGTM!
The PHONY declaration is correctly implemented for the configure-v3 target.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
docs/release-notes/release-notes.md (3)
11-18: Enhance BBR configuration instructions.Consider adding more context about:
- Prerequisites for running the
make enable-bbrcommand (e.g., root privileges required)- Expected outcome or verification steps
- What happens if BBR is not enabled
Also, remove the trailing space after "this can be done by calling" on line 12.
🧰 Tools
🪛 Markdownlint
12-12: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
34-40: Improve manual configuration instructions.
- Rephrase line 34 for better clarity:
-This involves updating the mempool TTLs and the send and the receive rates. +This involves updating the mempool TTLs, send rates, and receive rates.
- Consider adding:
- Explanation of why these values were chosen
- Impact of these settings on node performance
- Verification steps to confirm successful configuration
🧰 Tools
🪛 LanguageTool
[grammar] ~34-~34: Did you mean the noun “reception” (= the act of receiving) or “receipt” (= invoice)?
Context: ...g the mempool TTLs and the send and the receive rates. - Configuring Bandwidth Setting...(PREPOSITION_VERB)
Line range hint
41-67: Enhance upgrade signaling documentation.Consider adding:
- Example of complete command with transaction flags:
celestia-appd tx signal signal 3 --from=<key_name> --chain-id=<chain_id> --gas=auto
- Specify the exact duration of the hard-coded delay between confirmation and execution
🧰 Tools
🪛 LanguageTool
[grammar] ~34-~34: Did you mean the noun “reception” (= the act of receiving) or “receipt” (= invoice)?
Context: ...g the mempool TTLs and the send and the receive rates. - Configuring Bandwidth Setting...(PREPOSITION_VERB)
🪛 Markdownlint
12-12: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
Makefile (1)
307-317: Document the configure-v3 target in the help sectionAdd documentation for the new target in the help section to improve discoverability.
+## configure-v3: Configure node settings for v3 upgrade by updating recv_rate, send_rate, and ttl-num-blocks. configure-v3:
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
Makefile(1 hunks)docs/release-notes/release-notes.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/release-notes/release-notes.md
[grammar] ~34-~34: Did you mean the noun “reception” (= the act of receiving) or “receipt” (= invoice)?
Context: ...g the mempool TTLs and the send and the receive rates. - Configuring Bandwidth Setting...
(PREPOSITION_VERB)
🪛 Markdownlint
docs/release-notes/release-notes.md
12-12: Expected: 0 or 2; Actual: 1
Trailing spaces
(MD009, no-trailing-spaces)
🔇 Additional comments (1)
Makefile (1)
307-317: Verify the impact of configuration changes
The changes to recv_rate, send_rate, and ttl-num-blocks parameters could affect node performance and network stability. Let's verify the usage of these parameters in the codebase.
✅ Verification successful
Configuration changes are aligned with project standards
The changes to recv_rate, send_rate, and ttl-num-blocks in the Makefile are consistent with the project's standards and existing implementations:
- The
ttl-num-blocks = 12change matches the default value set inapp/default_overrides.go - The send/receive rate modifications align with the 10 MiB bandwidth setting documented in release notes and implemented in
app/default_overrides.go - These parameters are properly documented in the project's release notes, indicating they are part of planned configuration updates
The changes are safe and follow the established configuration patterns in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage and impact of configuration parameters
# Check for references to recv_rate and send_rate
echo "Checking usage of rate parameters..."
rg -A 5 "recv_rate|send_rate" --type go
# Check for references to ttl-num-blocks
echo "Checking usage of ttl-num-blocks..."
rg -A 5 "ttl-num-blocks" --type go
# Look for any performance-related tests that might need updating
echo "Checking for related test files..."
fd -e go -e yaml -e toml -E "vendor/*" -E "build/*" -x grep -l "recv_rate\|send_rate\|ttl-num-blocks" {}
Length of output: 468
Script:
#!/bin/bash
# Let's check the actual content of the config file and search for any documentation
echo "Checking config file content..."
cat ./local_devnet/celestia-app/config.toml
# Search for any documentation about these parameters
echo "Searching for documentation..."
rg -i "recv_rate|send_rate|ttl-num-blocks" --type md
# Look for any performance or network related code that might be affected
echo "Checking network related code..."
rg -A 5 "RecvRate|SendRate|TTLNumBlocks" --type go
# Check for any test configurations
echo "Checking test configurations..."
fd -e toml -e yaml -x grep -l "recv_rate\|send_rate\|ttl-num-blocks" {}
Length of output: 22747
Overview
adds a script and better docs for configuring everything for v3