Skip to content

fix(precompiles): restore Zero6 registration gate for PQ - #334

Open
kutluhaneth46 wants to merge 3 commits into
circlefin:mainfrom
kutluhaneth46:fix/pq-precompile-drop-zero6-doc-claim
Open

fix(precompiles): restore Zero6 registration gate for PQ#334
kutluhaneth46 wants to merge 3 commits into
circlefin:mainfrom
kutluhaneth46:fix/pq-precompile-drop-zero6-doc-claim

Conversation

@kutluhaneth46

@kutluhaneth46 kutluhaneth46 commented Sep 3, 2026

Copy link
Copy Markdown

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_flags looked unused because the guard that read it was deleted, not because gating was never meant to exist.

Changes

  1. Restore the ArcHardfork::Zero6 check in create_precompiles_map for PQ_ADDRESS
  2. Restore test_pq_precompile_available_with_zero6 / test_pq_precompile_not_available_without_zero6
  3. Keep the (Zero6-gated) doc on PQ_ADDRESS

Note

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).

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>

@osr21 osr21 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Restore the v0.7.3 guard verbatim in create_precompiles_map — it's already the spec, no design work needed.
  2. Restore test_pq_precompile_not_available_without_zero6.
  3. Revert test_pq_precompile_available to pass ArcHardforkFlags::with(&[ArcHardfork::Zero6]) and restore its original name. This is the piece I'd most want covered — leaving it asserting availability under default() 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.
  4. 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.

Comment thread crates/pq-precompile/src/lib.rs Outdated
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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ygd58

ygd58 commented Sep 4, 2026

Copy link
Copy Markdown

Agreed with @osr21's analysis -- the git-history trace against v0.7.1/v0.7.2/v0.7.3 is decisive, and the deleted test_pq_precompile_not_available_without_zero6 plus the flipped assertion on the surviving test is exactly the kind of regression-then-pinned-as-correct pattern that's worth catching before a squash-sync carries it into a release. To confirm from my side: the guard restoration is what I'd proposed on #293 before agreeing to hold for guidance, so this matches. Still holding off on opening a competing PR per that same agreement -- would rather this land once, in the right direction, than risk two conflicting fixes in flight.

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>
@kutluhaneth46 kutluhaneth46 changed the title docs(pq-precompile): drop inaccurate Zero6-gated claim fix(precompiles): restore Zero6 registration gate for PQ Sep 4, 2026
@kutluhaneth46

Copy link
Copy Markdown
Author

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 create_precompiles_map, restores test_pq_precompile_not_available_without_zero6, and switches the positive test back to ArcHardforkFlags::with(&[Zero6]) with the original name. The (Zero6-gated) doc comment stays.

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 osr21 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 the PQ_ADDRESS arm at pr334 against git show v0.7.3:crates/precompiles/src/precompile_provider.rs — no textual difference, comment included.
  • test_pq_precompile_not_available_without_zero6 is back asserting is_none() under ArcHardforkFlags::default(), and the positive test is renamed to test_pq_precompile_available_with_zero6 taking with(&[ArcHardfork::Zero6]). That's the piece I most wanted covered.
  • The (Zero6-gated) doc revert is dropped — the PR is now a single file and crates/pq-precompile/src/lib.rs is untouched on main.

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:

  1. Historical simulation is already wrong. eth_call / eth_estimateGas pinned to a pre-Zero6 block return PQ semantics that did not exist at that height. No hypothetical needed — that is the output above.
  2. 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.x originally 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>
@kutluhaneth46

Copy link
Copy Markdown
Author

CHANGELOG Unreleased was added per osr21 review — notes the Zero6 PQ registration gate restore so replay/bisect history stays legible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pq-precompile doc claims "Zero6-gated" but hardfork_flags is never read in the dispatch or execution path

3 participants