Repository navigation
Conversation
Co-authored-by: Toshi <toshi-noreply@coinbase.com>
🟡 Heimdall Review Status
|
Co-authored-by: Toshi <toshi-noreply@coinbase.com>
Review round 1Reviewers: anthropic/claude-opus-5-5, cbhq-openai/gpt-6.1-sol
🤖 Reply generated with Toshi |
Co-authored-by: Toshi <toshi-noreply@coinbase.com>
Review round 2Reviewers: anthropic/claude-opus-5-5, cbhq-openai/gpt-6.1-sol
🤖 Reply generated with Toshi |
Co-authored-by: Toshi <toshi-noreply@coinbase.com>
Review round 3Reviewers: anthropic/claude-opus-5-5, cbhq-openai/gpt-6.1-sol
🤖 Reply generated with Toshi |
Co-authored-by: Toshi <toshi-noreply@coinbase.com>
Review round 4Reviewers: anthropic/claude-opus-5-5, cbhq-openai/gpt-6.1-sol
🤖 Reply generated with Toshi |
| raise ValueError("invalid network identifier") | ||
| project = task if (task / "foundry.toml").is_file() else repo / "active/evm" | ||
| config = task / "config" / network | ||
| paths = {task / "Makefile", config / ".env", repo / "config" / f"{network}.env"} |
There was a problem hiding this comment.
P3: the offline fingerprint does not cover the shared make includes, so the "every input that shapes the transaction" claim is broader than the code.
Issue: fingerprint() hashes the task Makefile, config/<network>/ files, script/, the Forge entry point and its imports, and config/<network>.env. Every task Makefile also does include ../../Makefile and include ../../Multisig.mk (standby tasks use scripts/Standby.mk, which sets RPC_URL, PROJECT_DIR and NETWORK_ENV), and the root Makefile pins SIGNER_TOOL_COMMIT. None of these are in the path set. On a stamped copy, appending a line to Multisig.mk, to the root Makefile or to scripts/Standby.mk still printed "Validation input fingerprints match."
Impact: a change to a shared include that alters what is generated (for example RPC_URL, PROJECT_DIR, the signer tool commit) keeps public CI green. check-validations would catch it, but it needs RPC access and is not the check that runs on every PR. The PR body's claim and README line 208 read as complete coverage.
Requested change: either add Multisig.mk, the root Makefile and (for standby tasks) scripts/Standby.mk to the path set, accepting that editing them flags every pending network, or add one README sentence that shared includes and files outside config/<network>/ and script/ are covered only by check-validations, and correct the PR-body wording.
There was a problem hiding this comment.
Fixed in 2127788 with the docs option. Hashing the root Makefile, Multisig.mk and scripts/Standby.mk would flag every pending invocation in the repo on each shared-tooling edit (this PR itself edits Multisig.mk), and check-validations already catches drift from them. The README now states that shared Make includes, the signer tool pin and foundry.toml are outside the fingerprint and covered only by check-validations, and lists changes to them as a reason to run it. The PR description now says "task-owned inputs" and has a coverage-boundary note.
🤖 Reply generated with Toshi
…edback) Co-authored-by: Toshi <toshi-noreply@coinbase.com>
0xth4nh
left a comment
0xth4nh
left a comment
There was a problem hiding this comment.
Looks Good to Thanh's Toshi
…dation-file-drift-check Co-authored-by: Toshi <toshi-noreply@coinbase.com> # Conflicts: # scripts/tests/test_standby_lifecycle.py
mw2000
left a comment
mw2000
left a comment
There was a problem hiding this comment.
Reviewed the drift checker and Make integration — nice test coverage overall. Left a few comments on cases that came up with the recent task-scaffolder changes (#821) and open task PRs (#824, #825).
Also worth noting for merge ordering: open PRs #824 and #825 currently have unstamped READY TO SIGN validation files on Sepolia (and #823 updates config/sepolia.env), so they'll need a re-stamp/regeneration once this lands.
One broader design thought: most of the complexity in validation-drift.py is the Solidity/TOML/remapping/import-graph parser (without_comments, remappings, script_entrypoint, and the import walker). Since the only repo-owned Solidity outside <task>/script/ is active/evm/script/common/, hashing git ls-files over <task>/Makefile, <task>/script/, active/evm/script/common/, <task>/config/<network>/ (minus validations/, README.md, prestate.json), and config/<network>.env would give the same drift coverage without needing to parse foundry.toml, remappings.txt, or cmd.
| def fingerprint(repo, task, network, cmd): | ||
| if not re.fullmatch(r"[a-z0-9-]+", network): | ||
| raise ValueError("invalid network identifier") | ||
| project = task if (task / "foundry.toml").is_file() else repo / "active/evm" |
There was a problem hiding this comment.
Breaks on active tasks with a task-local foundry.toml (e.g. #825)
In #825 (active/evm/tasks/2026-10-20-upgrade-remaining-l2-predeploys), the task has its own foundry.toml for custom compiler profiles during build/deploy/simulate-l2, while GEN_VALIDATION still runs with PROJECT_DIR := $(abspath ../..) (active/evm) to match signer-tool. As a result, cmd in base-signer.json contains tasks/2026-10-20-upgrade-remaining-l2-predeploys/script/UpgradeL2Predeploys.s.sol:UpgradeL2Predeploys.
Because (task / "foundry.toml").is_file() is true here, script_entrypoint(project, cmd) resolves <task>/tasks/<task>/script/... and fails with FileNotFoundError when stamping or checking.
Consider falling back to repo / "active/evm" in script_entrypoint (or when selecting project for script resolution) if the path doesn't exist under task.
There was a problem hiding this comment.
Fixed in 1fc535c. The fingerprint now resolves the forge script entry point under the task's own Forge project first and falls back to active/evm when it is not there, so a task-local foundry.toml used only for build or deploy profiles no longer breaks stamping or checking. Remappings follow the project the entry point resolved in. Added a regression test.
🤖 Reply generated with Toshi
| if len(args) < 4 or not re.fullmatch(r"[A-Za-z0-9_.-]+\.json", args[3]): | ||
| raise ValueError(f"{task / 'Makefile'}: GEN_VALIDATION needs a literal signer JSON filename as its fourth argument") | ||
| files.add(args[3]) | ||
| return files |
There was a problem hiding this comment.
Silently passes when all GEN_VALIDATION calls are commented out
When make new-task (from #821) scaffolds a task, the $(call GEN_VALIDATION,...) line in Makefile starts commented out while config/<network>/README.md is initialized to Status: READY TO SIGN. declared_files(task) returns set(), so if validations/ is also empty (present == set()), declared - present, present - declared, and declared & present are all empty and check exits 0 with zero validation files.
By contrast, check-validations in Multisig.mk errors when no gen-validation* targets exist. Should declared_files (or check for a pending network) fail when files is empty?
Relatedly, new-task.py and standby/evm/tasks/*/FACILITATOR.md from #821 could also include make TASK_NETWORK=<network> check-validations in the facilitator instructions alongside make check-task-status.
There was a problem hiding this comment.
Fixed in 1fc535c. check now fails a pending network (has .env, not EXECUTED or CANCELED) when the task Makefile declares no GEN_VALIDATION files, matching check-validations. A freshly scaffolded task stays red in CI until its gen-validation target is written, which fits its READY TO SIGN status. The new-task.py facilitator guide now lists make TASK_NETWORK=<network> check-validations after gen-validation. I left the canonical standby FACILITATOR.md files alone, since editing them triggers a re-review of those operations; the README already covers the check for standby invocations.
🤖 Reply generated with Toshi
|
|
||
| def compare(args): | ||
| committed, regenerated = read_json(args.committed), read_json(args.regenerated) | ||
| if args.network != "mainnet": |
There was a problem hiding this comment.
Mainnet tasks that don't go through ProxyAdmin owner
Per AGENTS.md, task origin validation is only needed for mainnet scripts that go through the ProxyAdmin owner. For a mainnet task owned directly by CB_MULTISIG (like 2026-10-08-denim-system-config), if a facilitator adds "skipTaskOriginValidation": true by hand to the committed JSON rather than via a Makefile recipe step, make TASK_NETWORK=mainnet check-validations will fail with $.skipTaskOriginValidation: removed true because genValidationFile.ts doesn't emit that field.
There was a problem hiding this comment.
This one is intentional, so I kept the strict mainnet comparison. Offline, the checker can't tell whether a mainnet task goes through the ProxyAdmin owner, and a hand-added opt-out on one that does would silently disable task-origin validation. The supported path is now documented in README and AGENTS.md (1fc535c): set the opt-out in the gen-validation recipe after GEN_VALIDATION, writing to $(VALIDATIONS_DIR)/<file>. check-validations points VALIDATIONS_DIR at its temp directory, so regeneration reproduces the field, and reviewers see the opt-out in the Makefile.
🤖 Reply generated with Toshi
…alidations Fall back to active/evm when a task-local foundry.toml does not contain the script entry point, fail pending networks with no GEN_VALIDATION files, document recipe-based mainnet origin opt-outs, and add check-validations to the scaffolded facilitator guide. Co-authored-by: Toshi <toshi-noreply@coinbase.com>
Approved review 5576401451 from 0xth4nh is now dismissed due to new commit. Re-request for approval.
Approved review 5577317423 from xenoliss is now dismissed due to new commit. Re-request for approval.
What changed? Why?
Signer validation files are generated by simulating a task's script against an internal RPC, then committed. When a script, task
.env, oraddresses.jsonchanges later, nothing notices that the committed validation file no longer matches what signers will be asked to sign. This adds two checks, split by whether internal RPC access is needed:Offline fingerprint check (public CI).
GEN_VALIDATIONnow stamps each validation file with ataskInputsfingerprint: the sha256 of the task-owned inputs that shape the transaction: the taskMakefile; every file under the task'sscript/; the Forge script named in the validation file'scmd(which may be a sharedscript/common/...script); repo Solidity those scripts import transitively (bare package imports such assolady/and anything underlib/are dependencies pinned byBASE_CONTRACTS_COMMIT, so they are not hashed); every file git would commit underconfig/<network>/exceptvalidations/,README.md, andprestate.json(a post-execution baseline that does not feed the signed call), so.env,addresses.json, and JSON inputs such asfunding.json; and the rootconfig/<network>.env.python3 scripts/validation-drift.py checkscansactive/evm/tasks/*andstandby/evm/tasks/*, and for every network that is notEXECUTED/CANCELEDit requires:GEN_VALIDATIONcall) to be present, with no undeclared.jsoninvalidations/(the signer UI would list it);taskInputs, and the recomputed fingerprint to match. On mismatch it names the input files that changed, appeared, or disappeared.It runs in the new
Validate validation filesworkflow on every PR and onmain, with no secrets or network access.Regenerate and diff (internal machine).
make TASK_NETWORK=<network> check-validations, run from an active task or standby operation directory, re-runs the task's owngen-validation-*targets into a temp directory and semantically compares each result with the committed file (validation-drift.py compare). Missing or extra files and any difference incmd,rpcUrl, ledger id, expected hashes, overrides, state changes, balance changes, ortaskInputsfail. Fields a facilitator edits by hand are ignored:name,description, andallowDifferenceon each entry (plus the values of entries the committed file marksallowDifference: true), andtaskOriginConfig(it records the generating machine's identity). Committed files are never touched. It needs internal*.cbhq.netRPCs, so it runs on an internal machine, before collecting signatures and after any change to scripts or.env.README and AGENTS.md document where each check runs and how to adopt the fingerprint (regenerate with the task's
gen-validation-*targets; do not hand-edit values orcmd).Notes to reviewers
Makefile,Multisig.mk,scripts/Standby.mk), the signer tool pin, oractive/evm/foundry.toml. Hashing them would flag every pending invocation on each shared-tooling change, so drift from those files is caught only bycheck-validations. The README says so.Makefile), which would otherwise flag networks that already ran. Validation files only matter while signatures are pending.mainare executed on Zeronet, so CI passes onmaintoday. Any pending network whose validation files predate this change fails the check until its files are regenerated (which stamps them). That is intentional: an unstamped file has never been checked against its inputs.TaskConfigSchemais a non-strict zod object, so the extrataskInputskey is ignored when signing. The stamp is written at generation time, before task-origin signing tarballsconfig/<network>/.GEN_VALIDATION's--forge-cmdis unchanged, so the generatedcmdis byte-identical.solady/were resolved via the gitignoredlib/directory, which public CI does not install). The decision was to fix it by treating unresolved bare imports as dependencies, add a regression test that runs with nolib/present, and run one final review round.foundry.tomlproject first and falls back toactive/evm, because some tasks keep their ownfoundry.tomlfor build or deploy profiles whileGEN_VALIDATIONstill runs Forge fromactive/evm.checkfails a pending network whose task Makefile has noGEN_VALIDATIONcall (for example a fresh scaffold), the same ascheck-validations. The scaffolded facilitator guide now listscheck-validations.skipTaskOriginValidationon mainnet failscheck-validations. A mainnet task that legitimately needs it sets it in the gen-validation recipe via$(VALIDATIONS_DIR), which the README now documents.Task.mk,config/<network>/network.env, generation targets inactive/evm/make/*.mk) have no taskMakefile, sostampandcheckskip them with a printed notice instead of failing, andcheck-validationsfails with a clear message rather than passing silently. Extending fingerprints to that layout is a follow-up once it lands. This PR adds a new workflow file instead of editingvalidate-common-scripts.yml, so the two do not conflict textually.sign-pauseflow generates pauser signatures, notGEN_VALIDATIONfiles, so it is unaffected. No overlap in the rootMakefileor the existing workflow.mainthe Sepolia validation files need a regenerate (which also adopts the fingerprint) or the offline check fails for that network. feat(sepolia): add Denim upgrade with L2 predeploys #819 has since merged, and feat(sepolia): add Denim SystemConfig gas parameters task #824 and feat(sepolia): upgrade remaining L2 predeploys to v8.4.0 #825 add unstampedREADY TO SIGNSepolia files (fix(config): update Sepolia L1 RPC default #823 editsconfig/sepolia.env). Whichever lands after this PR needs its pending validation files regenerated.How has it been tested?
python3 -B -m unittest scripts.tests.test_validation_drift -v(tests over temp fixture repos: each input change named, missing/undeclared/unstamped files, executed and canonical standby skips, stamp round trip and formatting, semantic compare cases,GEN_VALIDATIONstamping without changingcmd,check-validationsisolation for active and both standby operations including-j2).python3 -B -m unittest scripts.tests.test_standby_lifecycle -v(existing, passes).python3 scripts/validation-drift.py checkonmain's tasks: passes, both Zeronet invocations reported as skipped.make -n TASK_NETWORK=zeronet check-validationsinactive/evm/tasks/2026-10-15-upgrade-l2-predeploys.make TASK_NETWORK=sepolia check-validationson the pending Denim Sepolia invocation from an internal machine, using that branch's content with this change merged locally: passed,No meaningful differencesforbase-signer.jsonandsecurity-council-signer.json(all state overrides, state changes and expected domain/message hashes match the committed, hand-edited files). For that run the fingerprint was stamped onto local copies of the committed files, and the L1/L2 RPCs were set to the public endpoints those files were generated with (see follow-up below).Follow-up found by this check (not changed here)
Running
check-validationsagainst the Denim Sepolia invocation in its own branch with its configured RPCs flags real drift: its two Sepolia validation files were generated with public RPCs (rpcUrlhttps://ethereum-sepolia-rpc.publicnode.com, andL2_RPC_URL=https://sepolia.base.orgincmd) instead of the internal RPCs inconfig/sepolia.envand the task config, and they carry notaskInputsfingerprint. Simulated content otherwise matches. Once both changes are onmain, those files should be regenerated on an internal machine (which also stamps them), or the offline check will fail for that network. That branch is intentionally untouched here.Generated with Toshi