docker_cluster.sh: support N>4 validators on private chains#7
Open
Taranpreet26311 wants to merge 2 commits intomainfrom
Open
docker_cluster.sh: support N>4 validators on private chains#7Taranpreet26311 wants to merge 2 commits intomainfrom
Taranpreet26311 wants to merge 2 commits intomainfrom
Conversation
Add patch_for_private_chain() that pushes Plato/Luban to block 100M and disables post-Plato time-based forks for genesis. Required for private chains with more validators than the default 4-validator setup, where fast-finality (BEP-126) panics during reorg at parlia/snapshot.go:411 when DoubleSign forks occur at block 2 due to multi-validator startup race conditions on a WAN. Also sort validators by consensusAddr ascending in validators.template so genesis extraData matches Parlia's snapshot.validators() ordering (otherwise block 1 sealing fails with "unauthorized validator" because in-turn calculation uses sorted-ascending while extraData was unsorted). Replace forge install --no-git with direct git clone of forge-std, because forge install fails when the parent directory is itself a git repo (submodule path lookup fails for forge-std's ds-test). Toggle: DISABLE_FAST_FINALITY=true (default) applies the patches. Set DISABLE_FAST_FINALITY=false in .env to preserve upstream 4-validator behavior. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughThis change adds two new functions to docker_cluster.sh: prepare_bsc_client() to optionally clone/build/install a local BSC geth binary, and patch_for_private_chain() to conditionally modify genesis-template.json and validators.template when DISABLE_FAST_FINALITY is true. The reset_genesis flow was changed to clone forge-std (v1.7.3) and ds-test directly instead of using forge install. The prepare command sequence now runs patch_for_private_chain between reset_genesis and prepare_config. Sequence Diagram(s)sequenceDiagram
participant User
participant Script as docker_cluster.sh
participant GitHub as "Remote Git Repos"
participant Builder as "go build"
participant FS as "Local filesystem (genesis/templates)"
User->>Script: run "prepare"
Script->>GitHub: clone lib/forge-std (v1.7.3) & ds-test
GitHub-->>Script: repo contents
Script->>FS: create/reset genesis templates
alt prepare_bsc_client requested
Script->>GitHub: clone bsc repository / checkout branch
GitHub-->>Script: bsc source
Script->>Builder: build geth binary
Builder-->>Script: built geth
Script->>FS: install local geth binary
end
Script->>Script: patch_for_private_chain (if DISABLE_FAST_FINALITY)
Script->>FS: modify genesis-template.json (push forks far future / null time forks)
Script->>FS: modify validators.template (optional sort by consensusAddr)
Script->>Script: prepare_config (generate final configs)
Script-->>User: prepare complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 6 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docker_cluster.sh`:
- Around line 117-121: The script currently uses an unquoted $VT and a fragile
sed insertion into validators.template targeting the literal "function
extraDataSerialize(validators) {"; fix by quoting the variable references (use
"$VT") and replace the brittle sed-based injection with a more robust patch
strategy: either apply a separate patch file (e.g., validators.template.patch)
and use patch or git apply, or perform the transformation with a small Node or
jq script that locates the extraDataSerialize function (by name) and inserts the
"// SORT_PATCHED" + sort line into its body; ensure you still check for the "//
SORT_PATCHED" marker before patching so the idempotent check using "$VT" and the
marker remains.
- Around line 98-111: The sed/grep commands use an unquoted variable $TPL
(risking globbing/word-splitting) and only verify the "lubanBlock" replacement,
allowing other sed failures to go unnoticed; update all sed and grep invocations
to double-quote "$TPL" and add at least one more verification grep (e.g., check
for '"platoBlock": 100000000') after the sed sequence so failures in sed lines
for "platoBlock" (and similar keys like "berlinBlock", "londonBlock", etc.) are
detected and cause a non-zero exit; locate the sed calls and the existing grep
check in the docker_cluster.sh patch_for_private_chain section and apply these
changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: getoptimum/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b62211b9-f835-4ef5-b882-ff9e095a6b91
📒 Files selected for processing (1)
docker_cluster.sh
Comment on lines
+98
to
+111
| sed -i 's/"lubanBlock": 6,/"lubanBlock": 100000000,/' $TPL | ||
| sed -i 's/"platoBlock": 7,/"platoBlock": 100000000,/' $TPL | ||
| sed -i 's/"berlinBlock": 8,/"berlinBlock": 100000001,/' $TPL | ||
| sed -i 's/"londonBlock": 8,/"londonBlock": 100000001,/' $TPL | ||
| sed -i 's/"hertzBlock": 8,/"hertzBlock": 100000002,/' $TPL | ||
| sed -i 's/"hertzfixBlock": 8,/"hertzfixBlock": 100000002,/' $TPL | ||
| sed -i 's/"bohrTime": 0,/"bohrTime": null,/' $TPL | ||
| sed -i 's/"pascalTime": 0,/"pascalTime": null,/' $TPL | ||
| sed -i 's/"pragueTime": 0,/"pragueTime": null,/' $TPL | ||
| sed -i 's/"lorentzTime": 0,/"lorentzTime": null,/' $TPL | ||
| sed -i 's/"maxwellTime": 0,/"maxwellTime": null,/' $TPL | ||
| sed -i 's/"fermiTime": 0,/"fermiTime": null,/' $TPL | ||
| sed -i 's/"haberFixTime": 0,/"haberFixTime": null,/' $TPL | ||
| grep -q '"lubanBlock": 100000000' $TPL || { echo "patch_for_private_chain: luban patch failed" >&2; exit 1; } |
There was a problem hiding this comment.
Quote variables and verify more patches to catch silent failures.
Two issues:
-
$TPLis unquoted in all sed/grep commands. Per shellcheck SC2086, double-quote to prevent globbing/word splitting. -
Only
lubanBlockis verified (line 111). If any other sed pattern doesn't match (e.g., different whitespace in template), the patch silently fails. At minimum, verifyplatoBlocksince it's the primary fast-finality gate.
🛠️ Proposed fix
- sed -i 's/"lubanBlock": 6,/"lubanBlock": 100000000,/' $TPL
- sed -i 's/"platoBlock": 7,/"platoBlock": 100000000,/' $TPL
- sed -i 's/"berlinBlock": 8,/"berlinBlock": 100000001,/' $TPL
- sed -i 's/"londonBlock": 8,/"londonBlock": 100000001,/' $TPL
- sed -i 's/"hertzBlock": 8,/"hertzBlock": 100000002,/' $TPL
- sed -i 's/"hertzfixBlock": 8,/"hertzfixBlock": 100000002,/' $TPL
- sed -i 's/"bohrTime": 0,/"bohrTime": null,/' $TPL
- sed -i 's/"pascalTime": 0,/"pascalTime": null,/' $TPL
- sed -i 's/"pragueTime": 0,/"pragueTime": null,/' $TPL
- sed -i 's/"lorentzTime": 0,/"lorentzTime": null,/' $TPL
- sed -i 's/"maxwellTime": 0,/"maxwellTime": null,/' $TPL
- sed -i 's/"fermiTime": 0,/"fermiTime": null,/' $TPL
- sed -i 's/"haberFixTime": 0,/"haberFixTime": null,/' $TPL
- grep -q '"lubanBlock": 100000000' $TPL || { echo "patch_for_private_chain: luban patch failed" >&2; exit 1; }
+ sed -i 's/"lubanBlock": 6,/"lubanBlock": 100000000,/' "$TPL"
+ sed -i 's/"platoBlock": 7,/"platoBlock": 100000000,/' "$TPL"
+ sed -i 's/"berlinBlock": 8,/"berlinBlock": 100000001,/' "$TPL"
+ sed -i 's/"londonBlock": 8,/"londonBlock": 100000001,/' "$TPL"
+ sed -i 's/"hertzBlock": 8,/"hertzBlock": 100000002,/' "$TPL"
+ sed -i 's/"hertzfixBlock": 8,/"hertzfixBlock": 100000002,/' "$TPL"
+ sed -i 's/"bohrTime": 0,/"bohrTime": null,/' "$TPL"
+ sed -i 's/"pascalTime": 0,/"pascalTime": null,/' "$TPL"
+ sed -i 's/"pragueTime": 0,/"pragueTime": null,/' "$TPL"
+ sed -i 's/"lorentzTime": 0,/"lorentzTime": null,/' "$TPL"
+ sed -i 's/"maxwellTime": 0,/"maxwellTime": null,/' "$TPL"
+ sed -i 's/"fermiTime": 0,/"fermiTime": null,/' "$TPL"
+ sed -i 's/"haberFixTime": 0,/"haberFixTime": null,/' "$TPL"
+ grep -q '"lubanBlock": 100000000' "$TPL" || { echo "patch_for_private_chain: luban patch failed" >&2; exit 1; }
+ grep -q '"platoBlock": 100000000' "$TPL" || { echo "patch_for_private_chain: plato patch failed" >&2; exit 1; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| sed -i 's/"lubanBlock": 6,/"lubanBlock": 100000000,/' $TPL | |
| sed -i 's/"platoBlock": 7,/"platoBlock": 100000000,/' $TPL | |
| sed -i 's/"berlinBlock": 8,/"berlinBlock": 100000001,/' $TPL | |
| sed -i 's/"londonBlock": 8,/"londonBlock": 100000001,/' $TPL | |
| sed -i 's/"hertzBlock": 8,/"hertzBlock": 100000002,/' $TPL | |
| sed -i 's/"hertzfixBlock": 8,/"hertzfixBlock": 100000002,/' $TPL | |
| sed -i 's/"bohrTime": 0,/"bohrTime": null,/' $TPL | |
| sed -i 's/"pascalTime": 0,/"pascalTime": null,/' $TPL | |
| sed -i 's/"pragueTime": 0,/"pragueTime": null,/' $TPL | |
| sed -i 's/"lorentzTime": 0,/"lorentzTime": null,/' $TPL | |
| sed -i 's/"maxwellTime": 0,/"maxwellTime": null,/' $TPL | |
| sed -i 's/"fermiTime": 0,/"fermiTime": null,/' $TPL | |
| sed -i 's/"haberFixTime": 0,/"haberFixTime": null,/' $TPL | |
| grep -q '"lubanBlock": 100000000' $TPL || { echo "patch_for_private_chain: luban patch failed" >&2; exit 1; } | |
| sed -i 's/"lubanBlock": 6,/"lubanBlock": 100000000,/' "$TPL" | |
| sed -i 's/"platoBlock": 7,/"platoBlock": 100000000,/' "$TPL" | |
| sed -i 's/"berlinBlock": 8,/"berlinBlock": 100000001,/' "$TPL" | |
| sed -i 's/"londonBlock": 8,/"londonBlock": 100000001,/' "$TPL" | |
| sed -i 's/"hertzBlock": 8,/"hertzBlock": 100000002,/' "$TPL" | |
| sed -i 's/"hertzfixBlock": 8,/"hertzfixBlock": 100000002,/' "$TPL" | |
| sed -i 's/"bohrTime": 0,/"bohrTime": null,/' "$TPL" | |
| sed -i 's/"pascalTime": 0,/"pascalTime": null,/' "$TPL" | |
| sed -i 's/"pragueTime": 0,/"pragueTime": null,/' "$TPL" | |
| sed -i 's/"lorentzTime": 0,/"lorentzTime": null,/' "$TPL" | |
| sed -i 's/"maxwellTime": 0,/"maxwellTime": null,/' "$TPL" | |
| sed -i 's/"fermiTime": 0,/"fermiTime": null,/' "$TPL" | |
| sed -i 's/"haberFixTime": 0,/"haberFixTime": null,/' "$TPL" | |
| grep -q '"lubanBlock": 100000000' "$TPL" || { echo "patch_for_private_chain: luban patch failed" >&2; exit 1; } | |
| grep -q '"platoBlock": 100000000' "$TPL" || { echo "patch_for_private_chain: plato patch failed" >&2; exit 1; } |
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 98-98: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 99-99: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 100-100: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 101-101: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 102-102: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 103-103: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 104-104: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 105-105: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 106-106: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 107-107: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 108-108: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 109-109: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 110-110: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 111-111: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docker_cluster.sh` around lines 98 - 111, The sed/grep commands use an
unquoted variable $TPL (risking globbing/word-splitting) and only verify the
"lubanBlock" replacement, allowing other sed failures to go unnoticed; update
all sed and grep invocations to double-quote "$TPL" and add at least one more
verification grep (e.g., check for '"platoBlock": 100000000') after the sed
sequence so failures in sed lines for "platoBlock" (and similar keys like
"berlinBlock", "londonBlock", etc.) are detected and cause a non-zero exit;
locate the sed calls and the existing grep check in the docker_cluster.sh
patch_for_private_chain section and apply these changes.
Comment on lines
+117
to
+121
| local VT=${workspace}/genesis/scripts/validators.template | ||
| if ! grep -q "// SORT_PATCHED" $VT; then | ||
| sed -i 's|function extraDataSerialize(validators) {|function extraDataSerialize(validators) {\n // SORT_PATCHED\n validators = validators.slice().sort((a,b) => a.consensusAddr.toLowerCase().localeCompare(b.consensusAddr.toLowerCase()));|' $VT | ||
| echo "patch_for_private_chain: validators.template now sorts by consensusAddr" | ||
| fi |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Quote $VT and consider more robust patching approach.
Same SC2086 issue: $VT unquoted on lines 118-119.
The sed-based JavaScript injection on line 119 is fragile—it depends on exact function signature formatting. If the template changes (e.g., different whitespace, renamed function), the patch silently fails and validators remain unsorted, causing "unauthorized validator" at block 1.
Consider using a dedicated patch file or jq/node-based approach for more robust patching.
♻️ Minimal fix: quote variables
- if ! grep -q "// SORT_PATCHED" $VT; then
- sed -i 's|function extraDataSerialize(validators) {|function extraDataSerialize(validators) {\n // SORT_PATCHED\n validators = validators.slice().sort((a,b) => a.consensusAddr.toLowerCase().localeCompare(b.consensusAddr.toLowerCase()));|' $VT
+ if ! grep -q "// SORT_PATCHED" "$VT"; then
+ sed -i 's|function extraDataSerialize(validators) {|function extraDataSerialize(validators) {\n // SORT_PATCHED\n validators = validators.slice().sort((a,b) => a.consensusAddr.toLowerCase().localeCompare(b.consensusAddr.toLowerCase()));|' "$VT"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| local VT=${workspace}/genesis/scripts/validators.template | |
| if ! grep -q "// SORT_PATCHED" $VT; then | |
| sed -i 's|function extraDataSerialize(validators) {|function extraDataSerialize(validators) {\n // SORT_PATCHED\n validators = validators.slice().sort((a,b) => a.consensusAddr.toLowerCase().localeCompare(b.consensusAddr.toLowerCase()));|' $VT | |
| echo "patch_for_private_chain: validators.template now sorts by consensusAddr" | |
| fi | |
| local VT=${workspace}/genesis/scripts/validators.template | |
| if ! grep -q "// SORT_PATCHED" "$VT"; then | |
| sed -i 's|function extraDataSerialize(validators) {|function extraDataSerialize(validators) {\n // SORT_PATCHED\n validators = validators.slice().sort((a,b) => a.consensusAddr.toLowerCase().localeCompare(b.consensusAddr.toLowerCase()));|' "$VT" | |
| echo "patch_for_private_chain: validators.template now sorts by consensusAddr" | |
| fi |
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 118-118: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 119-119: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docker_cluster.sh` around lines 117 - 121, The script currently uses an
unquoted $VT and a fragile sed insertion into validators.template targeting the
literal "function extraDataSerialize(validators) {"; fix by quoting the variable
references (use "$VT") and replace the brittle sed-based injection with a more
robust patch strategy: either apply a separate patch file (e.g.,
validators.template.patch) and use patch or git apply, or perform the
transformation with a small Node or jq script that locates the
extraDataSerialize function (by name) and inserts the "// SORT_PATCHED" + sort
line into its body; ensure you still check for the "// SORT_PATCHED" marker
before patching so the idempotent check using "$VT" and the marker remains.
… fix The BNB Chain team confirmed the >9-validator fast-finality panic at consensus/parlia/snapshot.go:411 and pushed a fix to the 'skip-execution' branch on bnb-chain/bsc: https://github.com/bnb-chain/bsc/tree/skip-execution Add BSC_GETH_BRANCH env var (default 'master') to control which bsc branch prepare_bsc_client() builds. Use 'BSC_GETH_BRANCH=skip-execution' to pull the fix. Once that branch lands in master, this can be reverted to default. Also handle the case where /workspace/bsc/Makefile already exists (from a previous build) by fetching+checking out the requested branch instead of just running git pull on whatever branch was already checked out. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
patch_for_private_chain()todocker_cluster.shthat:bohrTime/pascalTime/lorentzTime/maxwellTime/fermiTimeto nullconsensusAddrascending invalidators.templateAlso replaces flaky
forge install --no-gitwith a directgit cloneofforge-std.Why
The default
node-deploysetup is tested at 4 validators. For private chains with N > 4 validators across a WAN (e.g. 21 validators across multiple GCP regions), the chain consistently panics at block 8-9.The cascade
ReorgNeededWithFastFinality→parlia/snapshot.go:411) panics onvoteAddrs[idx]index out-of-range when the snapshot apply path encounters DoubleSigned headers.The validator sort fix
Parlia's
snapshot.validators()returns the validator set sorted-ascending. The originalvalidators.template'sextraDataSerializeemitted them in insertion order. Result: in-turn calculation picks a different validator than the one whose key is in keystore[0] → block 1 fails withunauthorized validator. This was only visible after pushing Luban back, because pre-Luban Parlia uses extraData order more directly.The forge install fix
forge install --no-git foundry-rs/forge-std@v1.7.3fails inside a parent directory that's a git repo (thegenesissubmodule's.gitis a redirect file pointing into the parent's.git/modules/). Forge's recursive ds-test init can't resolve the path. Directgit cloneworks reliably.Test plan
bash docker_cluster.sh preparewithDISABLE_FAST_FINALITY=true(default) and 21 validatorsgenesis.jsonhaslubanBlock: 100000000andplatoBlock: 100000000DISABLE_FAST_FINALITY=falseand 4 validators; confirm upstream behavior is preserved (lubanBlock: 6, fast finality active)Notes
DISABLE_FAST_FINALITYistruesince the canonical >4-validator path is via StakeHub registration on a running chain (mainnet pattern). Setting this tofalsepreserves the upstream 4-validator-with-fast-finality config.BSCValidatorSet.sol(turnLength = 16,if (block.number < 2000) return;) remain unchanged.🤖 Generated with Claude Code
Summary by CodeRabbit