Skip to content

ci: add a Commit-Boost check to the kurtosis battery - #708

Open
MegaRedHand wants to merge 8 commits into
mainfrom
ci/kurtosis-commit-boost
Open

ci: add a Commit-Boost check to the kurtosis battery#708
MegaRedHand wants to merge 8 commits into
mainfrom
ci/kurtosis-commit-boost

Conversation

@MegaRedHand

@MegaRedHand MegaRedHand commented Oct 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Adds a Commit-Boost check to the kurtosis battery: ethlambda builds blocks through Commit-Boost's PBS module, the MEV-Boost replacement that speaks the same builder-API.

What it runs

tooling/kurtosis-assertoor/commit_boost_params.yaml:

  • Participants: the base battery's three (lighthouse bootnode; lighthouse with the ethlambda validator client; ethlambda beacon node with the ethlambda validator client).
  • MEV: mev_type: commit-boost. ethereum-package then adds a flashbots relay, an rbuilder with its own lighthouse, and one Commit-Boost PBS sidecar per participant.
  • Transactions: spamoor at a light load (10 transfers and 1 blob transaction a slot). rbuilder submits no bid for an empty block, and spamoor's default load slowed the relay past Commit-Boost's 950 ms getHeader timeout.
  • Images: the relay, builder, PBS and spamoor images are pinned by digest, since the package's defaults are floating tags.
  • No assertoor: its checks still fail on the unrelated GLOAS_FORK_EPOCH spec mismatch (README, "Known failure").

scripts/check-builder-blocks.sh polls Prometheus metrics until all of these hold, or 40 minutes pass:

Service Condition Shows
ethlambda beacon node lean_beacon_builder_registrations_total{outcome="forwarded"} >= 1 registrations reach Commit-Boost
ethlambda beacon node lean_beacon_builder_payload_source_total{source="builder"} >= 1 produceBlockV3 served a builder block
ethlambda VC on ethlambda ethlambda_validator_builder_registrations_sent_total >= 1, ethlambda_validator_blinded_blocks_proposed_total >= 1 the validator client registers and publishes blinded blocks
ethlambda VC on lighthouse ethlambda_validator_blinded_blocks_proposed_total >= 1 the validator client works against a third-party beacon node

The script writes each service's builder metrics and the unmet conditions to the results directory on every exit.

Dispatch only, for now

Depends on

Upstream workarounds

Breakage Worked around in
rbuilder refuses ENR bootnodes fork PR #23, through the pin
Commit-Boost requires SECONDS_PER_SLOT, which the genesis generator no longer writes an explicit chain in mev_params.commit_boost_config (chain_id must be a hex string)

Tested

  • shellcheck, actionlint and yq pass.
  • Local run, passed: an image built from feat(validator): publish proposer preferences (gloas) #707's head (ae1a3d2f) met all five conditions about 12 minutes after the enclave started.
    • The relay delivered a builder block in 14 of the 32 slots of epoch 1.
    • ethlambda's beacon node served 1 builder payload out of 16 proposals.
    • Its validator client proposed 1 blinded block; the one behind lighthouse proposed 3.
  • Earlier local runs, failed: they are why spamoor runs, and at a light load.
    • With no transactions, the relay never held a bid, so every getHeader answered 204.
    • With spamoor's defaults, the validator client behind lighthouse got no bid in 40 minutes.
  • CI: the job itself has not been dispatched.

@MegaRedHand
ci: add a Commit-Boost check to the kurtosis battery
e345428 
The builder PR stack (#694 to #705) lets ethlambda's beacon node and
validator client build blocks through a MEV-Boost compatible builder, but
nothing exercises it against a real relay and builder. Commit-Boost's PBS
module is a drop-in MEV-Boost replacement, so a devnet with
`mev_type: commit-boost` covers the whole path.

The check reads the ethlambda services' builder metrics instead of running
assertoor, because the base battery's assertoor tests currently fail on an
unrelated spec-endpoint gap. It passes once registrations are forwarded, the
ethlambda beacon node serves a builder payload, and the ethlambda validator
client proposes blinded blocks both through that node and through lighthouse.

The new `commit-boost` job runs on manual dispatch only: main's image has no
`--builder` or `--builder-proposals` until the stack lands. It has its own
ethereum-package pin, which needs the fork's builder-flags branch pushed. The
relay, builder and PBS images are pinned by digest since the package defaults
are floating tags.
@MegaRedHand
docs: explain local images and the fork PR for the Commit-Boost check
ad9ebea 
A contributor running the check locally needs to know how to get an image
with the builder flags and that kurtosis will use it without pulling, or
it looks like the run needs a published image. The fork pin also now has a
PR to point at instead of an unpushed branch.
@github-actions

github-actions Bot commented Oct 26, 2026

Copy link
Copy Markdown

🤖 Codex Code Review

Looks good overall. I did not find consensus-layer logic changes here; this PR is CI/tooling-only, so fork choice, attestation processing, state transition, XMSS, and SSZ risks are not directly touched.

  • tooling/kurtosis-assertoor/scripts/check-builder-blocks.sh:52
    value_of() prints with "%d", but Prometheus sample values are numeric strings and may be rendered as floats or scientific notation. In awk, printf "%d" truncates, so values like 0.9 become 0 and 1e+00 handling is awk-dependent. Safer to compare numerically in awk and print with %.0f only if you really want integers, or keep the raw numeric string and compare as a number. This is a correctness risk for the CI gate.

  • tooling/kurtosis-assertoor/scripts/check-builder-blocks.sh:38
    scrape() strips the scheme and then hardcodes "/metrics": curl ... "${addr#*://}/metrics". If kurtosis port print already returns the full metrics URL rather than just scheme://host:port, this builds an invalid URL. The current code assumes a very specific output shape from Kurtosis. I’d either document that contract explicitly or normalize more defensively.

  • tooling/kurtosis-assertoor/scripts/check-builder-blocks.sh:31
    The check hardcodes service names and participant ordering. The README documents that coupling, but it is still brittle: any change in ethereum-package naming or participant order will make the check fail even if builder flow works. At minimum, consider asserting early that the expected services exist and fail with a clearer message.

  • .github/workflows/kurtosis_assertoor.yml:264
    notify only depends on [image, assertoor]. If battery: both and the new commit-boost job fails while assertoor passes, the workflow fails but Slack notification does not mention the failing job. If that is intentional, fine; otherwise notify should either include commit-boost in needs or be renamed/scoped to assertoor-only failure reporting.

  • .github/workflows/kurtosis_assertoor.yml:97
    The assertoor gating expression is correct for the three dispatch modes, but it’s subtle. A short comment here would help maintainability, because future edits could easily invert the logic and unintentionally skip the base battery for battery: both.

  • tooling/kurtosis-assertoor/commit_boost_params.yaml:47
    The comment says the digests are “what the tags pointed to on 2026-10-26”. Pinning by digest is good for supply-chain stability, but this comment will age quickly and can mislead reviewers if the PR is merged later. I’d rephrase to “pinned as of this update” or include how to refresh them, not a date-sensitive claim.

If you want, I can turn the first three points into concrete patch suggestions.

Automated review by OpenAI Codex · gpt-5.4 · custom prompt

@github-actions

github-actions Bot commented Oct 26, 2026

Copy link
Copy Markdown

🤖 Claude Code Review

Review of PR 708: Commit-Boost kurtosis check

This is a CI and tooling change with no consensus code. It looks sound overall. I read the diff but did not run the workflow or the script, so the points below are from reading only.

Good

  • Both the package commit and the relay, builder and PBS images are pinned (the images by digest). The job verifies that HEAD equals the pinned SHA after fetching.
  • The new job is dispatch-only, with a clear note on when to drop that restriction.
  • The base assertoor job's if is safe on non-dispatch events, because the event_name check short-circuits before inputs.battery is read.
  • The script handles set -euo pipefail carefully. The || true guards are in the right places, and the metrics dump runs on every exit through the trap.

Points to consider

  1. Failure notification. notify has needs: [image, assertoor] (the notify job, unchanged in the diff). A commit-boost failure, or a run with battery: commit-boost, will not post to Slack. This is fine while the job is manual. Add it to needs when the job is enabled on push, and check that the notify if copes with a skipped assertoor.
  2. Metric port name. check-builder-blocks.sh assumes the kurtosis port is named metrics for all three services (kurtosis port print "$ENCLAVE" "$1" metrics). If the name is wrong, scrape silently returns nothing. The script then reports "unmet = 0" for 40 minutes rather than naming the cause. Print a distinct message when the port lookup fails, at least on the last iteration.
  3. Hardcoded service names. cl-3-ethlambda-geth, vc-2-geth-lighthouse-ethlambda and vc-3-geth-ethlambda depend on the package's naming and on the participant order. The YAML comment and the README document this. A cheap guard would be to check that the services exist (kurtosis service inspect) up front, so a rename gives an immediate, clear error.
  4. value_of precision. printf "%d" truncates float values. Counters are integers, so this is fine. A gauge-style or 1e+00 exposition would still work in awk, so no change is needed.
  5. Fork pin fragility. The README already notes that the pinned commit can be garbage collected after a squash-merge of feat: wire ethlambda to the external builder ethereum-package#22. A CI failure at git fetch would be the first sign. Consider tagging the commit in the fork to keep it reachable.
  6. Minor. yq is assumed to be on ubuntu-latest, which is true today. The job's timeout-minutes: 90 comfortably covers the 40-minute poll plus startup.

No blocking issues.

Automated review by Claude (Anthropic) · sonnet · custom prompt

@github-actions

github-actions Bot commented Oct 26, 2026

Copy link
Copy Markdown

🤖 Kimi Code Review

I'll review this PR, which adds a Commit-Boost builder test battery to the Kurtosis/Assertoor CI workflow. The changes are primarily in CI configuration, documentation, and a shell test script — no consensus-layer Rust code is modified.

General Assessment

This is a well-structured CI addition with good documentation. The shell script has several issues that could cause flaky or incorrect behavior in CI.

Detailed Findings

1. Shell Script: value_of awk selector parsing is fragile

File: tooling/kurtosis-assertoor/scripts/check-builder-blocks.sh
Lines: 63–69

value_of() {
    awk -v sel="$2" '
        index(sel, "{") { if ($1 == sel) sum += $NF; next }
        { split($1, p, "{"); if (p[1] == sel) sum += $NF }
        END { printf "%d\n", sum }
    ' <<<"$1"
}

Issues:

  • Floating-point output risk: printf "%d\n" with awk's default floating-point arithmetic can produce scientific notation for large values (e.g., 1e+06), causing the subsequent [ "$value" -ge 1 ] test to fail with "integer expression expected" in bash. Prometheus counters can grow large over a long-running test.

  • Label matching with index(sel, "{") is imprecise: A metric name containing { as part of a label value (unlikely but valid in Prometheus text format) would trigger the wrong branch. The selector format is name{labels} — better to use a more robust parse.

Suggested fix:

value_of() {
    awk -v sel="$2" '
        BEGIN {
            # Determine if selector has labels: must start with name{ and end with }
            has_labels = (match(sel, /^{/) == 0 && match(sel, /{.*}$/) > 0)
        }
        has_labels { if ($1 == sel) sum += $NF; next }
        { split($1, p, "{"); if (p[1] == sel) sum += $NF }
        END { printf "%.0f\n", sum + 0 }  # force numeric, round to integer
    ' <<<"$1"
}

Even better, avoid awk floating-point entirely for integer counters:

value_of() {
    local text="$1" sel="$2"
    if [[ "$sel" == *"{"* ]]; then
        grep -F "$sel" <<<"$text" | awk '{sum+=$NF} END {printf "%d\n", sum+0}'
    else
        awk -v name="$sel" '{split($1,p,"{"); if(p[1]==name) sum+=$NF} END {printf "%d\n", sum+0}' <<<"$text"
    fi
}

2. Shell Script: scrape silently ignores failures

File: tooling/kurtosis-assertoor/scripts/check-builder-blocks.sh
Lines: 52–56

scrape() {
    local addr
    addr=$(kurtosis port print "$ENCLAVE" "$1" metrics 2>/dev/null) || return 0
    curl -sf --max-time 10 "${addr#*://}/metrics" 2>/dev/null || true
}

Issue: If kurtosis port print fails (service not yet started), the function returns 0 with empty output. This is intentional for startup polling, but if kurtosis itself fails (e.g., enclave doesn't exist), the script silently continues. Consider distinguishing "service not ready" from "enclave missing" for better debugging.

Minor suggestion: Add a warning when addr is empty but the enclave exists, or at least log kurtosis errors to stderr after initial startup period.

3. Shell Script: Race condition in evaluate with repeated scraping

File: tooling/kurtosis-assertoor/scripts/check-builder-blocks.sh
Lines: 88–99

evaluate() {
    ...
    for entry in "${CONDITIONS[@]}"; do
        IFS='|' read -r svc series desc <<<"$entry"
        value=$(value_of "$(scrape "$svc")" "$series")
        ...
    done
}

Issue: scrape "$svc" is called twice per service per loop iteration — once in evaluate and once in dump_metrics via the EXIT trap. More critically, within evaluate, each condition calls scrape separately, meaning metrics for CL3 are fetched 2 times, VC2 once, VC3 twice — all at different timestamps. A transient network issue could make one call succeed and another fail.

Suggested fix: Cache scrapes per service per evaluation:

evaluate() {
    local entry svc series desc value unmet=0
    local -A metrics
    # Pre-fetch all service metrics once
    for svc in "${SERVICES[@]}"; do
        metrics[$svc]=$(scrape "$svc")
    done
    
    for entry in "${CONDITIONS[@]}"; do
        IFS='|' read -r svc series desc <<<"$entry"
        value=$(value_of "${metrics[$svc]}" "$series")
        ...
    done
}

4. Shell Script: grep '^unmet' after evaluate may operate on stale data

File: tooling/kurtosis-assertoor/scripts/check-builder-blocks.sh
Lines: 113–114, 122

if summary=$(evaluate); then
    ...
fi
echo "$summary"
grep '^unmet' <<<"$summary" > "$OUT/failed.txt" || true

Issue: If evaluate fails (returns 1), summary still contains the output due to set -e not triggering on command substitution failure in the if condition. This is actually correct bash behavior, but subtle. However, if evaluate outputs to stderr or has a pipefail issue, summary could be partial.

More concerning: The finish trap calls dump_metrics which overwrites *.prom files. If the script is interrupted (SIGTERM from CI timeout), the EXIT trap runs, but the failed.txt write at line 122 may not happen if the signal arrives between the loop and that line. The failed.txt should probably be written in the finish trap as well for consistency.

5. Shell Script: Missing kurtosis availability check

File: tooling/kurtosis-assertoor/scripts/check-builder-blocks.sh
Line: 53

The script assumes kurtosis is in PATH. For local runs per the README, this is documented, but a clear error message would help:

command -v kurtosis >/dev/null 2>&1 || { echo "kurtosis not found in PATH" >&2; exit 2; }

6. Workflow: notify job dependency incomplete

File: .github/workflows/kurtosis_assertoor.yml
Line: 264 (original), context around line 170

notify:
    needs: [image, assertoor]

Issue: The notify job only depends on image and assertoor. If commit-boost is run via battery: both or battery: commit-boost, failures in commit-boost will not trigger the Slack notification. The commit-boost job is not in the needs array.

Suggested fix:

notify:
    needs: [image, assertoor, commit-boost]
    if: always() && (needs.assertoor.result == 'failure' || needs.commit-boost.result == 'failure' || needs.image.result == 'failure')

Or adjust the existing if condition to include commit-boost. Without this, the commit-boost job can fail silently in terms of notifications.

7. Workflow: commit-boost job missing outputs

File: .github/workflows/kurtosis_assertoor.yml
Lines: 188–267

The assertoor job defines outputs: passed: ${{ steps.result.outputs.passed }}, but commit-boost has no outputs. This is fine if no downstream job needs the result, but for consistency and future extensibility, consider adding a success/failure output.

8. YAML: commit_boost_params.yaml uses future date

File: tooling/kurtosis-assertoor/commit_boost_params.yaml
Line: 44

# bump them deliberately.
mev_params:
  # ... pin comments say "2026-10-26"

Issue: The comment says "2026-10-26" — this appears to be a future date (assuming current year is 2024 or 2025). If this is intentional (pre-planned pin date), document why. If it's a typo, correct it.

9. Documentation: Fork GC risk well-documented but needs action item

File: tooling/kurtosis-assertoor/README.md
Lines: 143–147

The README correctly identifies that COMMIT_BOOST_PACKAGE_REF may be garbage-collected if the fork PR is squash-merged. However, there's no tracking issue or TODO in the code to re-pin. Consider adding a TODO(#<issue>): comment in the workflow file near the pin, or a scheduled reminder.

10. Security: Docker image digests not fully pinned in workflow

File: .github/workflows/kurtosis_assertoor.yml

The workflow uses actions/checkout@v6 and actions/upload-artifact@v7 — these are version tags, not SHA pins. For a security-critical blockchain project, consider pinning to full commit SHAs for all GitHub Actions to mitigate supply-chain attacks. This is existing practice in the file, not new in this PR, but worth noting.

Positive Observations

  1. Good separation of concerns: The COMMIT_BOOST_PACKAGE_REF is separate from ETHEREUM_PACKAGE_REF, allowing independent updates.

  2. Clear dispatch-only gating: The if: github.event_name == 'workflow_dispatch' condition is well-explained in comments and README, with explicit guidance on when to remove it.

  3. Comprehensive metric coverage: The five conditions cover the full builder block flow: registration forwarding, payload sourcing, validator registration, and blinded block proposals through both native and third-party beacon nodes.

  4. Proper cleanup: Both assertoor and commit-boost jobs use if: always() for enclave removal and artifact upload.

  5. Documentation quality: The README is thorough, including local run instructions, CI usage, and the fork pin rationale.

Summary

Priority Item Location
High Fix notify job to depend on commit-boost .github/workflows/kurtosis_assertoor.yml
Medium Fix awk integer parsing in value_of scripts/check-builder-blocks.sh:63
Medium Cache metrics to avoid race conditions scripts/check-builder-blocks.sh:88
Low Add kurtosis availability check scripts/check-builder-blocks.sh (top)
Low Verify/fix date comment commit_boost_params.yaml:44
Low Add TODO for fork re-pin .github/workflows/kurtosis_assertoor.yml or README

Automated review by Kimi (Moonshot AI) · kimi-k2.6 · custom prompt

@MegaRedHand
ci: work around two upstream breakages in the Commit-Boost check
3a7bf07 
A local run did not start. The newest reth-rbuilder image parses only enode
bootnodes while the package passes ENRs, and Commit-Boost's PBS requires
SECONDS_PER_SLOT, which the genesis generator no longer writes.

The first is fixed in the fork (PR #23), so the package pin moves to that
commit. The second is worked around by giving Commit-Boost an explicit chain
through `commit_boost_config`; its `chain_id` must be a hex string. The README
lists both workarounds so they can be dropped when upstream is fixed, and
explains the harmless "Builder is not answering" logs at startup.
@MegaRedHand
ci: start spamoor in the Commit-Boost check
6372671 
A local run registered every validator but never got a builder block:
rbuilder submits no bid for a block without transactions, and the
enclave had none, so the relay held no bids for any proposer and every
getHeader answered 204. With transactions flowing, the same enclave
served a builder payload within a few slots, and the ethlambda
validator client signed the blinded block that the relay delivered.

spamoor runs with the package's default spammers (transfers and blob
transactions), pinned by digest like the other images. The README also
explains the no_finality fallbacks a run counts before the chain first
finalizes.
@MegaRedHand
Merge main into ci/kurtosis-commit-boost
ae289be
@MegaRedHand
ci: run spamoor at a light load in the Commit-Boost check
a211f49 
With spamoor's defaults (a chain-load group, 200 transfers and 6 blob
transactions a slot) the relay's getHeader slowed to Commit-Boost's
950 ms timeout, so most proposals got no bid, and in 40 minutes the
ethlambda validator client behind lighthouse never received one.

10 transfers and 1 blob transaction a slot keep bids coming without
that slowdown: a local run met every condition about 12 minutes after
the enclave started, and the relay delivered a builder block in 14 of
the 32 slots of epoch 1. The package's own 100 Uniswap swaps a slot,
which it adds whenever a builder runs, stay on; they are not
configurable.
@MegaRedHand
Merge main into ci/kurtosis-commit-boost
b259186
@MegaRedHand
Merge main into ci/kurtosis-commit-boost
1c9b9b7

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@MegaRedHand