feat: add privacy/commitments access-registry to marketplace - #10
feat: add privacy/commitments access-registry to marketplace#10Mrwicks00 wants to merge 1 commit into
Conversation
Replace the buyer->prompt_id storage pattern with an anonymous commitment scheme. Introduces a PolicyData struct keyed by opaque u64 id, a SHA256-based Merkle accumulator root updated on each access issuance, and a nullifier set that prevents reuse without revealing which prompt was consumed. New DataKey variants: PolicyCounter, Policy(u64), AccessRoot, Nullifier(BytesN<32>). New contract functions: register_policy, issue_access, root, consume, used. New events: PolicyRegistered, AccessIssued, NullifierConsumed. New privacy module at contracts/marketplace/src/privacy/ with Bytes32 type alias. 27 tests pass (17 existing + 10 new privacy storage tests). Closes Stellar-AgentVerse#2
|
Thanks for the PR. The overall direction is good, and I think the feature introduces useful building blocks for a privacy/access registry. However, I don't think this is merge-ready yet because several of the security and protocol guarantees implied by the API are not actually enforced. The main concern is that the contract currently stores privacy-related primitives, but does not fully enforce the properties those primitives are expected to guarantee. 1.
|
Joaco2603
left a comment
There was a problem hiding this comment.
Review — PR #10
Thanks @Mrwicks00 for the implementation. The overall direction of adding privacy primitives is valuable, but I agree with the concerns already raised by Joaco2603 in the PR comments — this is not ready to merge in its current form.
Critical: protocol guarantees not enforced
The contract stores privacy primitives but does not enforce the security properties the API implies:
1. consume() is publicly callable without authentication or proof validation
Anyone can mark any nullifier as consumed before the legitimate user. This is a real frontrunning/DoS vector. Either:
- Gate
consume()behind proof-of-ownership (e.g., the caller must prove knowledge of the leaf/secret), or - Acknowledge this is a public "mark as spent" utility with no security guarantees, rename accordingly, and document the limitation.
2. issue_access() does not enforce payment or proofs
The function checks the policy exists but does not verify payment, validate the session commitment, or enforce any policy condition. The claim "payment verification happens off-chain" is fine, but the API surface implies stronger guarantees than the contract provides.
3. PolicyData.commitment and PolicyData.price are stored but not enforced
Neither field participates in access enforcement. price is validated to be positive on registration but never checked on issuance. commitment is stored but never verified against the leaf.
4. The "Merkle accumulator" is a hash chain
sha256(root || leaf)This is not a Merkle tree or an incremental Merkle accumulator — it is a simple hash chain. Membership proofs cannot be constructed from this structure. Either implement a provable accumulator or rename/re-document accordingly.
5. Duplicate leaf issuance is not prevented
The same (policy_id, session_commitment) pair produces the same leaf, yet issue_access can be called multiple times with identical inputs, altering the root each time. This creates ambiguous accumulator semantics.
6. Nullifier scope is undefined
Are nullifiers global or policy-scoped? The current storage treats them as global (DataKey::Nullifier(BytesN<32>)), but this is not documented and has privacy implications (global nullifiers enable correlation across policies).
Missing tests
The test suite demonstrates storage behavior but does not validate protocol security:
- No frontrunning/DoS tests for
consume() - No duplicate leaf issuance tests
- No tests verifying
issue_accessrejects when payment conditions are not met - No adversarial tests for unused/null policies
- No nullifier scope tests
What I would keep
- The
Bytes32type alias andprivacymodule structure - The
register_policyfunction with admin gating - The event definitions
Veredicto: CHANGES_REQUESTED. The feature needs security enforcement or explicit documented limitations before merge. Happy to re-review once addressed.
feat: Anonymous Commitment-Based Access Registry
Summary
Implements the privacy architecture described in issue #2.
Replaces the
DataKey::Purchase(Address, String) → boolpattern — which leaks the buyer-to-prompt relationship in plain storage — with an anonymous commitment scheme consisting of three components:SHA256(prompt_policy ∥ ciphertext_ref ∥ expiration ∥ salt); content_uri and prompt identity never touch the ledger in the private flowissue_accesscall; each purchase produces an opaque leaf (SHA256(policy_id_bytes ∥ session_commitment)) with no buyer address in storageconsume(nullifier)marks a credential as spent;used(nullifier)checks without exposing which prompt was consumedFiles Changed
contracts/marketplace/src/privacy/mod.rscontracts/marketplace/src/privacy/types.rsBytes32 = BytesN<32>type aliascontracts/marketplace/src/storage/types.rsPolicyDatastruct; addedPolicyCounter,Policy(u64),AccessRoot,Nullifier(BytesN<32>)toDataKeycontracts/marketplace/src/contract.rscontracts/marketplace/src/lib.rspub mod privacycontracts/marketplace/src/tests.rsNew API
AccessRegistry trait (contract functions)
AccessNullifiers trait (contract functions)
Acceptance Criteria
content_urinot stored in clear for private flowPolicyDatastores onlycommitment: BytesN<32>+pricePolicyRegistered,AccessIssued,NullifierConsumed— no plaintext contentissue_accessdoes not expose wallet + prompt in public storageAccessRootand leaf derivation; no(Address, String)keyconsume(nullifier)blocks reuseused(nullifier)works correctlyfalsebefore consume,trueafterDesign Notes
Two flows coexist: The existing public marketplace (
register_prompt,buy_prompt,has_access) is unchanged for backward compatibility. The new private flow is opt-in viaregister_policy/issue_access.Merkle accumulator:
new_root = SHA256(old_root ∥ leaf)is a simple hash-chain accumulator. It does not support Merkle proofs in the classical sense, but it establishes a commitment to the set of issued leaves. A full sparse Merkle tree would require off-chain construction and on-chain root verification, which is out of scope for this issue.Auth model:
issue_accessis admin-gated. The admin verifies off-chain that the buyer paid (e.g., via a token-burn tx), then callsissue_access(policy_id, session_commitment)wheresession_commitmentis supplied by the buyer. This prevents the contract from ever writing(buyer_address, prompt_id)to storage.Unlinkability: Two purchases of the same policy with different
session_commitmentvalues produce distinct, unrelated leaves — confirmed bytest_issue_access_storage_is_opaque.Closes #2