[Nic-dorman]

Round 5 follow-up (93bb12c)

Hermes approved 36f0ea1 with one non-blocking note: the Foundry version was unpinned. contracts.yml now pins foundry-toolchain to v1.8.7, the version the byte-for-byte check passed on in CI (dev2's 1.7.1 also produced identical bytecode). Foundry 1.9.0 shipped on 2026-11-02, so stable would already have moved. Review range: 36f0ea1..93bb12c (optional). merge.yml's Foundry (Anvil for the tests) is left on stable.

Round 4 fix (36f0ea1): compare the full bytecode, strip nothing

Hermes requested changes on 7290274, valid again: {"solc": 0.8.28} (…a164736f6c634300081c000a) names the pinned compiler and also executes (LOG1), so it was stripped. No shape or version check can prove a tail is metadata rather than code. forge's rawMetadata doesn't hash to the tail either, so the tail can't be authenticated against it. Review range: 7290274..36f0ea1.

So the stripping is gone. On dev2 (forge 1.7.1, solc 0.8.28) I found why the metadata ever differed:

Checkout	Remappings in the metadata	Built vs committed bytecode
submodules: true (non-recursive, what contracts.yml did)	4 (no erc4626-tests, halmos-cheatcodes: they come from OpenZeppelin's nested submodules)	code identical, metadata hash differs
recursive	6, same as the committed artifact	identical byte for byte, metadata included

• contracts.yml checks out submodules recursively.
• check_contract_artifacts.py compares the ABI and the full bytecode/deployedBytecode. The CBOR parser and test_check_contract_artifacts.py are removed; none of the four counterexamples can occur when no bytes are dropped. On failure, it also says to check out submodules recursively if only the metadata hash differs.
• Stricter than before: the metadata hashes every source file, so a comment-only source change now fails until the artifacts are regenerated (inferred from solc's metadata format; not re-run).
• "Artifacts match sources" runs the real build and full comparison on this push.

Superseded review rounds: R1 (e1d4c44, footer trusted as a length), R2 (2a2722d, key whitelist), R3 (7290274, version pin) each tightened the metadata parser against a Hermes counterexample. Round 4 replaces that approach.

Follow-ups from the 2026-10-27 faux-CI audit (V2-1746). evmlib's CI was found honest; these close the small gaps it listed.

1. verify_prices gets unit tests. MerklePaymentCandidatePool::verify_prices had none. Six tests in src/merkle_payments/merkle_payment.rs:
   • matching prices pass;
   • the winner is found by hash, not by position (a decoy commitment with wrong prices is listed first);
   • a mismatch at candidate 0 and at candidate 15 is rejected with the right index and prices;
   • with several mismatches, the first one is reported;
   • a winner hash missing from the commitments (or an empty list) is rejected.
2. release.yml passes secrets through env:. cargo login "${{ secrets.CRATES_IO_TOKEN }}" and --git-token ${{ secrets.GITHUB_TOKEN }} were interpolated into the script. Now CARGO_REGISTRY_TOKEN and GIT_TOKEN are step env, with no cargo login; cargo reads CARGO_REGISTRY_TOKEN when release-plz publishes. Same pattern as self_encryption #449.
3. The committed PaymentVaultV2 artifacts are checked against contracts/. The tests deploy artifacts/PaymentVaultV2.json and bind abi/IPaymentVault.json via sol!, so a source change without regenerated artifacts would leave the tests exercising the old contract.
   • foundry.toml pins solc_version = "0.8.28" and evm_version = "cancun", the values recorded in the committed artifact's metadata. Unpinned, Foundry picks the newest matching solc and its own default EVM version (prague on Foundry 1.7.1).
   • New contracts.yml (path-filtered: contracts, artifacts, abi, lib, Foundry config) runs forge build into a temp dir. .github/scripts/check_contract_artifacts.py then compares the ABI and the full bytecode/deployedBytecode exactly. The job checks out submodules recursively, because the metadata at the end of the bytecode hashes the remappings Foundry auto-detects, including those from OpenZeppelin's nested submodules (see Round 4).
   • artifacts/AutonomiNetworkToken.json is not checked: it's a Hardhat artifact whose source isn't in this repo.

Not changed: the release test gate (V2-1746 item 2) and ADR Governance (V2-1746 item 4).

Linear issue

Closes V2-1750

Risk tier

• T0 — docs / tooling / CI / pure UX-output. Repo CI only.
• T1 — client-only, no network-facing behavior change. CI + prod compat smoke.
• T2 — node/client logic with behavioral surface, no protocol/format/economics change. Dev testnet + ADR.
• T3 — protocol / storage format / payments / routing. T2 evidence + adversarial testing.

Compatibility

• Wire: none
• Storage: none
• API: none (tests, CI and Foundry config only; no library code or committed artifact changes)

Semver impact

• breaking
• feature
• fix

Test evidence

On dev2 (Linux), same commands as merge.yml and wasm.yml:

• cargo fmt --all -- --check: clean
• RUSTFLAGS="-D warnings" cargo clippy --all-targets --all-features: clean
• cargo test --release (Anvil 1.7.1): all suites pass. Lib 36 (including the 6 new), cryptography 4, gas_fee_limit 1, network_token 3, payment_vault 5, wallet 8.
• wasm.yml: both cargo check --target wasm32-unknown-unknown variants are clean; cargo test --no-default-features passes (15 + 1); cargo test --lib --test cryptography --no-default-features --features rpc,external-signer passes (28 + 4).
• cargo publish --dry-run: OK.
• The new tests catch the bugs they target. I broke verify_prices locally two ways and ran the 6 tests:
  • dropping the price comparison fails the 3 mismatch tests;
  • taking the first commitment instead of looking the winner up by hash fails the decoy test and the missing-hash test.

Artifact check, on dev2 with Foundry 1.7.1 (Round 4 design):

• recursive checkout of this branch: built bytecode and deployedBytecode equal the committed artifact byte for byte, so the check passes;
• non-recursive checkout: differs only in the metadata hash, so the check fails (the reason contracts.yml is recursive now);
• earlier, against the metadata-stripping design: a new public function failed on the ABI (both files) and the bytecode, and a > → >= logic change failed on the bytecode. The full comparison is strictly stronger, so both still fail.

actionlint is clean on all workflows.

New dependency

none (Foundry is already installed by merge.yml's test job; the script is stdlib Python)

ADR

n/a

Mitigation / rollback

Revert the PR. contracts.yml is path-filtered and not a required check, so at worst it reds PRs that touch the contracts until fixed.

🤖 Generated with Claude Code

[MaidSafe-Hermes]

REQUEST_CHANGES. Coverage: 2/2 seats reviewed (sol-1 gpt-6.1-sol xhigh; glm-1 z-ai/glm-5.3). The seats disagreed: sol-1 raised the finding below; glm-1 approved but noted the same metadata-free robustness concern. I reproduced the false-negative behavior against the checker, so this blocks approval.

Other review evidence: all PR checks, including Artifacts match sources, are green. I ran cargo test --lib verify_prices -- --nocapture: 6 passed. The release-token change and test additions look sound. No local Foundry run (forge is not installed).

[MaidSafe-Hermes]

P2: This assumes the final two bytes always encode a valid CBOR metadata length. With metadata absent (for example, if a later change sets cbor_metadata = false in foundry.toml, which triggers this workflow), an oversized footer length can strip all executable code: I reproduced code("0x600160005500") == code("0x600260005500") == "" although the bytecode differs. The drift check could then pass despite a code change. Please validate the footer length and CBOR structure; if no valid metadata tail exists, compare the full bytecode or fail explicitly. Add tests for metadata-free, truncated, and oversized-footer input.

[MaidSafe-Hermes]

REQUEST_CHANGES. Coverage: 2/2 seats reviewed (sol-1 gpt-6.1-sol xhigh; glm-1 z-ai/glm-5.3). The seats disagreed: sol-1 identified the issue below; glm-1 approved but noted the parser can strip valid-looking but non-solc CBOR tails. I reproduced a false match with an invalid solc value, so the concern is confirmed.

The prior metadata-free false match is fixed: the new 11 artifact-checker tests pass locally, and the updated Artifacts match sources check and all other reported PR checks are green.

[MaidSafe-Hermes]

P2: Whitelisting only map keys does not establish that this is solc metadata. _cbor_item also conflates CBOR text and byte-string keys and silently overwrites duplicate map keys; arbitrary value types are accepted. Reproduction: code("0x600060006000a164736f6c63010007") returns 600060006000, the same as code("0x600060006000"): the suffix is the CBOR map {"solc": 1} plus its length, not valid solc metadata. Its bytes also execute as LOG1/PUSH5/STOP, so changed executable bytecode can be stripped and compare equal. Validate CBOR text-key types, uniqueness, required metadata keys and their value types/lengths; otherwise compare the full bytecode. Add this counterexample and duplicate/byte-string-key regression tests.

[MaidSafe-Hermes]

REQUEST_CHANGES. Coverage: 2/2 seats reviewed (sol-1 gpt-6.1-sol xhigh; glm-1 z-ai/glm-5.3). The seats disagreed on severity: sol-1 treated the invalid compiler-version suffix below as a blocker; glm-1 observed the same false normalization but considered executable impact unverified for this contract. I reproduced the false match in the checker.

The prior round's recognized-key/value-shape cases are now covered; all 17 checker tests pass locally, git diff --check passes, and all current PR checks are green.

[MaidSafe-Hermes]

P2: The parser validates only the solc value's type and length, not that it matches the pinned compiler (foundry.toml sets 0.8.28). It therefore strips {"solc": h'ffffff'} as metadata, even though this compiler cannot emit version 255.255.255. Reproduction: code("0x600060006000a164736f6c6343ffffff000a") and code("0x600060006000") both return 600060006000; the suffix is valid EVM instructions (LOG1/PUSH5/SELFDESTRUCT/STOP), not metadata from the pinned build. The test gate can thus normalize different bytecode to the same value. Require the solc field to match the pinned version (00 08 1c for 0.8.28; reject prerelease text for this release build), and add a regression test that keeps this executable suffix in the full comparison.

[MaidSafe-Hermes]

REQUEST_CHANGES. Coverage: 2/2 seats reviewed (sol-1 gpt-6.1-sol xhigh; glm-1 z-ai/glm-5.3). The seats disagreed: sol-1 treated the exact-pin false match below as blocking; glm-1 considered it non-blocking because the suffix resembles metadata. I independently reproduced it and confirmed the suffix also executes as EVM instructions, so the checker can still accept a bytecode mismatch.

The new version-pin tests pass locally (21 tests), and current PR checks are green.

[MaidSafe-Hermes]

P2: Matching the pinned compiler version still does not prove these bytes are metadata rather than executable code. The accepted solc-only shape is explicitly tested as valid metadata, but the same bytes can execute. Reproduction: code("0x600060006000a164736f6c634300081c000a") and code("0x600060006000") both return 600060006000; the longer program executes LOG1 (empty data, zero topic), unlike the shorter program. This suffix names the pinned 0.8.28 compiler, so the new version check does not reject it. The checker can therefore normalize distinct bytecode as equal. Authenticate the tail against the artifact/compiler metadata (for example, verify its hash against rawMetadata and compare the relevant metadata), or fail closed when the boundary cannot be established. Add this exact-pin executable-suffix regression test.

[MaidSafe-Hermes]

APPROVE. Coverage: 2/2 seats reviewed (sol-1 gpt-6.1-sol xhigh; glm-1 z-ai/glm-5.3). The seats disagreed: glm-1 flagged solc_version as unrecognized. I checked the official Foundry configuration docs, which use solc_version = "0.8.28" in the minimal foundry.toml example, and the PR's Artifacts match sources job passes on this head; that objection is refuted. The latest change compares full bytecode and recursively checks out submodules, removing the previous metadata-stripping false-match path. All PR checks are green. The Foundry version itself remains unpinned; I consider that a future reproducibility concern, not a current blocker.