Conversation
PR SummaryMedium Risk Overview EVM funding for steady-state: adds sei-load harness: load is sent only to the first RPC follower; receipts and the inclusion gate use the second. Profile rendering adds Reviewed by Cursor Bugbot for commit 0937db2. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
This PR adds TestNightlyLoadRegression, which funds a throwaway EVM root key at genesis, runs a 200 TPS sei-load Job against a fresh 4-validator chain, and gates included TPS, block-interval p50/p90 and the validators' CPU and memory against a recorded baseline; it also pulls the profile-fill, Job-run, block-page and key-Secret helpers out for sharing. Nothing blocks, so this is an approve with two non-blocking notes; I dropped codex's retry-pod finding because the Job template sets backoffLimit: 0 with restartPolicy: Never, so the Job has only one seiload pod and seiloadRunTimes cannot pick a failed earlier attempt.
Non-blocking
1 finding on the changed lines, as inline comments.
- The gate logic that decides nightly pass/fail (Measure's window-coverage and edge handling, nearest-rank percentile, Check's direction and max(pct, abs) allowance, and parseBaseline's rejection rules) has no unit tests, and all of it is pure functions. A sign or allowance bug would only show up as a spurious or missed REGRESSION in the nightly, so a small table test in the package would catch it at PR time instead.
1 nit, not posted on the code
test/integration/loadregression_test.go:158— time.Sleep here ignores ctx, so a SIGTERM during the wait isn't handled until the sleep ends. The wait is at most about 30s becausefinished >= tohas already been checked.
seidroid review · decision approve · session 2aab5e4ee3f949a5afe79adca1434e1e · turn resp_claude_fd6d583295e5e45ce233f683c52f645f · item b7ce020afa5550ada791e035cb83e9f5
Findings: 0 blocking | 2 non-blocking | 1 posted inline
| // change to one is reviewed with the others. TestNightlyLoadRegression in | ||
| // test/integration runs it. | ||
| // | ||
| // To change the workload, edit profile.json (sei-load's profile schema) and |
There was a problem hiding this comment.
suggestion — The package doc says a mismatch between profile.json and baseline.json's config block "fails this package's tests", but test/integration/loadregression has no _test.go files. Today a mismatch only shows up when the nightly starts and fails the Comparable check as UNEVALUABLE, not at PR time. Either add the unit test that calls RunConfig/RecordedBaseline/Comparable, or change the doc.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7544b84. Configure here.
bdchatham
left a comment
There was a problem hiding this comment.
Thanks for putting this together, the fail-closed handling and the split between UNEVALUABLE and REGRESSION are really thoughtful. Most of my comments come down to one direction, where the harness runs the load and reports whether the run was valid, and alerts become the validation layer for performance.
On the PR body, could you link a companion platform PR in the description? I'm thinking new rules for committed TPS, block interval p50/p90 and validator CPU/memory against a 7-day median, plus scoping the existing benchmark alerts to workload!="load-regression".
| return evmIdentityFromKey(priv) | ||
| } | ||
|
|
||
| func evmIdentityFromKey(priv *btcec.PrivateKey) (EVMIdentity, error) { |
There was a problem hiding this comment.
Could we add a fixed-key vector test for evmIdentityFromKey, the way hd_test.go pins Derive? A wrong address here would only show up as an unfunded root on a live chain.
| h.Write(uncompressed[1:]) | ||
| addr := h.Sum(nil)[12:] | ||
|
|
||
| converted, err := bech32.ConvertBits(addr, 8, 5, true) |
There was a problem hiding this comment.
The ConvertBits and bech32 Encode steps mirror keygen.go:77, so I'd pull them into one helper and keep the address format defined in a single place.
| // image (included TPS 0.003%, p90 1.6%, CPU 1.2%, memory 1.8%). p50 is the | ||
| // exception: it moved 7.1% between those runs, so it keeps a 10% allowance. | ||
| // Revisit them once the baseline holds a week of nightlies. | ||
| package loadregression |
There was a problem hiding this comment.
This is the first subpackage under test/integration, and our shared harness code lives in harness/ today, so I'd lean towards harness/loadregression or folding it into the test package.
| // recorded with and what the tracker needs to be trustworthy. | ||
| func RenderProfile(chainID, sendEVM, receiptEVM string) string { | ||
| return strings.ReplaceAll(bench.FillProfile(profileTmpl, chainID, []string{sendEVM}), | ||
| "__RECEIPT_ENDPOINT__", receiptEVM) |
There was a problem hiding this comment.
Adding __RECEIPT_ENDPOINT__ to bench.FillProfile would keep every profile placeholder in one replacer, rather than a second ReplaceAll layered on top.
| hc := &http.Client{Timeout: 10 * time.Second} | ||
| // rpc-0 takes the sends; rpc-1 takes none and is where receipts, heads and | ||
| // the measured blocks are read. | ||
| sendNode, receiptNode := ch.rpcNodes[0], ch.rpcNodes[1] |
There was a problem hiding this comment.
ch.rpcNodes[0] and [1] would panic if RPCNodes ever drops below 2, so a len check that fails as UNEVALUABLE keeps that failure readable.
| } | ||
| t.Logf("load-regression result: %s", result) | ||
|
|
||
| v := loadregression.Check(baseline, m.Metrics) |
There was a problem hiding this comment.
I'd like the test to fail only when the run itself isn't valid data (halt or lag, a short window, missing coverage, a high revert ratio), and let alerts own the regression call.
| @@ -0,0 +1,260 @@ | |||
| package loadregression | |||
There was a problem hiding this comment.
With alerts owning the comparison, I think the baseline, threshold and Check logic plus resources.go can go, since PrometheusRules can compare each night against the trailing nightlies without a hand-maintained baseline file.
| github.com/pelletier/go-toml/v2 v2.2.4 // indirect | ||
| github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect | ||
| github.com/prometheus/client_golang v1.23.2 // indirect | ||
| github.com/prometheus/client_golang v1.23.2 |
There was a problem hiding this comment.
Once resources.go is gone, client_golang and prometheus/common can drop back to indirect.
| t.Fatalf("UNEVALUABLE: %v", err) | ||
| } | ||
|
|
||
| promURL := mustEnv(t, "PROMETHEUS_URL") |
There was a problem hiding this comment.
Dropping PROMETHEUS_URL also means the harness needs no Prometheus access, which is one less piece of CronJob wiring to land in platform.
| Image: s.seiloadImage, | ||
| DurationMinutes: s.durationMin, | ||
| ProfileCM: profileCM, | ||
| Workload: "load-regression", |
There was a problem hiding this comment.
Let's keep Workload: "load-regression", since it's the label the new alerts scope on and the one the existing benchmark alerts will need to exclude.
|
@kollegian one more thought after sitting with this a bit longer. With the regression call moving to alerts, this suite and The two runs still answer different questions, so I'd keep both workloads. The benchmark measures saturation throughput, and this run measures what a fixed 200 TPS load costs the chain, which is where CPU, memory and block interval become comparable from night to night. A shape I think would work:
One thing I haven't checked is whether the AMM and ERC20 scenarios run on the |

No description provided.