YNU-858: prepare clearnode stress environment#646
Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdded a new stress-v1 environment and encrypted secrets; refactored signer/config initialization (runtime.go, main.go); introduced a new "storm" stress-test strategy with complex transfer/session flows; updated Helm chart templates/values for service accounts and GHCR image-pull secrets; adjusted stress-test invocation and reporting. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor CLI as "CLI"
participant Runner as "Storm Runner"
participant Signer as "Signer (local/GCP-KMS)"
participant RPC as "Blockchain RPCs"
participant Wallets as "Derived Wallets"
participant Report as "Reporter"
CLI->>Runner: clearnode stress-test storm <spec>
Runner->>Signer: initSigner (returns signer + closer)
Runner->>RPC: initBlockchainRPCs (load RPC endpoints)
Runner->>Wallets: derive origin + child wallets
loop Phases (forward/plateau/reverse)
Runner->>Wallets: instruct concurrent operations
Wallets->>Signer: sign txs/requests
Wallets->>RPC: submit txs / session ops
RPC-->>Wallets: confirmations/errors
Wallets-->>Runner: per-op results/timings
end
Runner->>Report: compute & print storm report (PASS/FAIL)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
clearnode/stress/README.md (1)
340-349:⚠️ Potential issue | 🟡 MinorThe Helm values example no longer matches the chart schema.
clearnode/chart/values.yaml:174-201documents the stress config understressTest.pods[].specs, but this snippet shows a top-levelstressTest.specs. Anyone copy-pasting this example will end up with values that do not match the current chart shape. Please update the doc example to usepods:and keep the strategy prefix in each spec.Suggested doc fix
stressTest: enabled: true - specs: - - "basic ping:100000:100" + pods: + - name: ping + specs: + - "basic ping:100000:100" privateKey: "<hex-key>" # optional, for state-mutating tests connections: 10 timeout: "10m" maxErrorRate: "0.01"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/stress/README.md` around lines 340 - 349, The README example shows stressTest.specs at the top level but the chart expects stressTest.pods[].specs; update the example so stressTest contains a pods array and each pod object carries specs, connections, timeout, privateKey and maxErrorRate as documented (i.e., use stressTest.pods[].specs instead of stressTest.specs) and ensure each spec string keeps the required strategy prefix per the chart (preserve the strategy token at the start of each spec entry).clearnode/chart/templates/env-secret.yaml (1)
1-15:⚠️ Potential issue | 🟠 MajorBroaden the guard so DB-only secrets still render.
The new
CLEARNODE_DATABASE_PASSWORDentry is still inside the top-levelif .Values.config.secretEnvs, so a deployment that only needs the database password will render no Secret at all. That leaves the password unset even thoughclearnode.common.envstill emits the other DB env vars.♻️ Minimal fix
-{{- if .Values.config.secretEnvs }} +{{- if or .Values.config.secretEnvs (and .Values.config.database .Values.config.database.password) }} apiVersion: v1 kind: Secret metadata:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/chart/templates/env-secret.yaml` around lines 1 - 15, The Secret is only rendered when .Values.config.secretEnvs is true so deployments that only set .Values.config.database get no Secret; update the top-level conditional around the Secret (the template that uses include "clearnode.common.fullname" and annotations) to render when either .Values.config.secretEnvs or .Values.config.database is present (e.g. use an or check), and keep the CLEARNODE_DATABASE_PASSWORD key generation scoped to the existing with .Values.config.database block so the password is only added if database data exists.clearnode/helmfile.yaml.gotmpl (1)
16-24:⚠️ Potential issue | 🟠 Major
action_gateway.yamlis no longer wired into the release.
clearnode/chart/templates/configmap.yamlLines 11-17 still render.Values.config.actionGateway, andclearnode/chart/values.yamldefaults it to empty. With thisset:block only populatingblockchainsandassets,/app/config/action_gateway.yamlbecomes empty; any environment withActionLimitsEnabledwill fail whenclearnode/runtime.goLines 141-149 try to load it.♻️ One straightforward fix
set: - name: image.tag value: '{{ requiredEnv "IMAGE_TAG" }}' - name: config.blockchains file: chart/config/{{ .Environment.Name }}/blockchains.yaml - name: config.assets file: chart/config/{{ .Environment.Name }}/assets.yaml + - name: config.actionGateway + file: chart/config/{{ .Environment.Name }}/action_gateway.yaml🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/helmfile.yaml.gotmpl` around lines 16 - 24, The release no longer populates .Values.config.actionGateway (rendered by the configmap template) causing runtime.go to fail when ActionLimitsEnabled loads action gateway config; fix the helmfile set block by adding a set entry that populates the config.actionGateway value (name: config.actionGateway) from the environment-specific action_gateway.yaml so the configmap has the expected content and runtime.go’s loader (the code around the ActionLimitsEnabled check) receives a non-empty config.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clearnode/chart/templates/full-deployment.yaml`:
- Around line 31-33: Update the template so it preserves a pre-created service
account name when serviceAccount.create is false: if .Values.serviceAccount.name
is set, always emit serviceAccountName: {{ .Values.serviceAccount.name }},
otherwise fall back to the existing behavior (emit serviceAccountName: {{
include "clearnode.common.fullname" . }} only when serviceAccount.create is
true). Also add a new key serviceAccount.name: null to values.yaml so
environments can override the SA name (documenting the field for overrides like
stress-v1/clearnode.yaml).
In `@clearnode/chart/templates/image-pull-secret.yaml`:
- Around line 8-11: Remove the Helm hook annotations from the Secret template so
it is managed as a normal release resource: delete the "helm.sh/hook",
"helm.sh/hook-delete-policy", and "helm.sh/hook-weight" entries in the
annotations block of the image-pull Secret template (the annotations block shown
in the diff), ensuring the Secret remains a standard tracked resource by Helm
and will be deleted on helm uninstall; keep the rest of the Secret metadata/spec
intact so Helm can manage its lifecycle.
In `@clearnode/chart/values.yaml`:
- Around line 98-103: The values schema removed support for specifying an
existing ServiceAccount name; update values.yaml to accept serviceAccount.name
alongside serviceAccount.create, and then modify the workload template (where
serviceAccountName is set) to use serviceAccount.name when provided, otherwise
use the generated name only when serviceAccount.create is true; ensure the
template logic checks serviceAccount.name first, falls back to the generated
name only if create: true, and does not inject a default name when create: false
to preserve backward compatibility.
In `@clearnode/main.go`:
- Around line 247-254: The operator address must still be printed to stdout
regardless of log level: after initSigner(...) (and before/after the existing
logger.Info call) write the raw address from
signer.PublicKey().Address().String() to stdout (e.g., fmt.Println or os.Stdout)
so the CLI always outputs the plain operator address; keep the existing
logger.Info line for structured logging but add the explicit stdout print using
the signer/PublicKey/Address symbols.
In `@clearnode/stress/README.md`:
- Around line 26-44: The fenced code blocks in the README (e.g., the CLI snippet
starting with "clearnode stress-test <strategy> [args...]" and other unlabeled
fences at ranges referenced) are missing language tags and trigger markdownlint
MD040; update each triple-backtick fence to include an appropriate language
identifier (for example use "bash" for CLI examples, "yaml" for config snippets,
or "text" for plain diagrams) for the fences around the shown block and the
additional ranges noted (72-74, 216-226, 278-288) so markdownlint no longer
flags them.
In `@clearnode/stress/run.go`:
- Around line 21-30: The new required strategy token breaks legacy specs; modify
the argument handling in the switch over args[0] so legacy spec strings (e.g.,
those containing ":" like "ping:100:5") are accepted as the old default by
routing them to runBasic. Concretely, update the switch in the code that
inspects args[0] to detect either a known strategy ("basic", "storm") or a
legacy-spec pattern (e.g., contains ':' or matches the legacy regex) and in the
legacy case call runBasic with the original args (runBasic(args) or
runBasic(args[0:])), leaving runStorm for "storm" and keeping the unknown branch
with printUsage unchanged for truly invalid input. Ensure you reference the same
args variable and functions runBasic and runStorm so existing chart/template
invocations (which pass specs verbatim) remain compatible.
In `@clearnode/stress/storm.go`:
- Around line 183-187: The loop launching sender goroutines (the for s := range
sendersCount block) currently returns immediately when ctx.Err() is non-nil,
which can close resources while launched workers are still running; change the
behavior to stop launching further workers (break the loop) and then wait for
started workers to finish (call wg.Wait() or use the existing errgroup pattern)
before returning the error code; apply the same fix to the analogous ctx.Err()
branches referenced around the other launch phases (the blocks at ~235-239,
~482-486, ~543-547) so each early-cancel path breaks/ends the launch phase and
waits for the corresponding worker WaitGroup or errgroup to finish before
cleaning up and returning.
- Around line 496-498: The current nonce calculation (nonce :=
uint64(iter)*10000 + uint64(sessionIdx)) can collide when iter grows; replace it
with a collision-free encoding that uniquely combines iter and sessionIdx (e.g.
pack iter into the high bits and sessionIdx into the low bits or multiply by a
guaranteeable larger base derived from max sessions), update the places that
construct nonces for forward sessions (where executeForwardSession is called)
and the analogous reverse session call sites (the other occurrence around lines
557-559) to use the same packing scheme, and ensure types (uint64) are used for
both parts so the resulting nonce cannot collide across iterations or session
indexes.
- Around line 212-217: The failure-reporting logic prints the storm report
before the current batch (iterResults) is appended to the cumulative results,
causing totals and failure counts to omit the failing batch; to fix, move the
append of iterResults into results so it happens before calling printStormReport
(i.e., ensure results = append(results, iterResults...) executes prior to
printStormReport("storm-transfers", results, time.Since(start))) in the
forward-phase error branch handling collectErrors, and apply the same reorder in
the other three failure branches that use
collectErrors/iterResults/printStormReport (the other occurrences around the
same patterns should be updated similarly).
- Around line 63-102: Limit and validate iterations before using it to size
structures: introduce a maxIterations constant (e.g. 30 or another safe bound),
reject values > maxIterations or < 1 with an error, and replace the math.Pow
float-based sizing with integer-safe arithmetic (compute totalNodes as
1<<iterations or using checked int64 shifts) plus explicit overflow checks so
totalNodes, derivedWallets and any slice sizes (e.g. wallets/nodes allocations
referenced later) cannot grow unbounded; compute originAmount using integer
totalNodes (originAmount := amount.Mul(decimal.NewFromInt(int64(totalNodes))))
and update all places that used math.Pow or float-to-int conversions (including
the other region around lines 332-372) to use these capped, integer-based
calculations.
In `@pkg/core/state_packer.go`:
- Line 6: Remove direct writes of signing payloads to stderr (the
Fprintf(os.Stderr, ...) calls) and instead log errors via the package logger
with proper redaction or omit printing the sensitive payload entirely; replace
the stderr Fprintf occurrences (and the "os" import if it becomes unused) with
calls to the existing logger/error-handling path (e.g., use the package's logger
or return the error) in the function where Fprintf(os.Stderr, ...) appears and
in the similar block around the other occurrences (lines ~163-170) so balances,
net flows, and token addresses are not emitted raw.
---
Outside diff comments:
In `@clearnode/chart/templates/env-secret.yaml`:
- Around line 1-15: The Secret is only rendered when .Values.config.secretEnvs
is true so deployments that only set .Values.config.database get no Secret;
update the top-level conditional around the Secret (the template that uses
include "clearnode.common.fullname" and annotations) to render when either
.Values.config.secretEnvs or .Values.config.database is present (e.g. use an or
check), and keep the CLEARNODE_DATABASE_PASSWORD key generation scoped to the
existing with .Values.config.database block so the password is only added if
database data exists.
In `@clearnode/helmfile.yaml.gotmpl`:
- Around line 16-24: The release no longer populates
.Values.config.actionGateway (rendered by the configmap template) causing
runtime.go to fail when ActionLimitsEnabled loads action gateway config; fix the
helmfile set block by adding a set entry that populates the config.actionGateway
value (name: config.actionGateway) from the environment-specific
action_gateway.yaml so the configmap has the expected content and runtime.go’s
loader (the code around the ActionLimitsEnabled check) receives a non-empty
config.
In `@clearnode/stress/README.md`:
- Around line 340-349: The README example shows stressTest.specs at the top
level but the chart expects stressTest.pods[].specs; update the example so
stressTest contains a pods array and each pod object carries specs, connections,
timeout, privateKey and maxErrorRate as documented (i.e., use
stressTest.pods[].specs instead of stressTest.specs) and ensure each spec string
keeps the required strategy prefix per the chart (preserve the strategy token at
the start of each spec entry).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 52839136-cf1c-437a-a667-9df73cfbc4ff
📒 Files selected for processing (19)
clearnode/chart/config/secrets.yamlclearnode/chart/config/stress-v1/assets.yamlclearnode/chart/config/stress-v1/blockchains.yamlclearnode/chart/config/stress-v1/clearnode.yamlclearnode/chart/config/stress-v1/secrets.yamlclearnode/chart/templates/debug-deployment.yamlclearnode/chart/templates/env-secret.yamlclearnode/chart/templates/full-deployment.yamlclearnode/chart/templates/helpers/_common.tplclearnode/chart/templates/image-pull-secret.yamlclearnode/chart/templates/service-account.yamlclearnode/chart/values.yamlclearnode/helmfile.yaml.gotmplclearnode/main.goclearnode/runtime.goclearnode/stress/README.mdclearnode/stress/run.goclearnode/stress/storm.gopkg/core/state_packer.go
💤 Files with no reviewable changes (1)
- clearnode/chart/templates/debug-deployment.yaml
philanton
force-pushed
the
feat/stress-env
branch
from
4e12be6 to
ec12464
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
clearnode/stress/README.md (1)
364-369:⚠️ Potential issue | 🟡 MinorHelm values example currently double-prefixes the strategy.
Line 368 includes
basic ..., but the Job command already prependsbasic, so copied config becomesstress-test basic basic ....Suggested doc fix
stressTest: enabled: true specs: - - "basic ping:100000:100" + - "ping:100000:100"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/stress/README.md` around lines 364 - 369, The example Helm values double-prefix the strategy: the specs entry currently uses "basic ping:100000:100" but the Job invocation already prepends the strategy name; update the example under stressTest.specs to omit the strategy (e.g., "ping:100000:100" or whatever payload format your Job expects) so the final command isn't "basic basic ...", and adjust any documentation text that references the example accordingly.
♻️ Duplicate comments (1)
clearnode/stress/README.md (1)
26-44:⚠️ Potential issue | 🟡 MinorAdd language tags to remaining fenced code blocks (MD040).
The code fences starting at Line 26, Line 72, Line 217, and Line 295 are still unlabeled, so markdownlint warnings remain.
Also applies to: 72-74, 217-233, 295-309
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/stress/README.md` around lines 26 - 44, The README contains unlabeled fenced code blocks (e.g., the ASCII diagram starting with "clearnode stress-test <strategy> [args...]" and the other blocks in the same README) which triggers markdownlint MD040; update each triple-backtick fence to include an appropriate language tag (for example ```text, ```bash or ```console) so the blocks are explicitly labeled; locate the fences around the ASCII tree/command examples and the other unlabeled blocks (the ones containing the stress-test CLI usage and related examples) and replace the opening ``` with the chosen language tag for each block.
🧹 Nitpick comments (2)
clearnode/chart/templates/image-pull-secret.yaml (1)
14-14: Consider removing thequotefilter for base64-encoded secret data.While functionally correct, base64-encoded values in Kubernetes Secret
datafields are conventionally rendered without quotes. The standard pattern is to render them directly.♻️ Proposed refinement
data: - .dockerconfigjson: {{ .Values.ghcrPullDockerConfigJson | quote }} + .dockerconfigjson: {{ .Values.ghcrPullDockerConfigJson }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/chart/templates/image-pull-secret.yaml` at line 14, Remove the unnecessary quote filter so the base64-encoded secret data is rendered unquoted: in the image-pull-secret.yaml template update the .dockerconfigjson line to use .Values.ghcrPullDockerConfigJson directly (remove the | quote) so the Kubernetes Secret data field emits the raw base64 string instead of a quoted string.clearnode/main.go (1)
294-298: Consider closing ethclient after use.The
ethclient.Dialconnection at Line 294 is never closed. While the OS reclaims resources on process exit, explicit cleanup is cleaner, especially if the loop iterates over many blockchains.♻️ Suggested fix
client, err := ethclient.Dial(rpcURL) if err != nil { logger.Fatal("failed to connect to EVM Node", "blockchainID", b.ID) } + defer client.Close()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/main.go` around lines 294 - 298, The ethclient created by ethclient.Dial is never closed; after successfully creating the client (the client variable returned by ethclient.Dial) add explicit cleanup (e.g., defer client.Close() immediately after the nil err check) so the connection is closed when done and resources aren’t leaked during iterations over blockchains; make sure this is added in the same scope where logger.Fatal("failed to connect to EVM Node", "blockchainID", b.ID) is used so it runs for the lifespan of the client.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clearnode/stress/README.md`:
- Around line 18-20: The storm quick-start example is missing the required
cycles parameter; update the example command (the "clearnode stress-test storm
transfers:3:usdc:1" string in the README) to include a cycles value (e.g.,
append ":cycles:<n>") so it matches the documented spec, and make the same
change for the other occurrence around line 197; ensure the format follows the
existing pattern (transfers:<count>:<token>:<amount>:cycles:<number>).
---
Outside diff comments:
In `@clearnode/stress/README.md`:
- Around line 364-369: The example Helm values double-prefix the strategy: the
specs entry currently uses "basic ping:100000:100" but the Job invocation
already prepends the strategy name; update the example under stressTest.specs to
omit the strategy (e.g., "ping:100000:100" or whatever payload format your Job
expects) so the final command isn't "basic basic ...", and adjust any
documentation text that references the example accordingly.
---
Duplicate comments:
In `@clearnode/stress/README.md`:
- Around line 26-44: The README contains unlabeled fenced code blocks (e.g., the
ASCII diagram starting with "clearnode stress-test <strategy> [args...]" and the
other blocks in the same README) which triggers markdownlint MD040; update each
triple-backtick fence to include an appropriate language tag (for example
```text, ```bash or ```console) so the blocks are explicitly labeled; locate the
fences around the ASCII tree/command examples and the other unlabeled blocks
(the ones containing the stress-test CLI usage and related examples) and replace
the opening ``` with the chosen language tag for each block.
---
Nitpick comments:
In `@clearnode/chart/templates/image-pull-secret.yaml`:
- Line 14: Remove the unnecessary quote filter so the base64-encoded secret data
is rendered unquoted: in the image-pull-secret.yaml template update the
.dockerconfigjson line to use .Values.ghcrPullDockerConfigJson directly (remove
the | quote) so the Kubernetes Secret data field emits the raw base64 string
instead of a quoted string.
In `@clearnode/main.go`:
- Around line 294-298: The ethclient created by ethclient.Dial is never closed;
after successfully creating the client (the client variable returned by
ethclient.Dial) add explicit cleanup (e.g., defer client.Close() immediately
after the nil err check) so the connection is closed when done and resources
aren’t leaked during iterations over blockchains; make sure this is added in the
same scope where logger.Fatal("failed to connect to EVM Node", "blockchainID",
b.ID) is used so it runs for the lifespan of the client.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3bfa7bf7-b953-453f-b8e9-7c38b8a43fd1
📒 Files selected for processing (21)
clearnode/chart/config/secrets.yamlclearnode/chart/config/stress-v1/assets.yamlclearnode/chart/config/stress-v1/blockchains.yamlclearnode/chart/config/stress-v1/clearnode.yamlclearnode/chart/config/stress-v1/secrets.yamlclearnode/chart/templates/debug-deployment.yamlclearnode/chart/templates/env-secret.yamlclearnode/chart/templates/full-deployment.yamlclearnode/chart/templates/helpers/_common.tplclearnode/chart/templates/image-pull-secret.yamlclearnode/chart/templates/service-account.yamlclearnode/chart/templates/tests/stress-test.yamlclearnode/chart/values.yamlclearnode/helmfile.yaml.gotmplclearnode/main.goclearnode/runtime.goclearnode/stress/README.mdclearnode/stress/report.goclearnode/stress/run.goclearnode/stress/storm.gopkg/core/state_packer.go
💤 Files with no reviewable changes (1)
- clearnode/chart/templates/debug-deployment.yaml
✅ Files skipped from review due to trivial changes (5)
- clearnode/chart/config/secrets.yaml
- clearnode/chart/config/stress-v1/secrets.yaml
- clearnode/chart/config/stress-v1/assets.yaml
- clearnode/chart/config/stress-v1/clearnode.yaml
- clearnode/stress/storm.go
🚧 Files skipped from review as they are similar to previous changes (8)
- clearnode/chart/templates/env-secret.yaml
- pkg/core/state_packer.go
- clearnode/helmfile.yaml.gotmpl
- clearnode/chart/templates/full-deployment.yaml
- clearnode/chart/templates/helpers/_common.tpl
- clearnode/chart/values.yaml
- clearnode/chart/config/stress-v1/blockchains.yaml
- clearnode/stress/run.go
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
clearnode/stress/README.md (1)
26-44:⚠️ Potential issue | 🟡 MinorAdd language identifiers to fenced code blocks to clear MD040.
Line 26, Line 72, Line 217, Line 295, and Line 328 use unlabeled triple-backtick blocks. Please tag each fence (
text,bash, etc.) so markdownlint passes.Proposed doc patch
-``` +```text clearnode stress-test <strategy> [args...] │ ├── basic ──► Individual method stress tests @@ └── storm ──► Cascading tree-based stress patterns │ ├── transfers: binary-tree transfer cascade └── sessions: ternary-growth app session cascade@@
-+bash
clearnode stress-test basic method:total_requests[:connections[:extra_params...]]@@ -``` +```text Forward: Iteration 1: A -> B (4 usdc) @@ Iteration 1: B -> A@@
-+text
Forward:
Iteration 1: session(A,B,C) — A deposits 6, reallocates 3 to B, 3 to C
@@
Iteration 1: B,C -> A@@ -``` +```text Stress Test Report ================== Method: ping @@ context deadline exceeded 2</details> Also applies to: 72-74, 217-233, 295-309, 328-354 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@clearnode/stress/README.mdaround lines 26 - 44, Add language identifiers to
the unlabeled fenced code blocks in the README: tag the block that starts with
"clearnode stress-test [args...]" as text, tag the block that starts
"clearnode stress-test basic
method:total_requests[:connections[:extra_params...]]" as bash, and tag the
three example/result blocks beginning with "Forward:", "Forward: Iteration 1:
session(A,B,C) — A deposits 6, reallocates 3 to B, 3 to C", and "Stress Test
Report" as text; update the correspondingfences totext or ```bash as
proposed so markdownlint MD040 no longer reports unlabeled code fences.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In@clearnode/stress/README.md:
- Around line 26-44: Add language identifiers to the unlabeled fenced code
blocks in the README: tag the block that starts with "clearnode stress-test
[args...]" as text, tag the block that starts "clearnode stress-test
basic method:total_requests[:connections[:extra_params...]]" as bash, and tag
the three example/result blocks beginning with "Forward:", "Forward: Iteration
1: session(A,B,C) — A deposits 6, reallocates 3 to B, 3 to C", and "Stress Test
Report" as text; update the correspondingfences totext or ```bash as
proposed so markdownlint MD040 no longer reports unlabeled code fences.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `dfa366f7-85cb-4470-b5d1-c0770d46d5e8` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between ec12464d448c49e2399bc77a28df057c97045081 and a034c2921395545897564f4c6c1882b9e4d6200a. </details> <details> <summary>📒 Files selected for processing (2)</summary> * `clearnode/chart/templates/image-pull-secret.yaml` * `clearnode/stress/README.md` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
nksazonov
approved these changes
Apr 6, 2026
Contributor
nksazonov
left a comment
nksazonov
left a comment
There was a problem hiding this comment.
Great work! LGTM
Left some doc-related and storm strategy comments
clearnode/stress/storm.go
Outdated
| ) | ||
|
|
||
| // runStorm is the entry point for the "storm" stress test strategy. | ||
| // Currently supports: transfers |
| fmt.Printf(" Cycles: %d\n", cycles) | ||
| fmt.Printf(" Asset: %s\n", asset) | ||
| fmt.Printf(" Amount/leaf: %s\n", amount.String()) | ||
| fmt.Printf(" Origin needs: %s %s\n", originAmount.String(), asset) |
Contributor
There was a problem hiding this comment.
I think it is worth checking whether origin has this amount of funds prior to any operations.
| forwardOps := forwardSessionsTotal * 4 | ||
| reverseOps := reverseSessionsTotal * 5 | ||
| totalOps := forwardOps + plateauOps + reverseOps | ||
| originAmount := amount.Mul(decimal.NewFromInt(int64(totalNodes))) |
Contributor
There was a problem hiding this comment.
The same here: it is worth checking whether origin address holds origin amount of the token prior to any ops.
Contributor
Author
There was a problem hiding this comment.
That's done here as well
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
clearnode/stress/storm.go (1)
605-670: Plateau nonce encoding also needs the same collision-free scheme.Lines 626 and 654 use
(100+c)*10000and(200+c)*10000multipliers, which could still collide with forward/reverse phase nonces at high iteration counts. Apply the same bit-shifting fix recommended for lines 570 and 701.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/stress/storm.go` around lines 605 - 670, The plateau back/forth nonces (used when calling executeReverseSession and executeForwardSession) still use "(100+c)*10000" and "(200+c)*10000" and can collide; replace those arithmetic expressions with a collision-free bit-field encoding that packs a phase identifier, the cycle number (c) and the session index (sessionIdx) into distinct bit ranges via bit-shifts so every phase uses a unique high-bit prefix (e.g., distinct phase IDs for plateau-back vs plateau-forth) and lower bits hold c and sessionIdx; update the nonce assignments before calling executeReverseSession and executeForwardSession accordingly so they match the same collision-free scheme used elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clearnode/stress/storm.go`:
- Around line 914-915: In executeReverseSession the pattern of ignoring errors
from PackAppStateUpdateV1 and stormSignAll must be fixed: check and handle the
returned errors for each call (e.g., PackAppStateUpdateV1(...)->deposit1Req, err
and stormSignAll(...)->deposit1Sigs, err), return or propagate the error (or log
and return) instead of discarding it; apply the same change to the other
occurrences mentioned (the analogous PackAppStateUpdateV1 and stormSignAll calls
at the other blocks that produce deposit2Req/deposit2Sigs, settleReq/settleSigs,
and finalReq/finalSigs) so no PackAppStateUpdateV1 or stormSignAll call ignores
its error.
- Around line 798-799: PackAppStateUpdateV1 and stormSignAll are currently
ignoring errors (using `_`) which can pass malformed data downstream; update the
deposit, reallocate, and close steps to capture the error returns from
PackAppStateUpdateV1 and stormSignAll (e.g., depositReq, err :=
app.PackAppStateUpdateV1(...); depositSigs, err := stormSignAll(...)), check and
handle errors the same way as the create step does (log/return/abort on error
using the same processLogger or error handling pattern used around the create
flow), and ensure you propagate/stop execution when packing or signing fails
rather than proceeding with nil/invalid values.
---
Nitpick comments:
In `@clearnode/stress/storm.go`:
- Around line 605-670: The plateau back/forth nonces (used when calling
executeReverseSession and executeForwardSession) still use "(100+c)*10000" and
"(200+c)*10000" and can collide; replace those arithmetic expressions with a
collision-free bit-field encoding that packs a phase identifier, the cycle
number (c) and the session index (sessionIdx) into distinct bit ranges via
bit-shifts so every phase uses a unique high-bit prefix (e.g., distinct phase
IDs for plateau-back vs plateau-forth) and lower bits hold c and sessionIdx;
update the nonce assignments before calling executeReverseSession and
executeForwardSession accordingly so they match the same collision-free scheme
used elsewhere.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+798
to
+799
| depositReq, _ := app.PackAppStateUpdateV1(depositUpdate) | ||
| depositSigs, _ := stormSignAll(signers, depositReq) |
Contributor
There was a problem hiding this comment.
Silently ignored errors from PackAppStateUpdateV1 and stormSignAll.
These calls discard errors with _. If packing or signing fails, the subsequent client call receives malformed data, producing confusing downstream errors. Handle these errors similarly to the create step (lines 767-778).
Proposed fix for deposit step (apply similar pattern to reallocate and close)
- depositReq, _ := app.PackAppStateUpdateV1(depositUpdate)
- depositSigs, _ := stormSignAll(signers, depositReq)
+ depositReq, err := app.PackAppStateUpdateV1(depositUpdate)
+ if err != nil {
+ results[1] = Result{Err: fmt.Errorf("pack deposit: %w", err)}
+ skipResults(results, 2, results[1].Err)
+ return results
+ }
+ depositSigs, err := stormSignAll(signers, depositReq)
+ if err != nil {
+ results[1] = Result{Err: fmt.Errorf("sign deposit: %w", err)}
+ skipResults(results, 2, results[1].Err)
+ return results
+ }Also applies to: 821-822, 844-845
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clearnode/stress/storm.go` around lines 798 - 799, PackAppStateUpdateV1 and
stormSignAll are currently ignoring errors (using `_`) which can pass malformed
data downstream; update the deposit, reallocate, and close steps to capture the
error returns from PackAppStateUpdateV1 and stormSignAll (e.g., depositReq, err
:= app.PackAppStateUpdateV1(...); depositSigs, err := stormSignAll(...)), check
and handle errors the same way as the create step does (log/return/abort on
error using the same processLogger or error handling pattern used around the
create flow), and ensure you propagate/stop execution when packing or signing
fails rather than proceeding with nil/invalid values.
Comment on lines
+914
to
+915
| deposit1Req, _ := app.PackAppStateUpdateV1(deposit1Update) | ||
| deposit1Sigs, _ := stormSignAll(signers, deposit1Req) |
Contributor
There was a problem hiding this comment.
Same ignored-error pattern in executeReverseSession.
Apply the same error handling as suggested for executeForwardSession to all PackAppStateUpdateV1 and stormSignAll calls here.
Also applies to: 935-936, 958-959, 981-982
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clearnode/stress/storm.go` around lines 914 - 915, In executeReverseSession
the pattern of ignoring errors from PackAppStateUpdateV1 and stormSignAll must
be fixed: check and handle the returned errors for each call (e.g.,
PackAppStateUpdateV1(...)->deposit1Req, err and stormSignAll(...)->deposit1Sigs,
err), return or propagate the error (or log and return) instead of discarding
it; apply the same change to the other occurrences mentioned (the analogous
PackAppStateUpdateV1 and stormSignAll calls at the other blocks that produce
deposit2Req/deposit2Sigs, settleReq/settleSigs, and finalReq/finalSigs) so no
PackAppStateUpdateV1 or stormSignAll call ignores its error.
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
2 participants
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 by CodeRabbit
New Features
Chores
Documentation