feat(dash-spv-bench): multi-scenario sync benchmark framework#908
Open
ZocoLini wants to merge 1 commit into
Open
feat(dash-spv-bench): multi-scenario sync benchmark framework#908ZocoLini wants to merge 1 commit into
ZocoLini wants to merge 1 commit into
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a new ChangesSPV benchmark harness
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Scenario as Scenario YAML
participant Runner as run.sh
participant Peers as Docker dashd peers
participant Client as dash-spv-bench
participant Metrics as BenchEventHandler
participant Report as bench-scenarios.sh
Scenario->>Runner: load benchmark configuration
Runner->>Peers: create local peers and chain snapshot
Runner->>Client: pass benchmark environment
Client->>Metrics: emit synchronization events
Metrics-->>Client: signal completion or timeout
Client-->>Runner: write metrics and summary
Report->>Runner: repeat scenarios and aggregate results
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ZocoLini
marked this pull request as ready for review
July 20, 2026 22:20
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
dash-spv-bench/scenario.example.yml (1)
7-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider documenting the
repeatsfield.The
repeatsfield is used in several scenario files (e.g.,congested-2core.yml,ideal-1-peer.yml) to define the number of times a scenario should be run in a batch. Consider adding it to this example for completeness.♻️ Proposed fix
cpus: "0-3" # optional. taskset CPU list pinning the MEASURED client (here cores 0,1,2,3). # In local mode the docker peers get every OTHER core. Omit => no pinning. +repeats: 1 # optional. Number of times to run this scenario in batch mode. Omit => 1. + max_peers: 3 # optional. Max simultaneous peers. Omit => the ClientConfig default.🤖 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 `@dash-spv-bench/scenario.example.yml` around lines 7 - 9, Add the repeats field to scenario.example.yml alongside the other scenario configuration fields, with a concise comment explaining that it controls how many times the scenario runs in a batch and documenting its default behavior if applicable.dash-spv-bench/src/metrics.rs (1)
17-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding unit tests for the non-trivial metric logic.
record,snapshot, and theDisplayimpl forRunMetricscontain real branching/formatting logic (milestone dedup, fallback to elapsed time, TIMED OUT suffix) with no tests in this file. As per coding guidelines, write unit tests for new functionality.Also applies to: 81-109
🤖 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 `@dash-spv-bench/src/metrics.rs` around lines 17 - 52, The metric logic lacks unit coverage for branching and formatting behavior. Add focused unit tests for RunMetrics::record, RunMetrics::snapshot, and its Display implementation, covering milestone deduplication, elapsed-time fallback, completed versus timed-out output, and error formatting; keep tests scoped to this module and use existing APIs.Source: Coding guidelines
🤖 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 `@dash-spv-bench/run.sh`:
- Around line 310-318: Update the perf profiling branch in dash-spv-bench/run.sh
at lines 310-318 to create a unique temporary data-file path with mktemp, pass
that path to perf record and perf script, and remove it after processing. Apply
the same change at lines 319-329 for the sample file: generate its path with
mktemp, use it throughout the profiling pipeline, and delete it when complete.
In `@dash-spv-bench/src/main.rs`:
- Around line 111-112: Make the benchmark timeout configurable through an
environment variable, matching the existing run-parameter pattern in main and
preserving 1800 seconds as the default when unset or invalid. Update the timeout
initialization next to load_mnemonics and use the resulting Duration wherever
the benchmark enforces its timeout.
- Around line 169-172: Preserve the result of run_client.run().await in the
spawned task and propagate or record its error so the surrounding benchmark flow
can distinguish an early client failure from the 30-minute timeout. Update the
run_handle handling to inspect the task’s returned result and report the client
error while retaining the existing timeout behavior.
- Around line 74-78: Update the tracing subscriber file writer in the fmt layer
to use a shared Mutex<File> implementing MakeWriter instead of calling
log_file.try_clone() for each event. Remove the per-event expect-based
descriptor cloning while preserving the existing file_filter and non-ANSI
formatting configuration.
In `@dash-spv-bench/src/metrics.rs`:
- Around line 87-92: Update Metrics::wait_done so it creates and snapshots the
self.done.notified() future before checking inner.completed, then retain the
early return when completed is true and await the captured notification
otherwise. This ordering prevents the SyncComplete notification from being lost
between the condition check and await.
---
Nitpick comments:
In `@dash-spv-bench/scenario.example.yml`:
- Around line 7-9: Add the repeats field to scenario.example.yml alongside the
other scenario configuration fields, with a concise comment explaining that it
controls how many times the scenario runs in a batch and documenting its default
behavior if applicable.
In `@dash-spv-bench/src/metrics.rs`:
- Around line 17-52: The metric logic lacks unit coverage for branching and
formatting behavior. Add focused unit tests for RunMetrics::record,
RunMetrics::snapshot, and its Display implementation, covering milestone
deduplication, elapsed-time fallback, completed versus timed-out output, and
error formatting; keep tests scoped to this module and use existing APIs.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: db1b5739-2c02-49a2-a598-799e065c5ff2
📒 Files selected for processing (23)
.github/ci-groups.ymlCargo.tomldash-spv-bench/.gitignoredash-spv-bench/Cargo.tomldash-spv-bench/Dockerfiledash-spv-bench/bench-scenarios.shdash-spv-bench/run.shdash-spv-bench/scenario.example.ymldash-spv-bench/scenarios/congested-2core.ymldash-spv-bench/scenarios/ideal-1-peer.ymldash-spv-bench/scenarios/ideal-3-peers.ymldash-spv-bench/scenarios/ideal-5-peers.ymldash-spv-bench/scenarios/no-lag-ideal-1-peer.ymldash-spv-bench/scenarios/no-lag-ideal-3-peers.ymldash-spv-bench/scenarios/no-lag-ideal-5-peers.ymldash-spv-bench/scenarios/one-bad-peer.ymldash-spv-bench/scenarios/real-mainnet.ymldash-spv-bench/scenarios/real-testnet.ymldash-spv-bench/scenarios/slow-many-peers.ymldash-spv-bench/snapshot-chain.shdash-spv-bench/src/dashboard.rsdash-spv-bench/src/main.rsdash-spv-bench/src/metrics.rs
Comment on lines
+169
to
+172
| let run_client = client.clone(); | ||
| let run_handle = tokio::spawn(async move { | ||
| let _ = run_client.run().await; | ||
| }); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Silently discarded client run() error hides real failures.
let _ = run_client.run().await; drops any error the client returns. Combined with the fixed 30-minute timeout, an early client failure (e.g. connection/storage error) would be indistinguishable from a slow sync — both surface only as completed: false (TIMED OUT) in the report, making failures hard to diagnose.
🐛 Suggested fix
let run_handle = tokio::spawn(async move {
- let _ = run_client.run().await;
+ if let Err(e) = run_client.run().await {
+ tracing::error!("client run failed: {e}");
+ }
});📝 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
| let run_client = client.clone(); | |
| let run_handle = tokio::spawn(async move { | |
| let _ = run_client.run().await; | |
| }); | |
| let run_client = client.clone(); | |
| let run_handle = tokio::spawn(async move { | |
| if let Err(e) = run_client.run().await { | |
| tracing::error!("client run failed: {e}"); | |
| } | |
| }); |
🤖 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 `@dash-spv-bench/src/main.rs` around lines 169 - 172, Preserve the result of
run_client.run().await in the spawned task and propagate or record its error so
the surrounding benchmark flow can distinguish an early client failure from the
30-minute timeout. Update the run_handle handling to inspect the task’s returned
result and report the client error while retaining the existing timeout
behavior.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #908 +/- ##
==========================================
+ Coverage 74.54% 74.57% +0.02%
==========================================
Files 327 327
Lines 75032 75032
==========================================
+ Hits 55936 55954 +18
+ Misses 19096 19078 -18
|
A docker-driven benchmark that syncs the SPV client against a frozen chain snapshot (local peers via tc netem) or the real testnet/mainnet, reporting sync time and a live dashboard. Base branch for the network-rewrite PR stacked on top; it drives the rewritten client and does not build on dev alone. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CUb3bkX9C1gBFA65GFsN53
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
dash-spv-bench/Dockerfile (1)
18-21: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winVerify integrity of externally-fetched binaries before use. Both sites
curla release binary from GitHub and use it immediately with no checksum or signature check, even though Dash Core publishes a signedSHA256SUMS.ascfor its releases.
dash-spv-bench/Dockerfile#L18-L21: verify the downloadeddashcore-${DASHVERSION}-${dlarch}-linux-gnu.tar.gzagainst Dash Core's publishedSHA256SUMS/SHA256SUMS.ascbefore extracting and copyingdashdinto the image.dash-spv-bench/run.sh#L63-L66: verify the downloadedyq_${os}_${arch}binary's checksum (mikefarah/yq publishes checksums/attestations per release) beforechmod +xand using it to parse scenario files.🤖 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 `@dash-spv-bench/Dockerfile` around lines 18 - 21, Verify externally downloaded binaries before use: in dash-spv-bench/Dockerfile lines 18-21, download Dash Core’s published SHA256SUMS/SHA256SUMS.asc, validate the selected dashcore archive, and only then extract and copy dashd; in dash-spv-bench/run.sh lines 63-66, validate the downloaded yq_${os}_${arch} binary against mikefarah/yq’s release checksum or attestation before chmod +x or using it.
🤖 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 `@dash-spv-bench/run.sh`:
- Around line 265-279: Update bring_up’s peer-start loop to health-check any
trailing partial batch after iteration completes: when started is non-empty and
count is not divisible by batch, call wait_loaded for the remaining services and
fail with the same timeout behavior if it does not become ready. Preserve the
existing full-batch checks and startup reporting.
- Around line 148-172: Separate command substitution from export for
BENCH_MAX_PEERS and BENCH_PEERS: assign each value first, then export the
variable. Preserve their existing scn expressions and ensure a failing scn
invocation remains visible to set -euo pipefail instead of being masked by
export.
In `@dash-spv-bench/snapshot-chain.sh`:
- Line 7: Update the snapshot-chain.sh image configuration to use an immutable
Dash Core version or digest instead of the latest tag. Adjust the surrounding
image reuse logic so it rebuilds or pulls when the Dockerfile changes, rather
than skipping whenever any local image exists, ensuring benchmark runs use the
intended image consistently.
- Line 11: Validate BENCH_HEIGHT as a non-negative integer immediately after its
required-variable check and before any Docker or directory setup in
snapshot-chain.sh; reject malformed or negative values with a clear error and
exit, while allowing valid integer heights to reach -stopatheight.
- Around line 21-26: After the docker run in snapshot-chain.sh, query the
generated chain’s actual tip height and validate it against the scenario’s
target-height contract represented by BENCH_HEIGHT. Fail the script when the tip
is missing or does not match the required height, and print the success message
only after validation passes.
---
Nitpick comments:
In `@dash-spv-bench/Dockerfile`:
- Around line 18-21: Verify externally downloaded binaries before use: in
dash-spv-bench/Dockerfile lines 18-21, download Dash Core’s published
SHA256SUMS/SHA256SUMS.asc, validate the selected dashcore archive, and only then
extract and copy dashd; in dash-spv-bench/run.sh lines 63-66, validate the
downloaded yq_${os}_${arch} binary against mikefarah/yq’s release checksum or
attestation before chmod +x or using it.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 376b86db-aa4b-4506-b67a-a55eed273a7a
📒 Files selected for processing (23)
.github/ci-groups.ymlCargo.tomldash-spv-bench/.gitignoredash-spv-bench/Cargo.tomldash-spv-bench/Dockerfiledash-spv-bench/bench-scenarios.shdash-spv-bench/run.shdash-spv-bench/scenario.example.ymldash-spv-bench/scenarios/congested-2core.ymldash-spv-bench/scenarios/ideal-1-peer.ymldash-spv-bench/scenarios/ideal-3-peers.ymldash-spv-bench/scenarios/ideal-5-peers.ymldash-spv-bench/scenarios/no-lag-ideal-1-peer.ymldash-spv-bench/scenarios/no-lag-ideal-3-peers.ymldash-spv-bench/scenarios/no-lag-ideal-5-peers.ymldash-spv-bench/scenarios/one-bad-peer.ymldash-spv-bench/scenarios/real-mainnet.ymldash-spv-bench/scenarios/real-testnet.ymldash-spv-bench/scenarios/slow-many-peers.ymldash-spv-bench/snapshot-chain.shdash-spv-bench/src/dashboard.rsdash-spv-bench/src/main.rsdash-spv-bench/src/metrics.rs
🚧 Files skipped from review as they are similar to previous changes (19)
- dash-spv-bench/scenarios/no-lag-ideal-1-peer.yml
- dash-spv-bench/scenarios/slow-many-peers.yml
- dash-spv-bench/Cargo.toml
- .github/ci-groups.yml
- dash-spv-bench/scenarios/no-lag-ideal-3-peers.yml
- dash-spv-bench/scenarios/real-mainnet.yml
- dash-spv-bench/.gitignore
- Cargo.toml
- dash-spv-bench/scenarios/congested-2core.yml
- dash-spv-bench/scenarios/no-lag-ideal-5-peers.yml
- dash-spv-bench/scenarios/ideal-1-peer.yml
- dash-spv-bench/scenarios/ideal-5-peers.yml
- dash-spv-bench/scenario.example.yml
- dash-spv-bench/scenarios/ideal-3-peers.yml
- dash-spv-bench/scenarios/one-bad-peer.yml
- dash-spv-bench/scenarios/real-testnet.yml
- dash-spv-bench/src/metrics.rs
- dash-spv-bench/src/dashboard.rs
- dash-spv-bench/src/main.rs
Comment on lines
+148
to
+172
| MODE="$(scn '.mode // "local"')" | ||
| case "${MODE}" in local | testnet | mainnet) ;; *) echo "Error: mode must be 'local', 'testnet' or 'mainnet' (got '${MODE}')" >&2; exit 1 ;; esac | ||
| export BENCH_MODE="${MODE}" | ||
| export CLONE_DIR="${CLONE_DIR:-/nonexistent}" | ||
|
|
||
| BENCH_CPUS="$(scn '.cpus // ""')" | ||
| export BENCH_MAX_PEERS="$(scn '.max_peers // ""')" # only if set; else the ClientConfig default | ||
|
|
||
| BENCH_WALLET_FILE="${SCRIPT_DIR}/wallets.txt" | ||
| [ -n "${WALLETS_ARG}" ] && BENCH_WALLET_FILE="$(abspath "${WALLETS_ARG}")" | ||
| export BENCH_WALLET_FILE | ||
| export BENCH_STORAGE_DIR="${SCRIPT_DIR}/bench-storage" | ||
|
|
||
| DESC="$(scn '.description // ""')" | ||
| [ -n "${DESC}" ] && echo "==> description: ${DESC}" | ||
|
|
||
| BLOCKS="" | ||
| if [ "${MODE}" = local ]; then | ||
| BLOCKS="$(scn '.blocks // 1000000')" | ||
| export BENCH_HEIGHT="${BLOCKS}" | ||
| unset BENCH_START_HEIGHT # local always syncs from genesis | ||
| else | ||
| export BENCH_PEERS="$(scn '.peers // [] | join(",")')" | ||
| sh="$(scn '.start_height // ""')"; [ -n "${sh}" ] && export BENCH_START_HEIGHT="${sh}" | ||
| fi |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
export VAR="$(cmd)" masks command-substitution failures under set -e.
Lines 154 and 170 combine export with a command substitution (export BENCH_MAX_PEERS="$(scn ...)", export BENCH_PEERS="$(scn ...)"). Per shellcheck SC2155, the exit status reported for such a statement is that of export/local, not the substituted command, so a failing yq invocation (e.g., malformed scenario YAML) would silently produce an empty value instead of aborting via set -euo pipefail.
🔧 Proposed fix: declare and assign separately
-export BENCH_MAX_PEERS="$(scn '.max_peers // ""')" # only if set; else the ClientConfig default
+BENCH_MAX_PEERS="$(scn '.max_peers // ""')" # only if set; else the ClientConfig default
+export BENCH_MAX_PEERS- export BENCH_PEERS="$(scn '.peers // [] | join(",")')"
+ BENCH_PEERS="$(scn '.peers // [] | join(",")')"
+ export BENCH_PEERS📝 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
| MODE="$(scn '.mode // "local"')" | |
| case "${MODE}" in local | testnet | mainnet) ;; *) echo "Error: mode must be 'local', 'testnet' or 'mainnet' (got '${MODE}')" >&2; exit 1 ;; esac | |
| export BENCH_MODE="${MODE}" | |
| export CLONE_DIR="${CLONE_DIR:-/nonexistent}" | |
| BENCH_CPUS="$(scn '.cpus // ""')" | |
| export BENCH_MAX_PEERS="$(scn '.max_peers // ""')" # only if set; else the ClientConfig default | |
| BENCH_WALLET_FILE="${SCRIPT_DIR}/wallets.txt" | |
| [ -n "${WALLETS_ARG}" ] && BENCH_WALLET_FILE="$(abspath "${WALLETS_ARG}")" | |
| export BENCH_WALLET_FILE | |
| export BENCH_STORAGE_DIR="${SCRIPT_DIR}/bench-storage" | |
| DESC="$(scn '.description // ""')" | |
| [ -n "${DESC}" ] && echo "==> description: ${DESC}" | |
| BLOCKS="" | |
| if [ "${MODE}" = local ]; then | |
| BLOCKS="$(scn '.blocks // 1000000')" | |
| export BENCH_HEIGHT="${BLOCKS}" | |
| unset BENCH_START_HEIGHT # local always syncs from genesis | |
| else | |
| export BENCH_PEERS="$(scn '.peers // [] | join(",")')" | |
| sh="$(scn '.start_height // ""')"; [ -n "${sh}" ] && export BENCH_START_HEIGHT="${sh}" | |
| fi | |
| MODE="$(scn '.mode // "local"')" | |
| case "${MODE}" in local | testnet | mainnet) ;; *) echo "Error: mode must be 'local', 'testnet' or 'mainnet' (got '${MODE}')" >&2; exit 1 ;; esac | |
| export BENCH_MODE="${MODE}" | |
| export CLONE_DIR="${CLONE_DIR:-/nonexistent}" | |
| BENCH_CPUS="$(scn '.cpus // ""')" | |
| BENCH_MAX_PEERS="$(scn '.max_peers // ""')" # only if set; else the ClientConfig default | |
| export BENCH_MAX_PEERS | |
| BENCH_WALLET_FILE="${SCRIPT_DIR}/wallets.txt" | |
| [ -n "${WALLETS_ARG}" ] && BENCH_WALLET_FILE="$(abspath "${WALLETS_ARG}")" | |
| export BENCH_WALLET_FILE | |
| export BENCH_STORAGE_DIR="${SCRIPT_DIR}/bench-storage" | |
| DESC="$(scn '.description // ""')" | |
| [ -n "${DESC}" ] && echo "==> description: ${DESC}" | |
| BLOCKS="" | |
| if [ "${MODE}" = local ]; then | |
| BLOCKS="$(scn '.blocks // 1000000')" | |
| export BENCH_HEIGHT="${BLOCKS}" | |
| unset BENCH_START_HEIGHT # local always syncs from genesis | |
| else | |
| BENCH_PEERS="$(scn '.peers // [] | join(",")')" | |
| export BENCH_PEERS | |
| sh="$(scn '.start_height // ""')"; [ -n "${sh}" ] && export BENCH_START_HEIGHT="${sh}" | |
| fi |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 154-154: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 170-170: Declare and assign separately to avoid masking return values.
(SC2155)
🤖 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 `@dash-spv-bench/run.sh` around lines 148 - 172, Separate command substitution
from export for BENCH_MAX_PEERS and BENCH_PEERS: assign each value first, then
export the variable. Preserve their existing scn expressions and ensure a
failing scn invocation remains visible to set -euo pipefail instead of being
masked by export.
Source: Linters/SAST tools
Comment on lines
+265
to
+279
| local batch=4 | ||
| echo "==> starting ${n} peers in batches of ${batch}" | ||
| local started="" count=0 | ||
| for svc in ${services}; do | ||
| CLONE_DIR="${clone_dir}" compose up -d "${svc}" >/dev/null 2>&1 | ||
| started="${started} spv-bench-${svc}"; count=$((count + 1)) | ||
| if [ $((count % batch)) -eq 0 ]; then | ||
| echo -n " loaded ${count}/${n} " | ||
| wait_loaded ${started} || { echo " timeout loading batch" >&2; exit 1; } | ||
| echo " ok" | ||
| fi | ||
| done | ||
|
|
||
| echo "==> ${n} peers started" | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Trailing partial batch of peers is never health-checked before bring_up() returns.
wait_loaded is only invoked when count % batch == 0 (Line 271). If n (total peers) isn't a multiple of batch, the last group of started containers is never waited on — bring_up() returns and the sync benchmark begins while those peers may still be loading. Since the entire point of this harness is accurate timing, starting the measured sync before all peers are actually ready to serve data would skew results.
🐛 Proposed fix: wait for stragglers after the loop
local batch=4
echo "==> starting ${n} peers in batches of ${batch}"
local started="" count=0
for svc in ${services}; do
CLONE_DIR="${clone_dir}" compose up -d "${svc}" >/dev/null 2>&1
started="${started} spv-bench-${svc}"; count=$((count + 1))
if [ $((count % batch)) -eq 0 ]; then
echo -n " loaded ${count}/${n} "
wait_loaded ${started} || { echo " timeout loading batch" >&2; exit 1; }
echo " ok"
fi
done
+ if [ $((count % batch)) -ne 0 ]; then
+ echo -n " loaded ${n}/${n} "
+ wait_loaded ${started} || { echo " timeout loading batch" >&2; exit 1; }
+ echo " ok"
+ fi
echo "==> ${n} peers started"📝 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 batch=4 | |
| echo "==> starting ${n} peers in batches of ${batch}" | |
| local started="" count=0 | |
| for svc in ${services}; do | |
| CLONE_DIR="${clone_dir}" compose up -d "${svc}" >/dev/null 2>&1 | |
| started="${started} spv-bench-${svc}"; count=$((count + 1)) | |
| if [ $((count % batch)) -eq 0 ]; then | |
| echo -n " loaded ${count}/${n} " | |
| wait_loaded ${started} || { echo " timeout loading batch" >&2; exit 1; } | |
| echo " ok" | |
| fi | |
| done | |
| echo "==> ${n} peers started" | |
| } | |
| local batch=4 | |
| echo "==> starting ${n} peers in batches of ${batch}" | |
| local started="" count=0 | |
| for svc in ${services}; do | |
| CLONE_DIR="${clone_dir}" compose up -d "${svc}" >/dev/null 2>&1 | |
| started="${started} spv-bench-${svc}"; count=$((count + 1)) | |
| if [ $((count % batch)) -eq 0 ]; then | |
| echo -n " loaded ${count}/${n} " | |
| wait_loaded ${started} || { echo " timeout loading batch" >&2; exit 1; } | |
| echo " ok" | |
| fi | |
| done | |
| if [ $((count % batch)) -ne 0 ]; then | |
| echo -n " loaded ${n}/${n} " | |
| wait_loaded ${started} || { echo " timeout loading batch" >&2; exit 1; } | |
| echo " ok" | |
| fi | |
| echo "==> ${n} peers started" | |
| } |
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 273-273: 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 `@dash-spv-bench/run.sh` around lines 265 - 279, Update bring_up’s peer-start
loop to health-check any trailing partial batch after iteration completes: when
started is non-empty and count is not divisible by batch, call wait_loaded for
the remaining services and fail with the same timeout behavior if it does not
become ready. Preserve the existing full-batch checks and startup reporting.
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
| CHAIN_DIR="${SCRIPT_DIR}/chain-data" | ||
| SUBDIR="testnet3" | ||
| IMAGE="dash-spv-bench/dashd:latest" |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Pin the Dash Core image for reproducible benchmarks.
Using latest and skipping the build whenever any local image exists can silently reuse a stale or different Dash Core image. Pin an immutable version/digest and rebuild or pull when the Dockerfile changes; otherwise benchmark results are not comparable across runs or machines.
Also applies to: 14-15
🤖 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 `@dash-spv-bench/snapshot-chain.sh` at line 7, Update the snapshot-chain.sh
image configuration to use an immutable Dash Core version or digest instead of
the latest tag. Adjust the surrounding image reuse logic so it rebuilds or pulls
when the Dockerfile changes, rather than skipping whenever any local image
exists, ensuring benchmark runs use the intended image consistently.
|
|
||
| cd "${SCRIPT_DIR}" | ||
|
|
||
| : "${BENCH_HEIGHT:?BENCH_HEIGHT must be set — run.sh exports it from the scenario blocks}" |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate BENCH_HEIGHT before invoking Docker.
The presence check accepts malformed values, which are passed to -stopatheight and fail only after image and directory setup. Reject non-integer or negative values before performing side effects.
Also applies to: 21-24
🤖 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 `@dash-spv-bench/snapshot-chain.sh` at line 11, Validate BENCH_HEIGHT as a
non-negative integer immediately after its required-variable check and before
any Docker or directory setup in snapshot-chain.sh; reject malformed or negative
values with a clear error and exit, while allowing valid integer heights to
reach -stopatheight.
Comment on lines
+21
to
+26
| docker run --rm --user "$(id -u):$(id -g)" -v "${CHAIN_DIR}:/data" "${IMAGE}" \ | ||
| dashd -testnet -datadir=/data -daemon=0 -server=1 \ | ||
| -blockfilterindex=1 -peerblockfilters=1 -stopatheight="${BENCH_HEIGHT}" \ | ||
| -txindex=0 -prune=0 -disablewallet=1 | ||
|
|
||
| echo "==> done. Snapshot is at ${CHAIN_DIR}/${SUBDIR} (height ${BENCH_HEIGHT})." |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Verify the generated snapshot height before reporting success.
The script prints that the snapshot is at BENCH_HEIGHT without checking the actual chain tip. Reused or partially existing chain-data can make this message inaccurate; query the resulting tip and fail if it does not satisfy the scenario’s target-height contract.
🤖 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 `@dash-spv-bench/snapshot-chain.sh` around lines 21 - 26, After the docker run
in snapshot-chain.sh, query the generated chain’s actual tip height and validate
it against the scenario’s target-height contract represented by BENCH_HEIGHT.
Fail the script when the tip is missing or does not match the required height,
and print the success message only after validation passes.
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.
A docker-driven benchmark that syncs the SPV client against a frozen chain snapshot (local peers via tc netem) or the real testnet/mainnet, reporting sync time and a live dashboard.
Summary by CodeRabbit