Clean up min block scripts - #429
Conversation
Make slp_eval and measure_e2e both easier to use wrappers around one common script.
e01bda0 to
f051651
Compare
There was a problem hiding this comment.
Pull request overview
Refactors MinBlockTimeMixed benchmark wrappers around a shared implementation.
Changes:
- Adds shared parsing, validation, execution, and failure aggregation.
- Restores the streamlined
slp_evalinterface. - Expands
measure_e2ewith scaling and latency controls.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
scripts/base.sh |
Adds shared benchmark implementation. |
scripts/slp_eval.sh |
Uses shared fixed evaluation configuration. |
scripts/measure_e2e.sh |
Adds configurable end-to-end latency benchmarking. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if ! is_nonnegative_integer "$BLOCK_TIME_MS" || [ "$BLOCK_TIME_MS" -le "$BLOCK_TIME_BAND_MS" ]; then | ||
| fail "Block time must be an integer greater than ${BLOCK_TIME_BAND_MS}ms." | ||
| fi |
There was a problem hiding this comment.
there's a special flag in core to ignore protocol-based bounds (4000ms min, 5000ms max), so this comment is wrong. Are we configuring the flag in this script though? It's called TESTING_IGNORE_LEDGER_TIME_UPGRADE_BOUNDS
There was a problem hiding this comment.
This flag gets auto-configured by min block.
supercluster/src/FSLibrary/StellarCoreCfg.fs
Lines 247 to 248 in f051651
supercluster/src/FSLibrary/MinBlockTimeTest.fs
Lines 355 to 357 in f051651
| return 1 | ||
| fi | ||
|
|
||
| existing=$((key_count / TIER1_ORG_SIZE)) |
| # Usage: sh scripts/measure_e2e.sh [SLP_EVAL_OPTION...] [-- MISSION_ARG...] | ||
| # This script runs the MinBlockTimeMixed mission with stellar-core's loadgen | ||
| # end-to-end latency metrics enabled, against the 2026-06-03 pubnet topology | ||
| # scaled out to 1000 nodes. It answers "how long does a transaction take to go |
There was a problem hiding this comment.
any reason to deviate from the original proposal here #394 (comment)? It uses survey topology without the scaling.
There was a problem hiding this comment.
I'll update the comment: this uses the current trimmed form of the graph we've been using for the most recent survey, the "scaling" refers to the addition of tier1 nodes when the number of tier1s is > 10.
Make slp_eval and measure_e2e both easier to use wrappers around one common script.
slp_eval's interface is mostly restored to how it was pre 8351664.measure_e2e's interface is changed to make it easier to test against our scaling goals. Additionally, the following changes are made--ingress-external-hostis now a documented parameter for both scripts--enable-relaxed-auto-qset-configis added to the command line since Use automatic quorum set configuration by default #396 has still not mergedsac/oz/soroswap, the return value is 0 only iff all returned 0