Skip to content

fix(l2): pin the TDX qpl tool's crate resolution to the repo's lockfile - #7219

Merged
ilitteri merged 1 commit into
mainfrom
fix/pin-tdx-qpl-crate-resolution
Aug 26, 2026
Merged

fix(l2): pin the TDX qpl tool's crate resolution to the repo's lockfile#7219
ilitteri merged 1 commit into
mainfrom
fix/pin-tdx-qpl-crate-resolution

Conversation

@ilitteri

Copy link
Copy Markdown
Collaborator

Motivation

Every Integration Test - TDX run after 2026-08-25 15:29 UTC fails during "Start L1 & Deploy contracts", killing 56 E0046 errors deep in a third-party crate — including on the v25.0.0 merge-back (#7215). Runs that look green after that time merely skipped the job through the paths filter. The TDX prover itself is never reached.

alloy-sol-types 1.7.0 (published at that timestamp) added required trait methods (abi_decode_returns_with_config and friends) that the generated bindings in the pinned automata-dcap-attestation rev do not implement, so compiling automata-dcap-evm-bindings fails for anyone resolving fresh.

Resolution floats because of our own recipe: the automata-dcap-qpl target deletes the cloned repo's workspace Cargo.toml to build the collateral CLI standalone, which orphans the repo's root Cargo.lock — every git rev in the Makefile is pinned (#7102), but the tool's crates.io dependencies re-resolved at latest on every build.

Description

Copy the repo's root Cargo.lock into the tool's new workspace before building. Cargo prunes the entries for the removed members and keeps the locked versions for everything else, so the build compiles the dependency set the pinned rev was developed against (alloy-sol-types 1.6.1).

Verified both directions:

  • without the lock: fresh resolution picks alloy-sol-types 1.7.1 → the bindings fail to compile with the same E0046 errors as CI
  • with the carried lock: resolves 1.6.1, and cargo check of the tool (including automata-dcap-evm-bindings) passes

Checklist

  • Updated STORE_SCHEMA_VERSION (crates/storage/lib.rs) if the PR includes breaking changes to the Store requiring a re-sync.

The `automata-dcap-qpl` target deletes the cloned repo's workspace manifest to
build the tool standalone, which orphans the repo's root Cargo.lock — so while
every git rev in this Makefile is pinned, the tool's crates.io dependencies
resolved fresh at the latest versions on every build.

alloy-sol-types 1.7.0 (published 2026-08-25) is what turned that into a
failure: it added required trait methods (`abi_decode_returns_with_config` and
friends) that the generated bindings in the pinned automata-dcap-attestation
rev do not implement, so `deploy-all` dies with 56 E0046 errors while
compiling `automata-dcap-evm-bindings`. Every TDX integration run after that
release fails identically; runs that appear green merely skipped the job
through the paths filter.

Copy the repo's root Cargo.lock into the tool's new workspace before building.
Cargo prunes the entries for the removed members and keeps the locked versions
for everything else, so the build compiles the same dependency set the pinned
rev was developed against (alloy-sol-types 1.6.1). Verified both ways: a fresh
resolve picks 1.7.1 and fails to compile, while with the carried lock the tool
resolves 1.6.1 and `cargo check` passes.
@ilitteri
ilitteri requested a review from a team as a code owner August 26, 2026 16:21
@github-actions

Copy link
Copy Markdown

⚠️ Known Issues — intentionally skipped tests

Source: docs/known_issues.md

rpc-compat log-bearing cases excluded

Where: KNOWN_EXCLUDED_TESTS in .github/scripts/check-hive-results.sh counts out
eight hive rpc-compat cases — the four eth_getLogs cases, eth_getBlockReceipts/get-block-receipts-latest,
and three eth_getTransactionReceipt cases. They are exactly the cases whose recorded
response contains at least one log object; every case with an empty log array still runs.
Note this leaves eth_getLogs with no rpc-compat coverage at all, since all four of its
cases are in the set.

Why: ethrex populates blockTimestamp on log objects, as geth, besu, nethermind, reth
and erigon all do. hive's rpc-compat compares responses byte-exactly (jsondiff.FullMatch;
the lenient checkJSONStructure path applies only to cases upstream marks speconly), and
the corpus is pinned to execution-apis d08382ae (2025-02-10), whose recordings predate the
field — it entered the schema in execution-apis#639 and the fixtures in #846 (2026-07-22).
So the extra key cannot match, and this is a property of the pin rather than of the response.

The pin cannot move, and this is not temporary. The pin sits one commit before
execution-apis#627, which moved the test chain to a pre-merge genesis: the current corpus has
~36 proof-of-work blocks before its terminal total difficulty. ethrex does not support
pre-merge chains and will not, so importing that chain.rlp fails at block 1 —
validate_block_header has no pre-London base-fee path. Every revision carrying
blockTimestamp in its fixtures also carries that chain, so there is no revision that
satisfies both. Nor can the corpus be patched locally: rpc-compat's Dockerfile clones
ethereum/execution-apis by hard-coded URL, so the branch buildarg cannot point at a fork.

Coverage: the field itself is pinned by
block_timestamp_is_on_the_log_and_not_on_the_receipt in
crates/networking/rpc/types/receipt.rs, which asserts it is present on each log and absent
from the receipt level.

Removal: delete the entries if ethrex ever gains pre-merge chain import, or if upstream
marks these cases speconly so they are type-checked instead of compared byte-for-byte.


The stateless schema id does not identify the encoding

Where: STATELESS_INPUT_SCHEMA_ID in crates/common/types/stateless_ssz.rs.

Upstream keeps the stateless input schema id at 0x1501
(fork_index 0x15 << 8 | revision 0x01) across incompatible body changes. Three
encodings have now shipped under it: tests-zkevm@v0.6.2, then #3248 + #3278,
then #3356, which moved state, codes and public_keys from SszList to
ProgressiveList. ethrex speaks the last one.

The consequence is that the 2-byte prefix cannot be used to detect a stale or
mismatched bundle. A wrong-dialect input is accepted by the id check and then
fails later — in SSZ decode, or on a root that does not match — rather than being
rejected up front for what it is. only_amsterdam_schema_id_decodes therefore
proves less than its name suggests.

Worth raising upstream: a revision field that does not move across a body change
provides no version negotiation at all.


ZisK guest program hash changes with the unsync_cell gate

Where: crates/common/types/block.rs, transaction.rs.

The gate on the single-threaded unsync_cell::OnceCell moved from
all(feature = "eip-8025", target_arch = "riscv64") to
all(feature = "zisk", target_arch = "riscv64") when the eip-8025 feature was removed.

The guest ELFs were previously built --features "<zkvm>-build-elf,ci", which never enabled
eip-8025, so they compiled the atomic once_cell variant. bin/zisk/Cargo.toml does enable
ethrex-common/zisk, so the ZisK guest now compiles the unsafe impl Sync cell instead.
That changes the ELF bytes and therefore the program hash and verification key.

This is intended (the guest is single-threaded, so the unsync cell is sound and cheaper), but it
is a VK change rather than a no-op refactor, and the diffstat presents it as a file rename
(eip8025_cell.rsunsync_cell.rs). Anyone pinning a ZisK VK across this change must
re-register it. The stateless-validator crate now forwards ethrex-common/zisk from its own
zisk feature so the two ZisK guests do not disagree on the cell type.


Release signing key is an unprotected repository secret

Where: .github/workflows/tag_release.yaml.

MINISIGN_SECRET_KEY is a plain repository secret. There is no environment: on
finalize-release or dry-run-release-assets, and gh api repos/lambdaclass/ethrex/rulesets
shows only branch-targeted rulesets, so the github.ref_type == 'tag' condition is a workflow
check rather than an enforced boundary: anyone who can push a tag can reach the signing key.

This is a repository-settings change, not a code change, so it is recorded here rather than
fixed in the tree. Recommended:

  1. Move MINISIGN_SECRET_KEY / MINISIGN_PASSWORD into a GitHub Environment with required
    reviewers, and add environment: to the two jobs that sign.
  2. Add a ruleset targeting refs/tags/v* restricting who may create release tags.

Until then, the compromise of that key is silent and durable: signatures would still verify
against the committed .github/minisign.pub.

@github-actions

Copy link
Copy Markdown

🤖 Kimi Code Review

This is a correct and necessary fix for build reproducibility. The detailed inline comment accurately explains the issue: removing the workspace manifest orphans the Cargo.lock, causing Cargo to re-resolve to latest crate versions and potentially breaking the build (as happened with alloy-sol-types 1.7.0).

Feedback:

crates/l2/tee/contracts/Makefile

  1. Line 127-128: Consider adding a defensive check to ensure the lock file exists before copying, though this is minor given the controlled git checkout context:

    test -f automata-dcap-qpl/Cargo.lock && \
    cp automata-dcap-qpl/Cargo.lock automata-dcap-qpl/automata-dcap-qpl-tool/Cargo.lock
  2. Security consideration: Preserving the lock file is critical for TEE attestation verification. Floating dependency versions could introduce compromised cryptographic libraries (e.g., alloy-sol-types handles ABI encoding for attestation contracts). This fix properly pins the supply chain.

  3. Idiomatic note: The approach of transplanting the lock file into the newly-declared workspace root is the correct way to maintain reproducibility when restructuring workspaces mid-build.

Verdict: LGTM. The fix prevents deterministic build failures and supply chain drift in security-critical TEE verification code.


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

@github-actions

Copy link
Copy Markdown

🤖 Codex Code Review

No findings.

crates/l2/tee/contracts/Makefile:124-133 looks correct to me. Copying the upstream Cargo.lock into the new automata-dcap-qpl-tool workspace is a reasonable way to keep the crates.io resolution pinned after removing the root workspace manifest, and it only affects an auxiliary build tool rather than deployed EVM bytecode or consensus-critical execution paths.

Residual gap: I could not run make automata-dcap-qpl here because the target requires network access to clone upstream repos, so I did not verify the pinned DCAP_QPL_REV still contains the expected root Cargo.lock or that the copied lock resolves cleanly with the current PCCS_READER_REV.


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

@github-actions

Copy link
Copy Markdown

🤖 Claude Code Review

Summary

This is a small, well-scoped Makefile fix: it copies the automata-dcap-qpl repo's own committed Cargo.lock into the automata-dcap-qpl-tool directory before that directory is turned into its own workspace, so cargo build --release there resumes from a resolved dependency set instead of re-resolving crates.io deps at latest. No Rust/EVM/consensus logic is touched, so the Ethereum-specific review criteria (gas accounting, EIP compliance, trie/RLP, tx/block validation) don't apply here — this is purely a build-reproducibility/CI fix.

The root-cause diagnosis (deleting the workspace Cargo.toml orphans the lock, so alloy-sol-types floats past a version the pinned attestation-binding rev can't compile against) is correct and consistent with the file's existing convention of pinning everything (git revs) to keep builds reproducible. The fix itself is minimal and in the right place.

Findings

  1. (Info) Confirm the mid-recipe comment doesn't get parsed oddly by Make. The new explanatory comment (lines 124–132) has no leading tab, while every actual recipe line in this target does (verified via cat -A). GNU Make treats bare # lines inside a recipe as makefile-level comments and does not terminate the recipe because of them, so rmcpecho/echocd && cargo build should still run as one recipe. This matches precedent elsewhere in the file (e.g. lines 29–34, 59–67), but since a parsing regression here would be a hard, loud Make error (not silent), it's low-risk — still worth a plain make automata-dcap-qpl smoke run (not just the manual cargo check mentioned in the description) to be certain the whole recipe fires as a unit.

  2. (Info) Implicit dependency on upstream committing Cargo.lock. The fix assumes automata-network/automata-dcap-qpl has a Cargo.lock checked in at DCAP_QPL_REV. That's a reasonable assumption for a binary/tool repo, and if it's ever untrue the cp fails loudly rather than silently, so the failure mode is safe — just flagging that a future DCAP_QPL_REV bump could reintroduce floating resolution if upstream ever stops committing the lock, with no explicit check/message calling that out.

  3. (Info) Coverage of pccs-reader-rs's transitive deps. pccs-reader-rs is cloned separately into automata-dcap-qpl-tool/pccs-reader-rs (line 121) and isn't part of upstream's own repo history. It's worth double-checking whether the carried Cargo.lock actually pins pccs-reader-rs's own transitive dependencies, or whether those remain subject to fresh (floating) resolution alongside the now-pinned main graph. Doesn't undermine the specific alloy-sol-types regression this PR fixes (per the description's verification), but is a gap in the "everything is now pinned" guarantee if true.

  4. (Info, no action needed) Pinning tradeoff. Freezing automata-dcap-qpl-tool's deps to whatever upstream had locked at DCAP_QPL_REV means future transitive security fixes (e.g., in alloy-sol-types) won't be picked up until someone bumps DCAP_QPL_REV. This is the same tradeoff already accepted for every other pinned revision in this file, so it's consistent with existing project conventions rather than a new risk — just noting it for whoever eventually rev-bumps this dependency.

Overall: solid, targeted fix that matches the file's existing pinning discipline and is backed by stated before/after verification. Nothing here blocks merging; the notes above are minor confirmations/awareness items.


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

@ilitteri
ilitteri added this pull request to the merge queue Aug 26, 2026
Merged via the queue into main with commit 5962a33 Aug 26, 2026
56 checks passed
@ilitteri
ilitteri deleted the fix/pin-tdx-qpl-crate-resolution branch August 26, 2026 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

L2 Rollup client

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants