test(privacy): add privacy-preserving test coverage for LEZ programs - #215
test(privacy): add privacy-preserving test coverage for LEZ programs#215jonesmarvin8 wants to merge 13 commits into
Conversation
… programs Validates the Q2 privacy features (shield/deshield, private-to-private transfers, existing-account crediting, group-owned accounts, and private PDAs) against the token and ata program flows, which previously ran almost entirely in public context. Adds the key_protocol dependency for GMS-based group-account tests and introduces a docs/privacy-test-matrix.md tracker mapping each program/instruction/ privacy-dimension combination to pass, fail, or not-expressible, with root-cause findings for each gap (notably that private PDAs are structurally unsupported by any program currently deriving addresses via for_public_pda). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…/ATA gaps Add Stablecoin privacy-preserving tests for WithdrawCollateral and RepayDebt (personal and group-owned variants), plus a regression test confirming OpenPosition is incompatible with the privacy circuit (chained-call re-authorization). Close the last planned Token row (MintWithAuthority to a private holding) and the ATA owner-signer gap for Transfer (personal and group-owned), plus a defensive Create/group-owner test. Extract shared privacy-test helpers (identity builders, GroupOwner seal/unseal handshake) into integration_tests/src/lib.rs and use them throughout token.rs, collapsing duplicated InputAccountIdentity/account construction. Update docs/privacy-test-matrix.md with all new findings.
…, refine ATA/token coverage Documents the private-account primitives (private PDA vs public PDA, group-shared accounts) and per-program privacy test results in docs/findings.md. Adds a stablecoin test confirming WithdrawCollateral can't pay out to a brand-new private destination, and folds in further ATA/token privacy test refinements.
…eLiquidity privacy tests Adds private-account tests for AMM's SwapExactInput/SwapExactOutput, AddLiquidity, and RemoveLiquidity confirming the "Invalid account_identities length" circuit bug also fires with real private accounts, not just the all-public control case, plus a distinct RemoveLiquidity finding (destination must already exist). Also deduplicates the shielded_token_transfer test helper and updates findings/matrix docs accordingly.
…, refine ATA/token coverage Adds ata_group_owned_owner_signing (Burn's missing GROUP variant) and two NewFungibleDefinition private-initial-holder tests to Token. Removes two experimental PDA-external-seed Token tests that had no real coverage purpose. Documents the root cause of the AMM privacy-test blocker (a spel-framework guest-wrapper filter silently drops the clock account before either transaction validator sees it) with full findings/matrix writeups, and reconciles table gaps found while cross-checking each program's privacy tests against docs/findings.md.
…ivacy tests, add 5 more Root cause: advance_clock() left the clock account DEFAULT_PROGRAM_ID-owned, which spel-framework's guest dispatcher (upstream in logos-co/spel) silently drops from a program's output as an unclaimed, non-default, default-owned account. That desynced the privacy circuit's account count from the caller-supplied identities, blocking every AMM chained-call privacy test with "Invalid account_identities length". Fix: own the fixture's clock account with a placeholder non-default ProgramId. Rewrote the 5 previously not-expressible Swap/AddLiquidity/RemoveLiquidity privacy tests to assert real success (transaction applied, resulting state and private commitments verified) now that they're unblocked. Added 5 more tests: Swap to a fresh PrivateUnauthorized/PrivateAuthorizedInit destination (both confirmed not-expressible, for two distinct reasons — guest ABI signer requirement vs. destination-must-already-exist), and NewDefinition with a private PrivateAuthorizedInit/PrivateUnauthorized initial LP holder (expressible for the former, not for the latter). Updated docs/findings.md and docs/privacy-test-matrix.md accordingly.
…n-zone The spel-framework dispatcher filter that drops the clock account is real, but ValidatedStateDiff::from_public_transaction never checks that a program's output accounts match the caller-declared message.account_ids, unlike the privacy circuit's own account_identities.len() == states_iter.len() check. That absence is why the drop went unnoticed by every pre-existing public AMM test. Update findings.md's conclusion and add a dated correction to privacy-test-matrix.md attributing this second, independent gap to logos-execution-zone.
…n_tests ff89025 bumped the workspace's nssa/nssa_core to logos-execution-zone v0.2.0 but missed integration_tests' own directly-pinned clock_core/key_protocol deps, left on v0.2.0-rc6. That split the dependency graph across two source revisions of lee_core, so types like NullifierPublicKey stopped unifying across crate boundaries and integration_tests failed to compile. Bump the two strays to v0.2.0 to match the rest of the workspace.
0x-r4bbit
left a comment
There was a problem hiding this comment.
@jonesmarvin8 Thank you for this!
I've left some comments. Maybe you can clarify those.
To me it's a little bit unclear if there's any action needed on our side.
Some comments in the docs suggest we need to adjust the programs, but it's not super explicit.
Can you confirm this?
|
|
||
| - `PrivateUnauthorized` | ||
|
|
||
| A special case for private accounts initialization that uses only public keys `npk` and `vpk`. Example: Alice can use Bob's keys (`npk`, `vpk`) and an `identifier` to send Bob a private transaction. Since Alice does not know the corresponding `nsk`, she is spend the resulting private account. E.g., Alice cannot authorize the transaction. |
There was a problem hiding this comment.
she is spend the resulting private account
This doesn't make a lot of sense to me. Would you mind clarifying?
There was a problem hiding this comment.
Yes....apparently my knowledge of written language gave up when I was writing...and proofreading this section. My apologies.
It should have been "Since Alice does not know the corresponding nsk, she is unable to spend the resulting private account."
| Only the account owner can (1) update their initialized account, and (2) use functions that require authorization with their account. | ||
|
|
||
| ### Remark | ||
| - `PrivateUnauthorized` initialization is used for account initialization. `is_authorized = false` is a protection that does not seem crucial. Artifically, blocks some functions. |
There was a problem hiding this comment.
is_authorized = falseis a protection that does not seem crucial. Artifically, blocks some functions.
Can you clarify what exactly this is trying to say? is_authorized is actually quite crucial for atm. But maybe you're referring to something else?
There was a problem hiding this comment.
Yes. I'll provide additional context in the document as well.
is_authorized is crucial for public accounts. Because public account ids can be "accessed" and used by anyone in a transaction. Private accounts cannot.
A private account can only be updated by the account owner (using the nsk). So, this specific change that I recommend is a protocol level change only for the branching logic for Private::Unauthorized. This is the case in which a third-party uses a published npk, vpk and some identifier (of their choosing) to initialize a new account for the account owner. Since this is a newly initialized account (and cannot be updated without the nsk), I see no reason why at the protocol level we cannot just set is_authorized = true. The ramifications of allowing is_authorized = true only extends to a newly initialized account which a user can choose to use or not use, but cannot impact any of their pre-existing accounts.
| 3. Bob **unseals** it with his own sealing secret key, then | ||
| independently re-derives the account's keys from the same seed. | ||
|
|
||
| This ensures that any member of the group can execute programs on shared accounts using either `PrivateAuthorizedInit` or `PrivateAuthorizedUpdate`. From a program's perspective, shared accounts should behave the same as regular public accounts. |
There was a problem hiding this comment.
From a program's perspective, shared accounts should behave the same as regular public accounts
Not regular private accounts?
| | SwapExactInput | `amm_swap_a_to_b_private_authorized_init_destination_is_not_expressible` | REGULAR, CHAIN | Swap paying out to a brand-new `PrivateAuthorizedInit` destination (owner self-initializes with its own `nsk`) | ❌ (confirmed not-expressible — same "destination must already exist" precondition as `RemoveLiquidity`) | | ||
| | NewDefinition | `amm_new_definition_private_initial_lp_holder` | REGULAR | Pool creation with a private `PrivateAuthorizedInit` initial LP holder | ✅ | | ||
| | NewDefinition | `amm_new_definition_private_unauthorized_lp_holder_is_not_expressible` | EXIST, REGULAR | Pool creation with a `PrivateUnauthorized` initial LP holder (`npk` only, no `nsk`) | ❌ (confirmed not-expressible — guest ABI requires `user_holding_lp` to be a signer, which `PrivateUnauthorized` can never satisfy; same shape as the `Swap` `PrivateUnauthorized` finding above) | | ||
|
|
There was a problem hiding this comment.
The confirmed not-expressible cases are okay right? Can you confirm?
There was a problem hiding this comment.
not-expressible are cases that would be ideal to allow for. But, with the current architecture are blocked.
For is_authorized = false for Private::Unauthorized (as mentioned in the next comment), this is a LEE protocol fix.
For amm_swap_a_to_b_private_authorized_init_destination_is_not_expressible, this can be fixed by removing the constraint that destination account is not default. This is a program fix.
There was a problem hiding this comment.
I made the tables clearer.
|
|
||
| ### Remarks | ||
| - `Swap` and `Remove` rejects any uninitialized destination account; this is a AMM design choice, and not Token program requirement. | ||
| - AMM tests were initially blocked by a bug. |
There was a problem hiding this comment.
Disclaimer: the bug was found while working with lez-programs but it's a bug in LEE itself (opened a PR to patch). I just wanted to make that clear.
For public transactions, we require that inputs accounts == output accounts; these sets are defined within the function's execution. However, there's nothing that explicitly checked to make sure the returned input accounts match with the function's parameters. E.g., we can quietly drop (or add) an input account to what the program returns.
This is blocked explicitly by privacy transactions. So, while I was writing tests...I kept getting this issue seemingly "blocking" me.
There was a problem hiding this comment.
I've added more details to the document for this.
| # Conclusions | ||
|
|
||
| Privacy coverage for LEZ program tests is greatly improved from the added tests. Though, there are a few noticable gaps: | ||
| - `PrivateUnauthorized` accounts can be blocked by programs with a check `is_authorized = true`. However, this issue can be avoided by defining `is_authorized = true` for account initialization with `PrivateUnauthorized` (e.g., no knowledge of `npk`). Account initialization cannot be used to maliciously alter a pre-existing account, and thus `is_authorized = true` would not offer any malicious path forward for the third-party initializing the account. |
There was a problem hiding this comment.
Can you elaborate what you're suggesting here? is_authorized is typically set by the LEZ environment. We don't control that unless its a PDA.
There was a problem hiding this comment.
Absolutely, I'm suggesting that at the LEE protocol level...we set is_authorized = true for PrivateUnauthorized accounts. These accounts are initialized by a third-party for the user. So, (imo) there's no good reason for the protocol level to block program behavior. Especially since the third-party can only initialize an account, they cannot update a pre-existing account.
|
|
||
| Privacy coverage for LEZ program tests is greatly improved from the added tests. Though, there are a few noticable gaps: | ||
| - `PrivateUnauthorized` accounts can be blocked by programs with a check `is_authorized = true`. However, this issue can be avoided by defining `is_authorized = true` for account initialization with `PrivateUnauthorized` (e.g., no knowledge of `npk`). Account initialization cannot be used to maliciously alter a pre-existing account, and thus `is_authorized = true` would not offer any malicious path forward for the third-party initializing the account. | ||
| - Privacy transactions have issues with chain calls in which multiple calls affect the same private account. This issue can be mitigated by adopting account diff paradigm instead of the current "account state replacement" that we currently use. |
There was a problem hiding this comment.
Any example of that? Do we have cases where this applies in our programs?
There was a problem hiding this comment.
Covering both issues I see listed:
-
PrivateUnauthorizedseveral functions have this issue, but a non issue. (PR 621 handles this issue, and will be merged today. I'll update the tests accordingly). -
Privacy transactions have issues with chain calls. This was actually an interesting issue that appeared in a single function (Stablecoin's
OpenPosition). What happens is initialization and token transfer occurs within the same chain call. The public version allows for this but it is blocked by the privacy version (due to the privacy preserving circuit). As mentioned, we have a proposed revision to the privacy preserving circuit that would fix this issue altogether.
IMO, I don't think devs should be (artificially) constrained by LEZ for "good" coding practices. E.g., initialize Token account and transfer to the same account seems to be reasonable behavior for a dev to try; even if strange to me since token transfer initializes. Instead, I would rather a protocol fix that allows this reasonable behavior to work in both public and privacy transfers.
There was a problem hiding this comment.
PR 621 is merged to logos-execution-zone 's dev branch; the dev branch is the active branch for new developments and work. main is the current build of logos.
I've added an Action section to specify whether a change should be handled be Zones or Programs.
|
…-matrix.md Address 0x-r4bbit's review comments on PR 215: expand the is_authorized reasoning, explain the AMM account-drop bug and its soundness implication, tag every not-expressible result with whether it's resolved (PR #621/#625) or still open, and add an Action items table attributing each open item to Zones (logos-execution-zone) or Programs (lez-programs). privacy-test-matrix.md was the AI-generated working scaffold behind findings.md; its load-bearing content has been folded into findings.md, so it's removed rather than left to drift out of sync.
Address 0x-r4bbit's review comments on PR 215: expand the is_authorized reasoning, explain the AMM account-drop bug and its soundness implication, tag every not-expressible result with whether it's resolved (PR #621/#625) or still open, and add an Action items table attributing each open item to Zones (logos-execution-zone) or Programs (lez-programs).
🎯 Purpose
Explore privacy claims of LEZ against LEZ programs. Add tests to demonstrate these capabilities, and explore where the features are not expressible.
⚙️ Approach
For each LEZ program (token, amm, stablecoin, ata): we add privacy tests to explore functionality against REGULAR private accounts (initialized and uninitialized accounts using
nsk), EXIST (private account initialization usingnpk), and GROUP (private account shared among a group).Private PDAs were not testable with these programs due to the
AccountIdformulas differing from the public variant's formula.docs/findings.mdgives an overview of the findings and tests.docs/privacy-test-matrix.mdis an AI (Claude) generated document that provides more context generated (has not been edited).🧪 How to Test
RISC0_DEV_MODE=1 cargo test🔗 Dependencies
N/A
🔜 Future Work
findings.mdoutlines minor issues/fixes that can be applied to LEZ, and a bug inspel-framework. Git issues for each of these will be opened.📋 PR Completion Checklist
Mark only completed items. A complete PR should have all boxes ticked.