fix(precompiles): restore Zero6 registration gate for PQ - #334
fix(precompiles): restore Zero6 registration gate for PQ#334kutluhaneth46 wants to merge 3 commits into
Conversation
hardfork_flags is unused on the PQ dispatch path, so availability is not Zero6-gated at registration time. Align the address doc comment with that. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Disclosure: I'm not affiliated with Circle — an external community contributor, not a maintainer. I have no write access to this repository, so any review state I set (approval or change request) carries no merge authority and is advisory only. Please treat this as one contributor's technical assessment, and defer to Circle maintainers for the binding review.
Thanks for picking this up, but I think this resolves #293 in the wrong direction, and the git history is fairly decisive about it. The (Zero6-gated) comment isn't inaccurate — it's the last surviving correct statement of intended behavior, and the code regressed away from it.
The gate existed and was removed
create_precompiles_map had an explicit registration-time Zero6 guard in v0.7.1, v0.7.2, and v0.7.3 — identical in all three:
// v0.7.3 crates/precompiles/src/precompile_provider.rs:63
PQ_ADDRESS => {
// Only register PQ precompile if Zero6 hardfork is active
if !hardfork_flags.is_active(ArcHardfork::Zero6) {
return None;
}
Some(DynPrecompile::new_stateful(
PrecompileId::Custom("PQ".into()),
move |input| run_pq(input, hardfork_flags),
))
}In v0.8.0 that whole arm collapsed to an unconditional PQ_ADDRESS => Some(DynPrecompile::new_stateful(...)), matching the other three precompiles.
So the PR's premise — "hardfork_flags is unused on the dispatch/execution path, so the comment overstated gating" — has cause and effect backwards. _hardfork_flags is unused because the guard that used to read it was deleted. It was never meant to be consumed inside run_pq; the gating was always at registration, which is exactly where the issue's suggested option 2 puts it back.
The test diff is the part that should give reviewers pause
v0.7.3 had a matched pair:
fn test_pq_precompile_available_with_zero6() {
... ArcHardforkFlags::with(&[ArcHardfork::Zero6]) ...
assert!(precompiles.get(&PQ_ADDRESS).is_some(),
"PQ precompile should be available when Zero6 is active");
}
fn test_pq_precompile_not_available_without_zero6() {
... ArcHardforkFlags::default() ...
assert!(precompiles.get(&PQ_ADDRESS).is_none(),
"PQ precompile should NOT be available without Zero6");
}On v0.8.0/main the second test is gone, and the first was renamed to test_pq_precompile_available and had its input changed from with(&[Zero6]) to ArcHardforkFlags::default() — all hardforks inactive — while still asserting is_some().
That is the precise input the deleted test used to assert is_none(). The suite now actively pins the regression: it asserts the PQ precompile is available with Zero6 inactive. A test that locks in the wrong behavior is worse than no test, and merging this PR would remove the last in-code signal that the behavior was ever meant to be otherwise.
The PR is also incomplete on its own terms
If the gating claim were genuinely wrong, this one doc comment isn't the only place asserting it. Two others survive untouched:
crates/precompiles/src/pq.rs:77—// Activate Zero6 hardfork at block 0 (PQ precompile is gated on Zero6)crates/execution-e2e/tests/e2e/pq_precompile.rs:26-27—//! There is no case here for Zero6 disabled (PQ precompile unavailable)...
Three independent in-tree statements of the same intent, in different crates, are not three mistakes. They're the fossil record of a behavior that was removed without updating its documentation.
Impact, and why that argues against merging this
Agreed with your assessment and @pplmaverick's: no live exposure. Zero6 has been active on testnet since ARC_ZERO6_HARDFORK_TIMESTAMP_ACTIVATION_TESTNET (2026-05-27) and mainnet activates it from block 0, so there is no reachable network state where an ungated PQ_ADDRESS differs observably from a gated one.
But that argues for fixing it properly rather than papering over it. The cost of restoring the guard today is one if and one test; the cost of rediscovering this later — after the doc comment is gone and the test asserts the opposite — is considerably higher. As #293 notes, the systemic risk is the next hardfork-gated precompile added to this dispatch table following the now-ungated pattern.
Two process notes
- #293 was routed into Circle's security triage through a private channel, and @ygd58 offered essentially this fix upthread and agreed to hold pending guidance. Worth waiting for a maintainer signal before landing anything here, mainly so the fix isn't done twice in two different directions.
- Development on this repo happens internally and syncs out — release tags land as single squashed sync commits. Public PRs here are most useful as reference implementations for the team to port, so it's worth optimizing for "unambiguously correct spec" over "minimal diff".
Suggested path
Either close this in favor of the restoration, or repurpose the branch to:
- Restore the
v0.7.3guard verbatim increate_precompiles_map— it's already the spec, no design work needed. - Restore
test_pq_precompile_not_available_without_zero6. - Revert
test_pq_precompile_availableto passArcHardforkFlags::with(&[ArcHardfork::Zero6])and restore its original name. This is the piece I'd most want covered — leaving it asserting availability underdefault()would contradict the restored guard and fail CI, which is the correct outcome but should be an intentional part of the change rather than a surprise. - Keep the
(Zero6-gated)doc comment as-is.
Marking request-changes on the substance, not the effort — the tracing in your PR description is accurate as far as it goes, it just stops one release short. I have no push access here, so treat the review state as advisory. Standard caveat: no cargo or rustc in my environment, so this is source review plus git show against the published tags, not a test run.
| use slh_dsa::{signature::Verifier, Sha2_128s, Signature, VerifyingKey as SlhDsaVerifyingKey}; | ||
|
|
||
| /// PQ precompile address — SLH-DSA-SHA2-128s signature verifier (Zero6-gated). | ||
| /// PQ precompile address — SLH-DSA-SHA2-128s signature verifier. |
There was a problem hiding this comment.
This comment was correct. The (Zero6-gated) claim matches an explicit registration-time guard that shipped in v0.7.1/v0.7.2/v0.7.3 and was dropped in v0.8.0:
// v0.7.3 precompile_provider.rs:63
PQ_ADDRESS => {
if !hardfork_flags.is_active(ArcHardfork::Zero6) {
return None;
}
...
}Alongside it, test_pq_precompile_not_available_without_zero6 was deleted and the surviving test flipped to assert availability under ArcHardforkFlags::default().
Deleting this line removes the last in-code trace of the intended behavior. Suggest restoring the guard instead and leaving the doc unchanged.
|
Agreed with @osr21's analysis -- the git-history trace against v0.7.1/v0.7.2/v0.7.3 is decisive, and the deleted |
The (Zero6-gated) doc was accurate: v0.7.x gated PQ at create_precompiles_map and v0.8.0 dropped that guard. Restore the registration-time check and the matched availability tests instead of deleting the comment. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Thanks @osr21 — agreed, the docs-only change had the cause/effect backwards. Pushed a follow-up that restores the v0.7.3 registration-time Zero6 gate in Treating this as a reference implementation for the internal sync, as you noted. Happy to close if maintainers want to wait on the #293 triage path instead. |
osr21
left a comment
There was a problem hiding this comment.
Disclosure: I'm not affiliated with Circle — an external community contributor, not a maintainer. I have no write access to this repository, so any review state I set (approval or change request) carries no merge authority and is advisory only. Please treat this as one contributor's technical assessment, and defer to Circle maintainers for the binding review.
Verified 4a9face4. All four points are addressed, and clearing my earlier request-changes. One thing to add: I understated the severity in that review, and the correction runs against my own "no live exposure" line.
The restoration checks out
- Guard is byte-identical to
v0.7.3. I diffed thePQ_ADDRESSarm atpr334againstgit show v0.7.3:crates/precompiles/src/precompile_provider.rs— no textual difference, comment included. test_pq_precompile_not_available_without_zero6is back assertingis_none()underArcHardforkFlags::default(), and the positive test is renamed totest_pq_precompile_available_with_zero6takingwith(&[ArcHardfork::Zero6]). That's the piece I most wanted covered.- The
(Zero6-gated)doc revert is dropped — the PR is now a single file andcrates/pq-precompile/src/lib.rsis untouched onmain.
Compiles by inspection (no cargo here, so stating the reasoning rather than a test run): the import correctly widens to {ArcHardfork, ArcHardforkFlags}, which was genuinely required; ArcHardforkFlags derives Copy (hardforks.rs:103), so reading hardfork_flags before it is captured by the move closure is fine; ArcHardforkFlags::with exists at hardforks.rs:167; and return None returns from the set_precompile_lookup closure rather than the enclosing fn — same structure as v0.7.3.
Nothing else in-tree breaks. The only other PQ_ADDRESS references are the address definition and the e2e suite, and that suite documents its own scope at execution-e2e/tests/e2e/pq_precompile.rs:26-27: it runs LOCAL_DEV, where Zero6 is active from genesis. No other test builds the map under default() and expects PQ.
Correcting myself: this is a historical-execution bug, not just a spec regression
I wrote that there is "no reachable network state where an ungated PQ_ADDRESS differs observably from a gated one." That holds for block production and it is wrong for block replay.
create_precompiles_map is not only an RPC-side helper — it is called on the execution path with per-block flags: evm.rs:1735 derives let hardfork_flags = self.get_hardfork_flags(&input.block_env); and passes them straight into it at :1743. Registration is therefore decided per block from block_env, which means the deleted guard changes how old blocks execute.
The relevant boundary on testnet, by binary search against rpc.testnet.arc.network (chain 0x4cef52, head 60426811):
block 44295020 ts 1779894516 2026-05-27T15:08:36Z <- last pre-Zero6 block
block 44295021 ts 1779894517 2026-05-27T15:08:37Z <- ARC_ZERO6_..._TESTNET
v0.8.0 was tagged 2026-08-28, three months after that — so no ungated binary ever produced a pre-Zero6 testnet block, which is why there is no persisted divergence for nodes that synced live. But roughly 44.3M blocks sit below that height, and an ungated binary registers PQ while replaying every one of them.
That is observable on the live fleet right now. eth_call to 0x1800000000000000000000000000000000000004 with empty calldata:
latest -> execution reverted: Input too short
44295021 -> execution reverted: Input too short
44295020 -> execution reverted: Input too short <- pre-Zero6
1000 -> execution reverted: Input too short <- pre-Zero6
Input too short is emitted only from the PQ path (pq-precompile/src/lib.rs:97, via the shared early-revert macro at precompiles/src/macros.rs:110). A call to an address with no precompile registered would not produce it. So the verifier is executing at heights where, under v0.7.x, it was not registered at all.
Two consequences:
- Historical simulation is already wrong.
eth_call/eth_estimateGaspinned to a pre-Zero6 block return PQ semantics that did not exist at that height. No hypothetical needed — that is the output above. - A from-genesis re-sync executes pre-Zero6 blocks with PQ registered. If any pre-Zero6 transaction called that address, replay diverges from what
v0.7.xoriginally produced.
Being precise about what I did not establish: I could not scan 44.3M blocks for a pre-Zero6 transaction to PQ_ADDRESS, so I am not claiming realized state-root divergence — only that the code permits it and that the simulation surface is already affected. Mainnet is untouched either way, since ARC_MAINNET_HARDFORKS sets Zero6 => ForkCondition::Block(0). Usual caveat on the fleet: deep-history responses can be served by different binaries than head, though here the newer ungated behavior is what shows up at depth, which is the direction that matters.
So this PR restores historical-replay consistency with v0.7.x, rather than merely re-aligning code with a doc comment. That seems worth stating in the description, since it changes who needs to care.
One suggestion
The v0.8.0 gate removal never got a changelog entry, and restoring it is a behavior change relative to v0.8.0. Worth a CHANGELOG.md line under the next release — something like [EL] Restore Zero6 registration gate for the PQ precompile (regressed in v0.8.0); pre-Zero6 blocks no longer execute the verifier on replay — so the round trip is legible to anyone bisecting later.
Approving on the substance. Given this touches per-block precompile registration, I would still defer to maintainers on whether it lands here or through the internal sync / #293 triage path that @ygd58 is holding for — my approval is advisory and carries no merge authority. Standard caveat: no cargo or rustc in my environment, so the Rust side is source review plus git show against published tags; the RPC results above are live calls.
Record the v0.8.0 regression fix so replay/bisect history stays legible. Co-authored-by: Cursor <cursoragent@cursor.com>
|
CHANGELOG Unreleased was added per osr21 review — notes the Zero6 PQ registration gate restore so replay/bisect history stays legible. |
Summary
Restores the v0.7.x Zero6 registration-time gate for the PQ precompile that was dropped in the v0.8.0 sync, and puts back the matched availability tests.
Per review from @osr21 on the previous docs-only approach: the
(Zero6-gated)comment was the last correct statement of intended behavior._hardfork_flagslooked unused because the guard that read it was deleted, not because gating was never meant to exist.Changes
ArcHardfork::Zero6check increate_precompiles_mapforPQ_ADDRESStest_pq_precompile_available_with_zero6/test_pq_precompile_not_available_without_zero6(Zero6-gated)doc onPQ_ADDRESSNote
Public PRs here are reference implementations for Circle's internal sync. Happy to close or adjust if maintainers prefer a different direction after security triage on #293.
Resolves #293 (option 2 from the issue).