feat(aztec-nr): V2 handshake registry for non interactive constrained delivery#23278
Conversation
| env.call_private(sender, registry.validate_handshake(sender, recipient, zero_padded_secret)); | ||
| } | ||
|
|
||
| // Regression for the unfiltered-page bug: insert 16 unrelated handshakes (different recipients) before |
There was a problem hiding this comment.
This and the get_app_siloed_secret test variant are regressions for https://linear.app/aztec-labs/issue/F-666/get-notes-and-view-notes-make-it-easy-to-filter-after-pagination. I originally did not use .select on the NoteGetterOptions and filtered the result of get_notes in the utility function which caused these tests to fail. I decided to leave these tests in for now as protection. But I felt aztec-nr should make it impossible for devs to run into such a foot-gun.
nchamo
left a comment
There was a problem hiding this comment.
Small initial review, will continue but wanted to report early findings
…eNote, Context>, Context>, Context> and additional test
nchamo
left a comment
There was a problem hiding this comment.
Ok, I need to go, but I wanted to share all my thoughts so far
| /// polluting registry state. | ||
| #[external("private")] | ||
| fn non_interactive_handshake(sender: AztecAddress, recipient: AztecAddress) { | ||
| fn non_interactive_handshake(sender: AztecAddress, recipient: AztecAddress) -> Field { |
There was a problem hiding this comment.
Ok, I went through the rabbit hole. I was concerned that an attacker could front-run a handshake, which would cause our transaction to fail (it would fail since a both txs would emit the initialization nullifier). But in PrivateMutable, the initialization nullifier requires the owner's nullifier hiding key, which an attacker won't have. So you can only specify a sender that you have the keys for, which is exactly what we want: flexibility + security
On the other side, with our previous storage, anyone could have called this, since PrivateSet has no check like that. So iterating through all of them would have been very expensive
Not sure if this is 100% correct, since I'm very tired 😅 . So could we add a check to verify? I would like to test that:
- An account can't set the handshake for another sender
- An account can set the handshake for another sender, if the sender is added as additional scopes
There was a problem hiding this comment.
Added non_interactive_handshake_rejects_other_sender and non_interactive_handshake_accepts_other_sender_in_additional_scopes.
| /// The raw ECDH shared-secret point `S = eph_sk * recipient_address_point`. Only this module can read it for | ||
| /// siloing, and it is never returned by an external function. |
There was a problem hiding this comment.
Only this module can read it for siloing, and it is never returned by an external function.
Nice, really like this approach!
…hake_registry_contract/src/main.nr Co-authored-by: Nicolas Chamo <nicolas@chamo.com.ar>
nchamo
left a comment
There was a problem hiding this comment.
Great work, we are almost there!
| /// Returning the latest (rather than the first) lets a sender re-initiate by issuing a fresh handshake; e.g. | ||
| /// if the previous secret was leaked. | ||
| /// Apps that receive an `app_siloed_secret` from an untrusted source (e.g. a PXE oracle) call this once to | ||
| /// bind that secret to the registry's stored handshake. Subsequent uses can rely on the prior-nullifier chain |
There was a problem hiding this comment.
"bind" feels off. I think we should stick to "validate" (and maybe rephrase the rest of the sentence with it)
| /// Returning the latest (rather than the first) lets a sender re-initiate by issuing a fresh handshake; e.g. | ||
| /// if the previous secret was leaked. | ||
| /// Apps that receive an `app_siloed_secret` from an untrusted source (e.g. a PXE oracle) call this once to | ||
| /// bind that secret to the registry's stored handshake. Subsequent uses can rely on the prior-nullifier chain |
There was a problem hiding this comment.
I would remove the entire "Subsequent uses can rely on the prior-nullifier chain..." text. The registry doesn't know about nullifier chains, nor should it. It just stores handshakes, and validates secrets
Co-authored-by: Nicolas Chamo <nicolas@chamo.com.ar>
nchamo
left a comment
There was a problem hiding this comment.
Great work!
I'm really happy with our approach, I think this is going to be a really cool feature with minimal changes needed 🎉
Flakey Tests🤖 says: This CI run detected 1 tests that failed, but were tolerated due to a .test_patterns.yml entry. |
BEGIN_COMMIT_OVERRIDE fix(aztec-up): fall back to no timeout when /usr/bin/timeout absent (macOS) (AztecProtocol#23310) chore: reduce compat e2e timeout (AztecProtocol#23318) feat(aztec-nr): V2 handshake registry for non interactive constrained delivery (AztecProtocol#23278) chore(aztec-nr): Public internal/utility methods self constructor (AztecProtocol#23115) END_COMMIT_OVERRIDE
Fixes https://linear.app/aztec-labs/issue/F-586/handshake-registry-non-interactive-handshake-function (updates work from #22854)
Updated the handshake registry following the new spec in https://www.notion.so/aztecnetwork/Plan-Onchain-constrained-delivery-34fa1f6b0e358063b64ecc25b768c359
msg_senderand returns it from the both the handshake and its utility method for fetching the siloed secret.I decided to just update the handshake registry directly rather than duplicating it with the old one. I felt if we ever wanted to go back to the old spec, we have the git history which we can reference for the old contract.
Note packing bug was revealed with this work: https://linear.app/aztec-labs/issue/F-665/note-properties-generates-incorrect-selectors-for-custom-packed-fields
I attempted to fitler notes after calling
get_noteswhich through testing revealed itself as the incorrect way to filter notes. This should be made easier as it is a foot-gun for developers: https://linear.app/aztec-labs/issue/F-666/get-notes-and-view-notes-make-it-easy-to-filter-after-pagination