feat(aztec-nr): Helper to validate the app-siloed secret and index for constrained tagging#23569
Conversation
…eNote, Context>, Context>, Context> and additional test
…hake_registry_contract/src/main.nr Co-authored-by: Nicolas Chamo <nicolas@chamo.com.ar>
… dom sep
The new DOM_SEP__CONSTRAINED_MSG_LOG_TAG domain separator added in this PR
adds one assert_dom_sep_matches_derived call, which pushes to both seen_u32
and seen_values. constants_tests::hashed_values_match_derived sized the
tester at <71, 64>, so the extra push overflowed the BoundedVec
("push out of bounds" at bounded_vec.nr:186). Bump to <72, 65> to match the
actual 72 value / 65 u32 hashed entries.
…next_app_tag_as_sender-for-constrained' into maxim/f-668-aztec-nr-extend-get_next_app_tag_as_sender-for-constrained
…for-constrained' into maxim/f-669-aztec-nr-calculate_secret_and_index-helper-for-constrained
…ecret_and_index-helper-for-constrained' into maxim/f-669-aztec-nr-calculate_secret_and_index-helper-for-constrained
…e_secret_and_index-helper-for-constrained
| // the `Some` branch. Without this, a later constrained message under the same secret restarts at index 0 | ||
| // and collides on `(secret, 0)`. | ||
| // Safety: this only advances a PXE-side counter; the returned index is constrained to 0 below. | ||
| let index = unsafe { get_next_constrained_tagging_index(secret) }; |
There was a problem hiding this comment.
Without this call the new bootstrap_seeds_index_counter_for_same_tx_reuse test will fail. We could do this inside of the handshake function as well.
…e_secret_and_index-helper-for-constrained
| @@ -0,0 +1,115 @@ | |||
| pub mod constrained_delivery; | |||
There was a problem hiding this comment.
I don't quite follow why you moved the file here in this PR and also dropped all the docs. Did you change something else? I don't quite see it
I'm ok with moving the file, but maybe we could have a follow up PR where we move the file and directly avoid the compatibility wrapper
| /// Resolves the app-siloed shared secret and next per-secret index for a constrained send. | ||
| /// | ||
| /// Wraps the registry calls needed by every constrained-delivery app: query the handshake registry for an | ||
| /// existing app-siloed secret, bootstrap a fresh handshake if there isn't one, and constrain the |
There was a problem hiding this comment.
| /// existing app-siloed secret, bootstrap a fresh handshake if there isn't one, and constrain the | |
| /// existing app-siloed secret, bootstrap a fresh handshake if there isn't one, and constrain the |
| // Safety: the returned `Option<Field>` is untrusted and is constrained below before being returned to the | ||
| // caller, either by performing a new handshake or by constraining the prior handshake's existence. | ||
| let maybe_secret: Option<Field> = unsafe { | ||
| let selector = comptime { |
There was a problem hiding this comment.
Can we somehow avoid the selector and to something like Registry::at(address).... Like we do for tokens. I'm concerned about a possible change in the function signature that would drift from this
| // Reserve index 0 for the freshly bootstrapped secret so the PXE seeds its per-secret counter, mirroring | ||
| // the `Some` branch. Without this, a later constrained message under the same secret restarts at index 0 | ||
| // and collides on `(secret, 0)`. | ||
| // Safety: this only advances a PXE-side counter; the returned index is constrained to 0 below. |
There was a problem hiding this comment.
This feels off. Calling the oracle so that the PXE counter can be started? Shouldn't the oracle fetch for logs when starting and start at the correct index? What if the messages started in another PXE, and the index should be 2 when get_next_constrained_tagging_index is first called, would be handle that correctly?
| let prev_nullifier = poseidon2_hash_with_separator( | ||
| [sender.to_field(), recipient.to_field(), secret, (index - 1) as Field], | ||
| DOM_SEP__CONSTRAINED_MSG_NULLIFIER, | ||
| ); |
There was a problem hiding this comment.
We could probably have a helper function in this file for this, since we'll need to re-use it when we actually send a message
| /// | ||
| /// Wraps the registry calls needed by every constrained-delivery app: query the handshake registry for an | ||
| /// existing app-siloed secret, bootstrap a fresh handshake if there isn't one, and constrain the | ||
| /// oracle-supplied secret. The returned `(secret, index)` pair is the input for the caller's tag derivation |
There was a problem hiding this comment.
Shouldn't this be an index?
| /// oracle-supplied secret. The returned `(secret, index)` pair is the input for the caller's tag derivation | |
| /// oracle-supplied tagging index. The returned `(secret, index)` pair is the input for the caller's tag derivation |
| /// | ||
| /// ## Implementation Details | ||
| /// 1. Calls `HandshakeRegistry::get_app_siloed_secret` offchain (untrusted hint). | ||
| /// 2. On `None`, calls `HandshakeRegistry::non_interactive_handshake(sender, recipient)` to bootstrap and | ||
| /// returns `(secret, 0)`. The constrained return value of the private call is the source of truth for the | ||
| /// secret. | ||
| /// 3. On `Some(secret)`, reads the per-secret index hint via [`get_next_constrained_tagging_index`] (also untrusted), | ||
| /// then constrains: | ||
| /// - `index == 0`: calls `HandshakeRegistry::validate_handshake` to bind the secret to the registry's | ||
| /// current note. `PrivateMutable::get_note` inside `validate_handshake` nullifies the current handshake | ||
| /// note and emits a fresh one, so this branch already produces a per-call nullifier on the registry | ||
| /// note. | ||
| /// - `index > 0`: asserts the prior nullifier | ||
| /// `poseidon2_hash_with_separator([sender, recipient, secret, index - 1], DOM_SEP__CONSTRAINED_MSG_NULLIFIER)` | ||
| /// exists via [`compute_nullifier_existence_request`](crate::nullifier::utils::compute_nullifier_existence_request). | ||
| /// The chain transitively constrains back to the index-0 `validate_handshake`. The caller is expected | ||
| /// to emit the matching nullifier alongside each constrained send so subsequent calls can prove the | ||
| /// chain. |
There was a problem hiding this comment.
I would skip this. I think that either the code is clear enough or, if not, we should make it clearer.
| /// | |
| /// ## Implementation Details | |
| /// 1. Calls `HandshakeRegistry::get_app_siloed_secret` offchain (untrusted hint). | |
| /// 2. On `None`, calls `HandshakeRegistry::non_interactive_handshake(sender, recipient)` to bootstrap and | |
| /// returns `(secret, 0)`. The constrained return value of the private call is the source of truth for the | |
| /// secret. | |
| /// 3. On `Some(secret)`, reads the per-secret index hint via [`get_next_constrained_tagging_index`] (also untrusted), | |
| /// then constrains: | |
| /// - `index == 0`: calls `HandshakeRegistry::validate_handshake` to bind the secret to the registry's | |
| /// current note. `PrivateMutable::get_note` inside `validate_handshake` nullifies the current handshake | |
| /// note and emits a fresh one, so this branch already produces a per-call nullifier on the registry | |
| /// note. | |
| /// - `index > 0`: asserts the prior nullifier | |
| /// `poseidon2_hash_with_separator([sender, recipient, secret, index - 1], DOM_SEP__CONSTRAINED_MSG_NULLIFIER)` | |
| /// exists via [`compute_nullifier_existence_request`](crate::nullifier::utils::compute_nullifier_existence_request). | |
| /// The chain transitively constrains back to the index-0 `validate_handshake`. The caller is expected | |
| /// to emit the matching nullifier alongside each constrained send so subsequent calls can prove the | |
| /// chain. |
| // Safety: the returned `Option<Field>` is untrusted and is constrained below before being returned to the | ||
| // caller, either by performing a new handshake or by constraining the prior handshake's existence. |
There was a problem hiding this comment.
What about something like this?
// Safety: when the utility call returns no secret, we attempt to create a fresh handshake. Creating
// a duplicate fails, so a forged empty response cannot hide an existing handshake. When the utility
// call returns a secret, it is validated against the registry's stored handshake.
| traits::{Deserialize, ToField}, | ||
| }; | ||
|
|
||
| /// Resolves the app-siloed shared secret and next per-secret index for a constrained send. |
There was a problem hiding this comment.
Warning, boss-man likes the first line to be 10 words max
| //! Tests for `calculate_secret_and_index`. | ||
| //! | ||
| //! TODO(F-670): Coverage here is limited to the bootstrap branch and the `Some(secret)` branch with `index == 0` (which | ||
| //! constrains via `HandshakeRegistry::validate_handshake`). The `index > 0` branch of the helper is reachable | ||
| //! only after the per-secret index has been advanced in the PXE, which requires a tx to land a private log | ||
| //! tagged with `compute_log_tag(poseidon2_hash([secret, index]), DOM_SEP__CONSTRAINED_MSG_LOG_TAG)` AND emit | ||
| //! the chained constrained-message nullifier. Both are produced by the forthcoming `emit_constrained_message` | ||
| //! helper, so positive and negative `index > 0` tests live with that helper. Add | ||
| //! `advances_index_above_zero_when_prior_message_was_emitted` and | ||
| //! `fails_at_index_above_zero_without_prior_nullifier` alongside `emit_constrained_message`. |
There was a problem hiding this comment.
Can't you mock the get_next_constrained_tagging_index oracle and test it here? Yes, for the full e2e version we would need more code and PXE, but we could test that branch by mocking the oracle, right?
|
Closing as we have fully migrated over to #23866 |
Fixes F-699
Stacks on #23359
noir-projects/aztec-nr/aztec/src/messages/delivery/(to matchmessages/discovery/)MessageDeliveryintonoir-projects/aztec-nr/aztec/src/messages/delivery/mod.nrcalculate_secret_and_indexhelper insidenoir-projects/aztec-nr/aztec/src/messages/delivery/constrained_delivery.nrnoir-projects/noir-contracts/contracts/test/constrained_delivery_test_contractwith test module. Some tests have been left for the F-670 follow-up.