feat(shielded-pool)!: publish an OVK blob per private transfer, and tag the pool per nullifier - #129
Closed
nol4lej wants to merge 4 commits into
Closed
feat(shielded-pool)!: publish an OVK blob per private transfer, and tag the pool per nullifier#129nol4lej wants to merge 4 commits into
nol4lej wants to merge 4 commits into
Conversation
…nchmark shielded-pool and tag the pool per nullifier
… the relay fallback to the decoder
…he proof cost in the slope
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
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.
Summary
Adds outgoing viewing keys (OVK) to
private_transfer, so a sender who loses their vault can recover what they sent. Along the way this fixes two security issues found while auditing the change — one of them in code that predates it.Ships as spec 9 / transaction_version 3. Breaking in both directions: wallet and runtime must be deployed together.
Why OVK
A note's memo is sealed toward the recipient, so its own author cannot reopen it. Restore a wallet from its seed and the incoming notes come back — but the record of what you sent does not, because it never existed anywhere except the lost device.
Each
private_transfernow publishes a 56-byte blob wrapping that memo's shared secret under the sender'sovk:Unwrapping yields the same
sharedSecretthe recipient derives by ECDH, feeding the same decrypt-and-verify routine — one note-decryption path in the system, not two. The sender recovers the exact amount and the recipient's one-time key, including for exact-amount transfers that leave no change note and were previously unrecoverable.The
ovkderives from the master seed as a sibling of the incoming viewing key, not from it, so the two capabilities stay separately delegable.The chain validates only the length. It holds no key, so any 56 bytes are accepted — zeros included. A sender opting out publishes 56 random bytes: zeros would be greppable, permanently marking the opt-out and making everyone who chose it a trivially identifiable set.
The call emits
OutgoingBlobPublished { commitment, blob }, bound tocommitments[0]. It is a separate event becauseCommitmentsInsertedis shared withshield,shield_batchandunshield, none of which carry a blob.Security fixes
1. Mempool tags did not enforce mutual exclusion
and_providescontributes exactly one tag, so passing it aVecencoded the whole nullifier set plus the relayer into a single blob. Three consequences, all free to exploit since the fee is only charged on execution:A+BandA+C) did not collide at all, letting one note back unboundedly many entries;private_transferandunshieldused different tag prefixes, so the same note could back one of each simultaneously.Every variant propagates and is revalidated network-wide while at most one can ever execute.
Fix: one tag per nullifier under a shared
ShieldedPoolSpendprefix.relayerdeliberately leaves the tag — binding it made a fee-recipient-swapped copy a separate entry, so anyone could rebroadcast another user's spend pointed at their own account. Keyed on the nullifier the two are mutually exclusive, so taking the fee requires out-bidding, which means paying it.2. Relay fallback selectors were stale (ME-8)
The runtime's fallback selector list — used when the Runtime API call fails — held
0x47fc44a2and0x8c0f5d24. Derived by keccak, these turn out to be real selectors from signatures two versions old, predatingcircuit_version. Falling back would have rejected every relay call as "unsupported selector", silently, because that is indistinguishable from a legitimate rejection.The same stale literals were hardcoded in
ts-tests/tests/test-relay-rpc.ts, so the tests stayed green while testing nothing.Fix: both now derive from
pallet_evm_precompile_shielded_pool::selectors::{UNSHIELD, PRIVATE_TRANSFER}/ the ABI signature, so they cannot drift again.Related:
source_pk— the chain's side of a leak fixed in the SDKA separate privacy leak surfaced during this work: the recipient's memo carried the spent note's
owner_pkunconditionally, which for a shield note is the sender's permanent key. That fix lives in the SDK, since the memo is built there — the chain treats it as opaque ciphertext and holds no key to inspect it.What belongs in this PR is proving the chain stays out of it:
attack_decoder_is_blind_to_the_sourcepk_region_of_the_memoasserts that two calls differing only in those bytes are treated identically. A decoder that could tell them apart would mean the field was not actually encrypted.Breaking changes
private_transferovk_blob: OvkBlobprivateTransferselector0x66ed2cd40x1ec439cftransaction_versionspec_versionExtrinsics encoded for tx 2 no longer decode; callers on the old selector are rejected as unsupported. No storage migration —
ovk_blobis additive and read only at dispatch.Weights
shielded-poolwas marked STALE since OVK landed without re-measuring. Re-benchmarked: +1.9% mean, +9% worst case. The marginal cost per output moved 910M → 989M ps and stays parameterised by output count, soprivate_transfer_weight_scales_with_outputsstill guards against under-pricing the second leaf insert.Testing
pallet-shielded-poolpallet-evm-precompile-shielded-poolfc-rpc(relay)cargo test --release --lib --allcargo fmt --all,taplo fmt --check,clippy --release --all-targetswithruntime-benchmarks,skip-proof-verification,try-runtime -D warningsAdversarial batteries were added rather than only happy-path tests — 27 attacks across the pallet, ABI decoder and relay RPC, plus 45 000 fuzz rounds over the unauthenticated relay surface and the calldata decoder. These cover double-spend within one extrinsic, non-canonical field elements, forged Merkle roots, blob transplantation onto another note, mempool tag collisions, and tree-rollover invariants.
Verified end-to-end against a live dev node and indexer:
e2e-ovk(15 checks) ande2e-provenance(32 checks), covering shield → transfer with a real Groth16 proof, recipient scan from the indexer, sender-side OVK recovery, payment-slip regeneration, and selective disclosure.Also in this PR
shield,private_transfer,unshield,claim_shielded_fees) now carry numbered steps matched to their ABI-layout tables, plus namedHEAD_SIZEconstants and explicit notes on why certain values come from the caller rather than calldata.counterparty_pk→source_pk. The old name asserted something false — it does not identify the sender. Renamed in comments and docs across the node; the unusedorbinum-encrypted-memocrate keeps the old spelling. Byte offsets are unchanged.primitives/encrypted-memo/README.mddeclared a 116-byte plaintext / 176-byte wire format, missingcircuit_version— actually 120 / 180.