feat: add unstable-voting-circuits feature to widen internals by czarcas7ic · Pull Request #488 · zcash/orchard

czarcas7ic · 2026-04-20T16:14:46Z

Summary

Adds an opt-in feature flag unstable-voting-circuits that surgically exposes the orchard internals needed to build a downstream voting circuit against this crate, following the same pattern as the existing unstable-frost flag.

All visibility changes only upgrade to pub under --features unstable-voting-circuits. The unstable- prefix signals that any items exposed under this flag are not covered by the crate's semver stability guarantees and may change in any future release.

ebfull · 2026-04-20T16:25:10Z

Note that this should be merged after we do the 0.13 release.

The feat commit added `OrchardBaseFieldBases::SpendAuthGBase` and `OrchardShortScalarBases::SpendAuthGShort` to the `pub use` re-export list in src/constants.rs, but `mod constants;` was still private — so the re-exports had no external effect, triggering `unused_imports` on the new names and failing the `deny: warnings` Clippy job on upstream CI. Making `pub mod constants;` with `#[allow(missing_docs)]` mirrors the identical hunk already present in zcash#488 (visibility-widening). When both PRs merge, git resolves the shared hunk trivially — no dead code or follow-up cleanup needed.

Generalizes OrchardFixedBases to support multiplying the spend authorization generator G^Orchard by: 1. Full-width base-field scalars via FixedPointBaseField (new OrchardBaseFieldBases::SpendAuthGBase variant, reuses the existing 85-window U/Z tables) 2. 64-bit signed short scalars via FixedPointShort (new OrchardShortScalarBases::SpendAuthGShort variant, with new 22-window Z_SHORT/U_SHORT precomputed tables) New types: - OrchardBaseFieldBases { NullifierK, SpendAuthGBase } - OrchardShortScalarBases { ValueCommitV, SpendAuthGShort } Breaking change: OrchardFixedBases is now {Full(OrchardFixedBasesFull), Base(OrchardBaseFieldBases), Short(OrchardShortScalarBases)} instead of the previous flat {Full(...), NullifierK, ValueCommitV}. Internal callers (value_commit_orchard, derive_nullifier) are rewired with .into(). The existing `impl From<NullifierK|ValueCommitV> for OrchardFixedBases` continue to work, so callers using `.into()` are unaffected; only callers that match on the enum variants directly need to update. Also flips `mod constants;` to `#[allow(missing_docs)] pub mod constants;` so that the new `pub use fixed_bases::{OrchardBaseFieldBases, OrchardShortScalarBases}` re-exports in src/constants.rs are externally visible. The identical 3-line hunk is also present in the sibling visibility-widening PR (zcash#488); git will auto- resolve on merge. The main Orchard action circuit VK is unchanged, enforced by the existing byte-equality check in src/circuit.rs::test_against_stored_vk (format!("{:#?}\n", vk.vk.pinned()) == include_str!("circuit_description")). All existing orchard tests pass unchanged.

Generalizes OrchardFixedBases to support multiplying the spend authorization generator G^Orchard by: 1. Full-width base-field scalars via FixedPointBaseField (new OrchardBaseFieldBases::SpendAuthGBase variant, reuses the existing 85-window U/Z tables) 2. 64-bit signed short scalars via FixedPointShort (new OrchardShortScalarBases::SpendAuthGShort variant, with new 22-window Z_SHORT/U_SHORT precomputed tables) New types: - OrchardBaseFieldBases { NullifierK, SpendAuthGBase } - OrchardShortScalarBases { ValueCommitV, SpendAuthGShort } Breaking change: OrchardFixedBases is now {Full(OrchardFixedBasesFull), Base(OrchardBaseFieldBases), Short(OrchardShortScalarBases)} instead of the previous flat {Full(...), NullifierK, ValueCommitV}. Internal callers (value_commit_orchard, derive_nullifier) are rewired with .into(). The existing `impl From<NullifierK|ValueCommitV> for OrchardFixedBases` continue to work, so callers using `.into()` are unaffected; only callers that match on the enum variants directly need to update. Also flips `mod constants;` to `#[allow(missing_docs)] pub mod constants;` so that the new `pub use fixed_bases::{OrchardBaseFieldBases, OrchardShortScalarBases}` re-exports in src/constants.rs are externally visible. The identical 3-line hunk is also present in the sibling visibility-widening PR (zcash#488); git will auto- resolve on merge. Adds a public-crate re-export at public/src/lib.rs so that orchard::constants::{OrchardBaseFieldBases, OrchardShortScalarBases, OrchardFixedBases, OrchardFixedBasesFull, NullifierK, ValueCommitV} are reachable through the public `orchard` crate, matching the CHANGELOG entries. The main Orchard action circuit VK is unchanged, enforced by the existing byte-equality check in src/circuit.rs::round_trip (format!("{:#?}\n", vk.vk.pinned()) == include_str!("circuit_description")). All existing orchard tests pass unchanged.

nuttycom

NACK pending detailed discussion, breaking the sealed-ness of the SigType trait needs strong justification. Also, are you certain that all of this surface area actually needs to be exposed?

czarcas7ic · 2026-04-21T18:10:52Z

@nuttycom I had a more confined version that only exposed the items that voting circuits needed, but then got guidance from @ebfull to convert anything with pub(crate) to pub instead.

To move things forward, I can open a second PR that is only the items that we actually need for voting circuits if that is desired.

nuttycom · 2026-04-21T18:12:50Z

@nuttycom I had a more confined version that only exposed the items that voting circuits needed, but then got guidance from @ebfull to convert anything with pub(crate) to pub instead.

To move things forward, I can open a second PR that is only the items that we actually need for voting circuits if that is desired.

Yes, please. We can always expose more if needed, but it's more of a hassle to close things off.

czarcas7ic · 2026-04-21T18:13:24Z

@nuttycom on it now, thanks!

ebfull · 2026-04-21T18:36:51Z

@nuttycom The purpose of the crate split was to enable us to avoid having to make deliberate stable API decisions every time someone needs to repurpose an internal abstraction. As a general rule, anything that was pub(crate) should be safe to expose from the orchard_internal crate, since we're using the orchard crate as a stable guard. (I explicitly told @czarcas7ic that anything that was visibility-scoped to a module needed to have specific intention, but as a rule pub(crate) -> pub is fair game IMO.)

If we really needed to close down visibility afterward for some reason we could do it with a major (breaking) release of orchard_internal, which we should feel free to do whenever we want.

The specific change that was a sealed trait pattern that was turned into a pub is a mistake but not because of the PR author. The existing code is wrong, because a sealed trait pattern should almost never be pub(crate), it should be pub inside of a private module.

czarcas7ic · 2026-04-21T20:45:23Z

@nuttycom just for clarity, I do have the minimal change staged on our fork as promised (valargroup#15), but it seems like the full widening might still be desired (aside from the error you flagged, which is now fixed)

nuttycom

utACK 1a1a057; I'd prefer if a constructor were exposed for Nullifier instead of making the internals public but I won't block on it.

daira · 2026-04-23T17:19:34Z

+    #[cfg_attr(feature = "unstable-voting-circuits", visibility::make(pub))]
    pub(crate) fn zero() -> Self {


This can be pub, it's equivalent to NoteValue::from_raw(0) which is already pub. (Alternatively, that means it doesn't actually need to be changed by this PR.)

Suggested change

#[cfg_attr(feature = "unstable-voting-circuits", visibility::make(pub))]

pub(crate) fn zero() -> Self {

pub fn zero() -> Self {

Instead of making this a pub method, I'd like to make this a pub constant ZERO (and remove the pub(crate) method).

daira

utACK with suggestions; like Kris I would strongly prefer the suggested change to Nullifier. Note that anything I suggested to make unconditionally pub must be documented as an addition.

czarcas7ic · 2026-04-23T17:39:12Z

Some of the suggestions I accepted seems to have been reverted on my force push, I am re-applying them now. Will post here when ready for re-review. Thanks all!

Mirrors the existing `unstable-frost` pattern: flag off leaves the public API byte-identical to main; flag on promotes the `pub(crate)` items a downstream voting circuit needs to `pub`. Uses `#[cfg_attr(..., visibility::make(pub))]` for most items, with cfg twin-declarations where the `visibility` proc-macro can't apply on stable (non-inline modules, tuple-struct fields).

daira · 2026-04-23T18:06:10Z

I have some suggestions that I want to just push onto the branch, please hold off force-pushing for a bit.

Edit: ok for you to push now (I didn't have permission to push to this branch in any case), see valargroup#18 for my suggested changes.

Remove the six `#[allow(missing_docs)]` attributes gating newly-public modules under `unstable-voting-circuits`, and document every item the lint was suppressing (modules, sub-configs, chip wrappers, enum variants, and the `generator()` entry points of each fixed-base table). The crate-wide `#![deny(missing_docs)]` policy now applies uniformly under the feature rather than being selectively relaxed. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

daira · 2026-04-23T18:18:56Z

I've opened valargroup#18 on to this PR, please review @nuttycom @czarcas7ic.

daira

Review — current state (assuming doc PR #18 merged)

Resolved since the first round of comments:

Sealed-trait break in src/primitives/redpallas.rs — fixed by the squashed commit; ebfull confirmed at line 186.
Nullifier opaque-wrapper (Kris's and Daira-Emma's request) — applied.
L_ORCHARD_{BASE,SCALAR} / L_VALUE unconditionally pub (Daira-Emma's suggestion) — applied, plus pub use at crate root.
src/note.rs re-exports (Daira-Emma's suggestion) — applied.
NoteValue::zero unconditionally pub (Daira-Emma's suggestion) — applied.
Address::{g_d, pk_d} documented in CHANGELOG (Kris's request) — done.
#[allow(missing_docs)] on the six newly-public modules — replaced with real docs in valargroup#18 (pending merge into this branch).
src/primitives/redpallas.rs:118 (cfg(test) fn dummy) — current state matches Kris's "doesn't need to change."
Daira-Emma's "Address::from_parts?" — reply clarified voting-circuits only reads, doesn't construct; acceptable.
src/circuit/gadget.rs:31 visibility::make attempt — ebfull linked the non-inline modules in proc macro input unstable-issue showing it can't work; Kris agreed "fine as it is."

[Removed false-positive discussion of clippy failures caused by version skew between my local clippy and CI's.]

Still-live design discussion:

Kris suggested a pub const ZERO: NoteValue in src/value.rs rather than pub fn zero(). The current code went with the pub fn. A const is marginally more idiomatic for a zero-argument invariant, and usable in const contexts; the fn matches NoteValue::from_raw(0)'s shape. Maintainer call.

Minor cleanup (discretionary, pre-existing):

gen_const_array in src/constants/util.rs is pub but used only within the crate (util.rs, spec.rs). Kris flagged this. Not introduced by this PR, but since the PR is already in that file, a pub(crate) narrowing fits.
Kris's three sinsemilla.rs concerns (items could be private / panic needs docs) are pre-existing; not introduced by this PR. Out of scope.
SpendInfo pub(crate) fields in src/builder.rs — same story, pre-existing.

Structural (Kris's main concern):

"Huge expansion of public API" — still true in absolute terms (~30 items conditionally graduated to pub under the feature, mapped 1:1 against valargroup/voting-circuits/UPSTREAM.md's checklist). Mitigations in place: feature flag, unstable- prefix, CHANGELOG flagging non-semver-stable. Each exposure justified by a concrete downstream consumer. Still worth being conservative about which items migrate from unstable-voting-circuits to unconditional pub in future releases.

🤖 This comment was prepared with the assistance of Claude Opus 4.7 (1M context).

czarcas7ic · 2026-04-23T18:45:52Z

I've opened valargroup#18 on to this PR, please review @nuttycom @czarcas7ic.

Merged it in, thanks!

czarcas7ic · 2026-04-23T18:54:31Z

I am now mobile but I will address Kris' concern at my soonest availability

daira

utACK

ValarDragon · 2026-04-23T22:21:42Z

For the one remaining item:

Kris suggested a pub const ZERO: NoteValue in src/value.rs rather than pub fn zero(). The current code went with the pub fn. A const is marginally more idiomatic for a zero-argument invariant, and usable in const contexts; the fn matches NoteValue::from_raw(0)'s shape. Maintainer call.

I'll just remove this as a change in this PR, and do that in an isolated follow-up

…cond PR as its not core to voting

czarcas7ic commented Apr 20, 2026

View reviewed changes

Comment thread src/keys.rs Outdated

ebfull mentioned this pull request Apr 20, 2026

feat: widen visibility on orchard_internal for external reuse ebfull/orchard#1

Closed

ebfull previously approved these changes Apr 20, 2026

View reviewed changes

czarcas7ic mentioned this pull request Apr 20, 2026

[staging] feat: add SpendAuthG fixed-base multiplication support (stacked on visibility-widening) valargroup/orchard#13

Closed

czarcas7ic marked this pull request as ready for review April 20, 2026 16:47

czarcas7ic mentioned this pull request Apr 20, 2026

feat: add SpendAuthG fixed-base multiplication support ebfull/orchard#2

Closed

ebfull mentioned this pull request Apr 21, 2026

feat: add SpendAuthG fixed-base multiplication support #489

Merged

nuttycom reviewed Apr 21, 2026

View reviewed changes

Comment thread src/primitives/redpallas.rs Outdated

nuttycom requested changes Apr 21, 2026

View reviewed changes

czarcas7ic dismissed ebfull’s stale review via 974ee0a April 21, 2026 20:15

ebfull approved these changes Apr 21, 2026

View reviewed changes

czarcas7ic force-pushed the adam/visibility-widening branch from 163527e to ec28cec Compare April 21, 2026 20:34

ebfull approved these changes Apr 21, 2026

View reviewed changes

daira reviewed Apr 23, 2026

View reviewed changes

Comment thread src/note.rs

nuttycom previously approved these changes Apr 23, 2026

View reviewed changes

Comment thread src/circuit/gadget.rs

daira reviewed Apr 23, 2026

View reviewed changes

daira previously approved these changes Apr 23, 2026

View reviewed changes

czarcas7ic dismissed stale reviews from daira, nuttycom, and ebfull via 81f879c April 23, 2026 17:23

czarcas7ic force-pushed the adam/visibility-widening branch from 81f879c to e54e4e4 Compare April 23, 2026 17:34

czarcas7ic force-pushed the adam/visibility-widening branch 2 times, most recently from 6b79bcc to 2341449 Compare April 23, 2026 17:47

czarcas7ic force-pushed the adam/visibility-widening branch from 2341449 to c0b6b91 Compare April 23, 2026 17:51

daira mentioned this pull request Apr 23, 2026

docs: replace #[allow(missing_docs)] with actual doc comments valargroup/orchard#18

Merged

daira reviewed Apr 23, 2026

View reviewed changes

Merge pull request #18 from daira/missing-docs-unstable-voting-circuits

4dc6386

daira previously approved these changes Apr 23, 2026

View reviewed changes

Restore visibility of zero() to pub(crate); Change visibility in a se…

21d85e8

…cond PR as its not core to voting

ValarDragon dismissed daira’s stale review via 21d85e8 April 23, 2026 22:25

ebfull approved these changes Apr 23, 2026

View reviewed changes

ebfull merged commit d05a1f6 into zcash:main Apr 24, 2026
15 checks passed

ValarDragon mentioned this pull request Apr 24, 2026

value: Replace NoteValue::zero() with NOTE_VALUE_ZERO const #495

Merged

5 tasks

p0mvn mentioned this pull request Apr 25, 2026

orchard 0.13: stack upstream zcash/orchard#489 + #495 valargroup/orchard#19

Merged

ebfull mentioned this pull request Apr 27, 2026

Release orchard v0.13.1 #498

Merged

		#[cfg_attr(feature = "unstable-voting-circuits", visibility::make(pub))]
		pub(crate) fn zero() -> Self {

	#[cfg_attr(feature = "unstable-voting-circuits", visibility::make(pub))]
	pub(crate) fn zero() -> Self {
	pub fn zero() -> Self {

Conversation

czarcas7ic commented Apr 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

Uh oh!

ebfull commented Apr 20, 2026

Uh oh!

Uh oh!

nuttycom left a comment

Choose a reason for hiding this comment

Uh oh!

czarcas7ic commented Apr 21, 2026

Uh oh!

nuttycom commented Apr 21, 2026

Uh oh!

czarcas7ic commented Apr 21, 2026

Uh oh!

ebfull commented Apr 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

czarcas7ic commented Apr 21, 2026

Uh oh!

Uh oh!

nuttycom left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

daira Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

nuttycom Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

daira left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

czarcas7ic commented Apr 23, 2026

Uh oh!

daira commented Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daira commented Apr 23, 2026

Uh oh!

daira left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Review — current state (assuming doc PR #18 merged)

Uh oh!

czarcas7ic commented Apr 23, 2026

Uh oh!

czarcas7ic commented Apr 23, 2026

Uh oh!

daira left a comment

Choose a reason for hiding this comment

Uh oh!

ValarDragon commented Apr 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

czarcas7ic commented Apr 20, 2026 •

edited

Loading

ebfull commented Apr 21, 2026 •

edited

Loading

daira left a comment •

edited

Loading

daira commented Apr 23, 2026 •

edited

Loading

daira left a comment •

edited

Loading