docs: mark orbinum-zk-circuits as a non-production reference implementation - #137
Open
0xrlawrence wants to merge 1 commit into
Open
docs: mark orbinum-zk-circuits as a non-production reference implementation#1370xrlawrence wants to merge 1 commit into
0xrlawrence wants to merge 1 commit into
Conversation
…tation The verification keys registered on chain are generated from the Circom sources in orbinum/circuits. This crate is a separate arkworks implementation of the same circuits, and nothing in its documentation says so. It carries a crates.io badge, an install snippet, and a "Prove" usage example, so the reasonable reading is that it is the thing that produces Orbinum proofs. The two have drifted: - UnshieldCircuit exposes 6 public signals; unshield.circom declares 7, including change_commitment, which is also what UNSHIELD_PUBLIC_INPUTS and encode_unshield expect. - UnshieldCircuit does not model a change note at all, while both the pallet and the deployed circuit support partial unshield. - Neither circuit here constrains note values to a range. Balance conservation is enforced as field arithmetic, so sum(inputs) == sum(outputs) + fee can be satisfied by wrapping modulo the BN254 scalar field. The deployed Circom circuits apply Num2Bits(128) to every value and are not affected. The last point is why this is worth a warning rather than a note: a trusted setup run against TransferCircuit or UnshieldCircuit would produce a verifier that accepts inflated notes, and nothing currently warns a reader off doing that. Adds the warning to the crate README and the crate-level docs, and links orbinum/circuits from the root README, which described the circuits without saying where they live. Documentation only. No code is changed, and the divergences above are described rather than fixed, since which direction to reconcile them is a decision for the team. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The verification keys registered on chain are generated from the Circom sources in orbinum/circuits.
primitives/zk-circuitsis a separate arkworks implementation of the same circuits, and nothing in its documentation says so. It carries a crates.io badge, an install snippet, and a "Prove" usage example, so the reasonable reading is that it is what produces Orbinum proofs.The two have drifted:
primitives/zk-circuitsorbinum/circuitschange_commitmentNum2Bits(128)The signal count is the easy one to spot:
UnshieldCircuit::public_inputs()returns six, whileUNSHIELD_PUBLIC_INPUTS,encode_unshieldandunshield.circomall agree on seven.The range checks are why this is worth a warning rather than a note. Balance conservation in this crate is enforced as field arithmetic with no bound on any value, so
sum(inputs) == sum(outputs) + feecan be satisfied by wrapping modulo the BN254 scalar field. A trusted setup run againstTransferCircuitorUnshieldCircuitwould produce a verifier that accepts inflated notes, and nothing currently warns a reader off doing exactly that. The deployed Circom circuits applyNum2Bits(128)to every value and are not affected, which is why this is a packaging problem rather than a chain problem.What this changes
Adds the warning to the crate README and to the crate-level docs, and links
orbinum/circuitsfrom the root README, which described the circuits without saying where they live.The divergences are described rather than fixed. Which direction to reconcile them is your call: deleting the crate, demoting it to a checked reference implementation with cross-implementation vector tests against the
orbinum/circuitsfixtures, or bringing it back into line all seem defensible, and I did not want to pick one on your behalf.Verification
Documentation and comments only, no code changed. I was not able to build the workspace, so
cargo docis unrun; the crate-level block is plain//!text with intra-doc links avoided, so I would not expect rustdoc warnings, but treat that as unverified.Separately, I have emailed
security@orbinum.netabout a number of higher-severity findings from the same review pass. Those are deliberately not in this PR, in any other PR, or in a public issue. Nothing above is one of them: the range-check gap is confined to this unused crate, and I confirmed the deployed circuits are sound before writing any of this down.