chore(deps): security #3296
Conversation
📝 WalkthroughWalkthroughThe PR updates EVM test infrastructure by changing genesis hash/state root constants, upgrading tastora dependency to v0.19.0 across modules, refactoring readiness checks to use eth_getBlockByNumber and engine_forkchoiceUpdatedV3, migrating Docker client APIs from docker/docker to moby/moby, and adding new Spamoor and VictoriaTraces benchmark nodes for e2e benchmarking. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
|
Claude finished @tac0turtle's task in 2m 35s —— View job Code Review
SummaryThis PR makes two distinct changes: (1) security/maintenance dependency bumps, and (2) new benchmark test infrastructure. The dep migration from Dependency ChangesPositive changes:
Observation:
|
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3296 +/- ##
==========================================
- Coverage 62.52% 62.51% -0.02%
==========================================
Files 122 122
Lines 13047 13047
==========================================
- Hits 8158 8156 -2
- Misses 4004 4008 +4
+ Partials 885 883 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
test/docker-e2e/resiliency_test.go (1)
890-947:⚠️ Potential issue | 🟠 MajorDemultiplex Docker streams and verify exec exit codes; silent command failures invalidate corruption test.
When
ExecCreateOptionsomitsTty(defaults to false), bothExecAttachandContainerLogsreturn multiplexed stdout/stderr using Docker's 8-byte header protocol (STREAM_TYPE + size). Reading withio.ReadAllorio.Copyincludes these framing bytes in the output. More critically, the corruption commands (lines 729, 738, 747) are wrapped insh -cwith2>/dev/null || true, masking failures. The code checks for errors fromExecCreateandExecStartbut never callsExecInspectto retrieve the actual exit code. This means a failed corruption command appears successful, causingTestDataCorruptionRecoveryto pass without actually corrupting data—a false positive. Usestdcopy.StdCopyfromgithub.com/moby/moby/api/pkg/stdcopyto demultiplex, and callExecInspectto check the exit code before returning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/docker-e2e/resiliency_test.go` around lines 890 - 947, The exec and log readers are returning Docker's multiplexed stream frames and the exec path never checks the command exit code, so failures get masked; in execCommandInContainer use github.com/moby/moby/api/pkg/stdcopy.StdCopy to demultiplex resp.Reader into separate stdout/stderr buffers (instead of io.ReadAll) and after ExecStart call s.dockerClient.ExecInspect(ctx, exec.ID) to obtain the exit code and return an error if it is non-zero; likewise in getContainerLogs demultiplex the returned stream from s.dockerClient.ContainerLogs using stdcopy.StdCopy into a strings.Builder (or separate buffers) rather than io.Copy so framing bytes are removed. Ensure you reference execCommandInContainer, getContainerLogs, ExecAttach, ContainerLogs, ExecStart, ExecInspect and stdcopy.StdCopy when making the changes.execution/evm/test/execution_test.go (2)
59-60: 🛠️ Refactor suggestion | 🟠 MajorTest determinism: remove
time.Now()from genesis + block timestamps.
genesisTime := time.Now().UTC().Truncate(time.Second)andbaseTimestamp := time.Now()make the chain setup dependent on wall-clock time. Per repo guidance for*_test.go, tests should be deterministic. While build/sync comparisons happen within the same run, this still risks cross-run variability and harder-to-reproduce flakes.Use fixed timestamps (e.g., a constant
time.Date(...)) and derive all block timestamps from it.Proposed fix
- genesisTime := time.Now().UTC().Truncate(time.Second) + genesisTime := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) - baseTimestamp := time.Now() + baseTimestamp := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) blockTimestamp := baseTimestamp.Add(time.Duration(blockHeight-initialHeight) * time.Second)Also applies to: 97-98, 128-129
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@execution/evm/test/execution_test.go` around lines 59 - 60, Replace non-deterministic calls to time.Now() used for test timestamps (e.g., genesisTime and baseTimestamp in execution_test.go) with a fixed, hard-coded time (for example using time.Date) and derive any subsequent block timestamps by adding durations to that fixed base; update all occurrences (including the variables genesisTime, baseTimestamp and any places that compute block timestamps near lines referenced) so the test uses the single deterministic seed timestamp instead of time.Now().
108-114:⚠️ Potential issue | 🟠 MajorResource + timeout hygiene in tx submission loop.
In the per-block loop:
defer ethClient.Close()is inside the loop, so connections are not closed until the test ends (potentially accumulating resources).ethClient.SendTransaction(context.Background(), ...)ignores thectxwith the 300s timeout you already created, so calls may hang beyond the test deadline.Use
ctx(fromcontext.WithTimeout) and close the client immediately at the end of each iteration (no defer).Proposed fix
for blockHeight := initialHeight; blockHeight <= 10; blockHeight++ { ... ethClient := createEthClient(t, ethURL) - defer ethClient.Close() + // Close immediately after using it for this blockHeight. for i := range txs { - txs[i] = evm.GetRandomTransaction(TEST_PRIVATE_KEY, ... , &lastNonce) - require.NoError(tt, ethClient.SendTransaction(context.Background(), txs[i])) + txs[i] = evm.GetRandomTransaction(TEST_PRIVATE_KEY, ... , &lastNonce) + require.NoError(tt, ethClient.SendTransaction(ctx, txs[i])) } + require.NoError(tt, ethClient.Close()) ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@execution/evm/test/execution_test.go` around lines 108 - 114, The loop creates ethClient via createEthClient(t, ethURL) but defers ethClient.Close() inside the loop and calls ethClient.SendTransaction with context.Background(), causing resource leakage and ignoring the test timeout; change to call ethClient.Close() explicitly at the end of each iteration (no defer) and replace context.Background() with the existing ctx (from context.WithTimeout) when calling ethClient.SendTransaction so the send honors the test timeout and the client is closed immediately after each transaction attempt (ensure Close() runs even on send error).
🧹 Nitpick comments (4)
test/e2e/benchmark/suite_test.go (1)
77-78: Pin the Spamoor and VictoriaTraces images in the suite.This setup relies on helper defaults, and both helpers currently fall back to
latest. That makes the benchmark suite non-reproducible across runs on the same commit and can skew perf/trace baselines. Please pass explicit tags or digests from the suite instead of inheriting floating defaults.Based on learnings "Applies to **/*_test.go : Ensure tests are deterministic".
Also applies to: 131-188
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/benchmark/suite_test.go` around lines 77 - 78, Pin the container images used by the benchmark suite instead of relying on helper defaults: update the victoriaTracesConfig passed to newVictoriaTracesNode (and the equivalent spamoorConfig/newSpamoorNode used later) to include explicit image references (tag or digest) coming from the suite-level variables/constants, and use those pinned values when constructing the nodes so neither VictoriaTraces nor Spamoor falls back to "latest" floating tags.test/e2e/benchmark/victoriatraces_node.go (1)
73-95: Wait for VictoriaTraces to accept HTTP before returning fromStart.This marks the node as started as soon as Docker has mapped a port, but
suite_test.gowires the sequencer to the OTLP endpoint immediately afterwards. Without a readiness probe here, the first trace exports can race the VictoriaTraces process startup and disappear intermittently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/benchmark/victoriatraces_node.go` around lines 73 - 95, Start currently marks the node as started once Docker maps a port but returns before VictoriaTraces is accepting HTTP; change Start (in victoriatraces_node.Start) to perform an HTTP readiness probe against the mapped external HTTP port (use the value assigned to n.externalHTTPPort after extractPort(hostPorts[0])) after calling n.ContainerLifecycle.StartContainer and before setting n.started = true, retrying with a short sleep until a successful 200 response or a configurable timeout, and return an error if the probe never succeeds; keep all existing container creation and port-extraction logic (createContainer, ContainerLifecycle.GetHostPorts, extractPort) but only mark started after the probe succeeds.execution/evm/test/execution_test.go (2)
61-61: Minor: renameGenesisStateRootto avoid exported-like naming and clarify type.
GenesisStateRoot := genesisStateRoot[:]creates a slice from acommon.Hash, but theGenesisStateRootname is inconsistent with the local lowercase pattern and looks exported. Also it’s not obvious whether downstream code expects[]bytevscommon.Hash.Rename to something like
genesisStateRootBytesand/or derive explicitly (e.g.,genesisStateRootBytes := genesisStateRoot.Bytes()if applicable) to make intent clear.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@execution/evm/test/execution_test.go` at line 61, Rename the local variable `GenesisStateRoot` to a lowercase, clearer name like `genesisStateRootBytes` and ensure it is the correct type (byte slice) by deriving it explicitly from `genesisStateRoot` (e.g., use `genesisStateRoot.Bytes()` if `genesisStateRoot` is a `common.Hash`, or `[:]` only if you need a slice of the underlying array) so downstream uses in this test (the assignment currently written as `GenesisStateRoot := genesisStateRoot[:]`) clearly reflect a `[]byte` and follow local naming conventions.
252-268: Reliability: fail fast instead of returning sentinel values on RPC errors.
checkLatestBlocklogs warnings and returns(0, common.Hash{}, 0)whenHeaderByNumberorBlockByNumberfails. That can lead to confusing downstream failures (or, worse, misleading comparisons) and hides the real root cause.Prefer:
- return
(…, …, …, err)and require.NoError at call sites, or- call
t.Fatalf(...)/require.NoError(t, err)inside the helper.Given this is an assertion helper used for state correctness, silent fallback is risky.
Proposed direction
header, err := ethClient.HeaderByNumber(ctx, nil) if err != nil { - t.Logf("Warning: Failed to get latest block header: %v", err) - return 0, common.Hash{}, 0 + t.Fatalf("failed to get latest block header: %v", err) } ... block, err := ethClient.BlockByNumber(ctx, header.Number) if err != nil { - t.Logf("Warning: Failed to get full block: %v", err) - return blockNumber, blockHash, 0 + t.Fatalf("failed to get full block: %v", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@execution/evm/test/execution_test.go` around lines 252 - 268, The helper checkLatestBlock currently swallows RPC errors and returns sentinel values; change it to fail fast by either (A) updating checkLatestBlock to return (uint64, common.Hash, int, error) and propagate the error to callers so tests call require.NoError(t, err) / t.Fatal on failure, or (B) have checkLatestBlock itself assert the RPC success using require.NoError(t, err) or t.Fatalf when HeaderByNumber or BlockByNumber returns an error; locate the helper function named checkLatestBlock and its call sites in tests and apply one of these two fixes so RPC errors are not converted to (0, common.Hash{}, 0).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@execution/evm/test/execution_test.go`:
- Around line 22-28: The test currently embeds a raw private key constant
TEST_PRIVATE_KEY; remove that constant and instead obtain the test key at
runtime—either read EVM_TEST_PRIVATE_KEY from the environment (fail the test
with require.NotEmpty(t, privKey, "missing EVM_TEST_PRIVATE_KEY") if absent) or
derive a deterministic key from a fixed seed at test startup so CI is
reproducible; then pass the resulting privKey into the existing
evm.GetRandomTransaction(...) call (and any other places referencing
TEST_PRIVATE_KEY) so no raw private key is committed in execution_test.go.
In `@test/e2e/benchmark/spamoor_node.go`:
- Around line 241-245: The benchmark is passing the raw private key string into
the container command args (see the Cmd construction using n.cfg.PrivateKey and
the WithPrivateKey flow), which leaks secrets; instead write or mount the
private key as a file and pass the file path (or use a container environment
variable mapped from a mounted secret) rather than embedding the key in Cmd.
Update the code that builds cmd (the slice containing "--privkey",
n.cfg.PrivateKey) to reference a path (e.g., n.HomeDir()/spamoor.key) and ensure
the key is written with restrictive permissions or mounted into the container as
a secret, and adjust any consumer logic that expects "--privkey" to read the key
from the file or from a secure env var so the secret no longer appears in the
container command line; keep references to WithPrivateKey and
defaultSpamoorHTTPPort intact.
---
Outside diff comments:
In `@execution/evm/test/execution_test.go`:
- Around line 59-60: Replace non-deterministic calls to time.Now() used for test
timestamps (e.g., genesisTime and baseTimestamp in execution_test.go) with a
fixed, hard-coded time (for example using time.Date) and derive any subsequent
block timestamps by adding durations to that fixed base; update all occurrences
(including the variables genesisTime, baseTimestamp and any places that compute
block timestamps near lines referenced) so the test uses the single
deterministic seed timestamp instead of time.Now().
- Around line 108-114: The loop creates ethClient via createEthClient(t, ethURL)
but defers ethClient.Close() inside the loop and calls ethClient.SendTransaction
with context.Background(), causing resource leakage and ignoring the test
timeout; change to call ethClient.Close() explicitly at the end of each
iteration (no defer) and replace context.Background() with the existing ctx
(from context.WithTimeout) when calling ethClient.SendTransaction so the send
honors the test timeout and the client is closed immediately after each
transaction attempt (ensure Close() runs even on send error).
In `@test/docker-e2e/resiliency_test.go`:
- Around line 890-947: The exec and log readers are returning Docker's
multiplexed stream frames and the exec path never checks the command exit code,
so failures get masked; in execCommandInContainer use
github.com/moby/moby/api/pkg/stdcopy.StdCopy to demultiplex resp.Reader into
separate stdout/stderr buffers (instead of io.ReadAll) and after ExecStart call
s.dockerClient.ExecInspect(ctx, exec.ID) to obtain the exit code and return an
error if it is non-zero; likewise in getContainerLogs demultiplex the returned
stream from s.dockerClient.ContainerLogs using stdcopy.StdCopy into a
strings.Builder (or separate buffers) rather than io.Copy so framing bytes are
removed. Ensure you reference execCommandInContainer, getContainerLogs,
ExecAttach, ContainerLogs, ExecStart, ExecInspect and stdcopy.StdCopy when
making the changes.
---
Nitpick comments:
In `@execution/evm/test/execution_test.go`:
- Line 61: Rename the local variable `GenesisStateRoot` to a lowercase, clearer
name like `genesisStateRootBytes` and ensure it is the correct type (byte slice)
by deriving it explicitly from `genesisStateRoot` (e.g., use
`genesisStateRoot.Bytes()` if `genesisStateRoot` is a `common.Hash`, or `[:]`
only if you need a slice of the underlying array) so downstream uses in this
test (the assignment currently written as `GenesisStateRoot :=
genesisStateRoot[:]`) clearly reflect a `[]byte` and follow local naming
conventions.
- Around line 252-268: The helper checkLatestBlock currently swallows RPC errors
and returns sentinel values; change it to fail fast by either (A) updating
checkLatestBlock to return (uint64, common.Hash, int, error) and propagate the
error to callers so tests call require.NoError(t, err) / t.Fatal on failure, or
(B) have checkLatestBlock itself assert the RPC success using require.NoError(t,
err) or t.Fatalf when HeaderByNumber or BlockByNumber returns an error; locate
the helper function named checkLatestBlock and its call sites in tests and apply
one of these two fixes so RPC errors are not converted to (0, common.Hash{}, 0).
In `@test/e2e/benchmark/suite_test.go`:
- Around line 77-78: Pin the container images used by the benchmark suite
instead of relying on helper defaults: update the victoriaTracesConfig passed to
newVictoriaTracesNode (and the equivalent spamoorConfig/newSpamoorNode used
later) to include explicit image references (tag or digest) coming from the
suite-level variables/constants, and use those pinned values when constructing
the nodes so neither VictoriaTraces nor Spamoor falls back to "latest" floating
tags.
In `@test/e2e/benchmark/victoriatraces_node.go`:
- Around line 73-95: Start currently marks the node as started once Docker maps
a port but returns before VictoriaTraces is accepting HTTP; change Start (in
victoriatraces_node.Start) to perform an HTTP readiness probe against the mapped
external HTTP port (use the value assigned to n.externalHTTPPort after
extractPort(hostPorts[0])) after calling n.ContainerLifecycle.StartContainer and
before setting n.started = true, retrying with a short sleep until a successful
200 response or a configurable timeout, and return an error if the probe never
succeeds; keep all existing container creation and port-extraction logic
(createContainer, ContainerLifecycle.GetHostPorts, extractPort) but only mark
started after the probe succeeds.
🪄 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: c1a88668-40b3-48ae-ba79-078e653fcc29
⛔ Files ignored due to path filters (3)
execution/evm/test/go.sumis excluded by!**/*.sumtest/docker-e2e/go.sumis excluded by!**/*.sumtest/e2e/go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
execution/evm/test/execution_test.goexecution/evm/test/go.modexecution/evm/test/test_helpers.gotest/docker-e2e/go.modtest/docker-e2e/resiliency_test.gotest/e2e/benchmark/spamoor_node.gotest/e2e/benchmark/suite_test.gotest/e2e/benchmark/victoriatraces_node.gotest/e2e/go.mod
| const ( | ||
| CHAIN_ID = "1234" | ||
| GENESIS_HASH = "0x2b8bbb1ea1e04f9c9809b4b278a8687806edc061a356c7dbc491930d8e922503" | ||
| GENESIS_STATEROOT = "0x05e9954443da80d86f2104e56ffdfd98fe21988730684360104865b3dc8191b4" | ||
| GENESIS_HASH = "0x6699a4ef6e7b76499c6cd68443455a3584e505b1da43cf82e6efaef41f5e1d8a" | ||
| GENESIS_STATEROOT = "0x0373244015ce5fa583ed6e575b7a22c39a542233a62d2bd47c37fd1a2b035366" | ||
| TEST_PRIVATE_KEY = "cece4f25ac74deb1468965160c7185e07dff413f23fcadb611b05ca37ab0a52e" | ||
| TEST_TO_ADDRESS = "0x944fDcD1c868E3cC566C78023CcB38A32cDA836E" | ||
| ) |
There was a problem hiding this comment.
Security: avoid committing a raw private key constant.
TEST_PRIVATE_KEY is hardcoded in the test file. Even if intended for local/E2E usage, this is still a sensitive artifact and can be reused accidentally or leaked via logs/exports.
Consider:
- Read the key from an env var (and fail if missing), or
- Generate a deterministic test key at runtime from a fixed seed (so CI stays reproducible) without storing the raw private key.
Proposed direction
const (
CHAIN_ID = "1234"
GENESIS_HASH = "0x6699a4ef..."
GENESIS_STATEROOT = "0x037324..."
- TEST_PRIVATE_KEY = "cece4f25ac74deb1468965160c7185e07dff413f23fcadb611b05ca37ab0a52e"
+ // Load from env to avoid committing raw private key material.
+ // Fallback: generate deterministically from a fixed seed if you prefer
+ // (but avoid storing the raw key in the repo).
TEST_TO_ADDRESS = "0x944fDcD1c868E3cC566C78023CcB38A32cDA836E"
)Then:
privKey := os.Getenv("EVM_TEST_PRIVATE_KEY")require.NotEmpty(t, privKey, "missing EVM_TEST_PRIVATE_KEY")- use
privKeyinevm.GetRandomTransaction(...).
🧰 Tools
🪛 Betterleaks (1.1.2)
[high] 26-26: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@execution/evm/test/execution_test.go` around lines 22 - 28, The test
currently embeds a raw private key constant TEST_PRIVATE_KEY; remove that
constant and instead obtain the test key at runtime—either read
EVM_TEST_PRIVATE_KEY from the environment (fail the test with
require.NotEmpty(t, privKey, "missing EVM_TEST_PRIVATE_KEY") if absent) or
derive a deterministic key from a fixed seed at test startup so CI is
reproducible; then pass the resulting privKey into the existing
evm.GetRandomTransaction(...) call (and any other places referencing
TEST_PRIVATE_KEY) so no raw private key is committed in execution_test.go.
| cmd := []string{ | ||
| "--privkey", n.cfg.PrivateKey, | ||
| "--port", defaultSpamoorHTTPPort, | ||
| "--db", fmt.Sprintf("%s/%s", n.HomeDir(), "spamoor.db"), | ||
| } |
There was a problem hiding this comment.
Don't pass the benchmark private key via Cmd.
WithPrivateKey feeds the raw key into --privkey here, so the secret becomes part of the container command line. In external mode that value comes from BENCH_PRIVATE_KEY, which makes routine debugging/inspection of the benchmark container an easy leak path. Please switch this to a mounted secret/file or another channel that does not serialize the key into container args.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/benchmark/spamoor_node.go` around lines 241 - 245, The benchmark is
passing the raw private key string into the container command args (see the Cmd
construction using n.cfg.PrivateKey and the WithPrivateKey flow), which leaks
secrets; instead write or mount the private key as a file and pass the file path
(or use a container environment variable mapped from a mounted secret) rather
than embedding the key in Cmd. Update the code that builds cmd (the slice
containing "--privkey", n.cfg.PrivateKey) to reference a path (e.g.,
n.HomeDir()/spamoor.key) and ensure the key is written with restrictive
permissions or mounted into the container as a secret, and adjust any consumer
logic that expects "--privkey" to read the key from the file or from a secure
env var so the secret no longer appears in the container command line; keep
references to WithPrivateKey and defaultSpamoorHTTPPort intact.
chatton
left a comment
chatton
left a comment
There was a problem hiding this comment.
it looks like this is pulling in a bunch of stuff that was in tastora? The spamoor / traces stuff. I think it's because we were on a commit and not a tag for benchmarking, I think we can merge this for now but I will look into making sure all the stuff we use for benchmarking is in a proper tag in tastora.
2 participants
Overview
Summary by CodeRabbit
Release Notes
Tests
Chores