Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: making private refunds smooth-brain friendly #7343

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions noir-projects/aztec-nr/aztec/src/note/utils.nr
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,18 @@ use dep::protocol_types::{
utils::arr_copy_slice
};

pub fn compute_inner_note_hash_from_preimage(storage_slot: Field, note_content_hash: Field) -> Field {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name here is slightly odd to me as it is always a pre-image, but here I would think it would be the note preimage 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how else to call this but I am open to suggestions 😜

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe silo_note_content_hash_to_storage_slot?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't tell you that the return value is what we call inner_note_hash in our terminology and I think it's important to emphasize these terms to not get lost in all the hashing.

pedersen_hash(
[storage_slot, note_content_hash],
GENERATOR_INDEX__INNER_NOTE_HASH
)
}

fn compute_inner_note_hash<Note, N, M>(note: Note) -> Field where Note: NoteInterface<N, M> {
let header = note.get_header();
let note_hash = note.compute_note_content_hash();

pedersen_hash(
[header.storage_slot, note_hash],
GENERATOR_INDEX__INNER_NOTE_HASH
)
compute_inner_note_hash_from_preimage(header.storage_slot, note_hash)
}

pub fn compute_siloed_nullifier<Note, N, M>(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,13 @@ use dep::aztec::oracle::logs::emit_unencrypted_log_private_internal;
use dep::aztec::hash::compute_unencrypted_log_hash;
use dep::aztec::context::PrivateContext;

fn emit_nonce_as_unencrypted_log(context: &mut PrivateContext, nonce: Field) {
fn emit_randomness_as_unencrypted_log(context: &mut PrivateContext, randomness: Field) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👁️ 🥄

Copy link
Contributor Author

@benesjan benesjan Jul 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a consequence of the function being nuked from private context in your PR 😬 Not really sure what to do about this. Maybe we could move the original function to a danger.nr file in aztec-nr?

let counter = context.next_counter();
let log_slice = nonce.to_be_bytes_arr();
let log_hash = compute_unencrypted_log_hash(context.this_address(), nonce);
let log_slice = randomness.to_be_bytes_arr();
let log_hash = compute_unencrypted_log_hash(context.this_address(), randomness);
// 40 = addr (32) + raw log len (4) + processed log len (4)
let len = 40 + log_slice.len().to_field();
let side_effect = LogHash { value: log_hash, counter, length: len };
context.unencrypted_logs_hashes.push(side_effect);
let _void = emit_unencrypted_log_private_internal(context.this_address(), nonce, counter);
let _void = emit_unencrypted_log_private_internal(context.this_address(), randomness, counter);
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ contract PrivateFPC {
use dep::aztec::protocol_types::{abis::log_hash::LogHash, address::AztecAddress};
use dep::aztec::state_vars::SharedImmutable;
use dep::private_token::PrivateToken;
use crate::lib::emit_nonce_as_unencrypted_log;
use crate::lib::emit_randomness_as_unencrypted_log;

#[aztec(storage)]
struct Storage {
Expand All @@ -20,19 +20,19 @@ contract PrivateFPC {
}

#[aztec(private)]
fn fund_transaction_privately(amount: Field, asset: AztecAddress, nonce: Field) {
fn fund_transaction_privately(amount: Field, asset: AztecAddress, randomness: Field) {
assert(asset == storage.other_asset.read_private());
// convince the FPC we are not cheating
context.push_nullifier(nonce, 0);
context.push_nullifier(randomness, 0);

// allow the FPC to reconstruct their fee note
emit_nonce_as_unencrypted_log(&mut context, nonce);
emit_randomness_as_unencrypted_log(&mut context, randomness);
benesjan marked this conversation as resolved.
Show resolved Hide resolved

PrivateToken::at(asset).setup_refund(
storage.admin_npk_m_hash.read_private(),
context.msg_sender(),
amount,
nonce
randomness
).call(&mut context);
context.set_as_fee_payer();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,9 @@ mod test;
contract PrivateToken {
use dep::compressed_string::FieldCompressedString;
use dep::aztec::{
hash::compute_secret_hash,
note::utils::compute_inner_note_hash_from_preimage, hash::compute_secret_hash,
prelude::{NoteGetterOptions, Map, PublicMutable, SharedImmutable, PrivateSet, AztecAddress},
protocol_types::{
abis::function_selector::FunctionSelector, hash::pedersen_hash,
constants::GENERATOR_INDEX__INNER_NOTE_HASH
},
protocol_types::{abis::function_selector::FunctionSelector, hash::pedersen_hash},
oracle::unsafe_rand::unsafe_rand,
encrypted_logs::encrypted_note_emission::{encode_and_encrypt_note, encode_and_encrypt_note_with_keys}
};
Expand Down Expand Up @@ -160,57 +157,69 @@ contract PrivateToken {

#[aztec(private)]
fn setup_refund(
fee_payer_npk_m_hash: Field,
sponsored_user: AztecAddress,
funded_amount: Field,
refund_nonce: Field
fee_payer_npk_m_hash: Field, // NpkMHash of the entity which will receive the fee note.
user: AztecAddress, // A user for which we are setting up the fee refund.
funded_amount: Field, // The amount the user funded the fee payer with (represents fee limit).
randomness: Field // A randomness to mix in with the generated notes.
) {
assert_current_call_valid_authwit(&mut context, sponsored_user);
// 1. This function is called by fee paying contract (fee_payer) when setting up a refund so we need to support
// the authwit flow here and check that the user really permitted fee_payer to set up a refund on their behalf.
assert_current_call_valid_authwit(&mut context, user);

// 2. Get all the relevant user keys
let header = context.get_header();
let sponsored_user_npk_m_hash = header.get_npk_m_hash(&mut context, sponsored_user);
let sponsored_user_ovpk = header.get_ovpk_m(&mut context, sponsored_user);
let sponsored_user_ivpk = header.get_ivpk_m(&mut context, sponsored_user);
storage.balances.sub(sponsored_user_npk_m_hash, U128::from_integer(funded_amount)).emit(encode_and_encrypt_note_with_keys(&mut context, sponsored_user_ovpk, sponsored_user_ivpk));
let points = TokenNote::generate_refund_points(
let user_npk_m_hash = header.get_npk_m_hash(&mut context, user);
let user_ovpk = header.get_ovpk_m(&mut context, user);
let user_ivpk = header.get_ivpk_m(&mut context, user);

// 3. Deduct the funded amount from the user's balance - this is a maximum fee a user is willing to pay
// (called fee limit in aztec spec). The difference between fee limit and the actual tx fee will be refunded
// to the user in the `complete_refund(...)` function.
// TODO(#7324): using npk_m_hash here does not work with key rotation
benesjan marked this conversation as resolved.
Show resolved Hide resolved
storage.balances.sub(user_npk_m_hash, U128::from_integer(funded_amount)).emit(encode_and_encrypt_note_with_keys(&mut context, user_ovpk, user_ivpk));

// 4. We generate the refund points.
let (fee_payer_point, user_point) = TokenNote::generate_refund_points(
fee_payer_npk_m_hash,
sponsored_user_npk_m_hash,
user_npk_m_hash,
funded_amount,
refund_nonce
randomness
);

// 5. Set the public teardown function to `complete_refund(...)`. Public teardown is the only time when a public
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Me being slightly ignorant around fees. Is anything stopping me from calling set_public_teardown_function multiple times? Is there a point wherein it must have been set? @just-mitch.

Namely here thinking about the case where it is updated in a later call 👀 Would the complete_refund then simply not be executed?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, there is a check in noir-projects/noir-protocol-circuits/crates/private-kernel-lib/src/components/private_kernel_circuit_output_validator.nr that ensures you cannot overwrite the teardown function; attempts to do so will cause the kernel to fail.

// function has access to the final transaction fee, which is needed to compute the actual refund amount.
context.set_public_teardown_function(
context.this_address(),
FunctionSelector::from_signature("complete_refund(Field,Field,Field,Field)"),
[points[0].x, points[0].y, points[1].x, points[1].y]
[fee_payer_point.x, fee_payer_point.y, user_point.x, user_point.y]
);
}

#[aztec(public)]
#[aztec(internal)]
fn complete_refund(
fpc_point_x: Field,
fpc_point_y: Field,
fee_payer_point_x: Field,
benesjan marked this conversation as resolved.
Show resolved Hide resolved
fee_payer_point_y: Field,
user_point_x: Field,
user_point_y: Field
) {
let fpc_point = EmbeddedCurvePoint { x: fpc_point_x, y: fpc_point_y, is_infinite: false };
// 1. We get the final note content hashes by calling the `complete_refund` on the note.
let fee_payer_point = EmbeddedCurvePoint { x: fee_payer_point_x, y: fee_payer_point_y, is_infinite: false };
let user_point = EmbeddedCurvePoint { x: user_point_x, y: user_point_y, is_infinite: false };
let tx_fee = context.transaction_fee();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More ignorance from my side. But when computing the fee at this point, you will not include the insertions that happen later here or any of the compute. Does the FPC end up paying for those and then the user underpays here, or 👀? @just-mitch

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The teardown phase is special in that the user pre-pays for the gas consumed. So the user specifies:

  • DA/L2 gas limit
  • teardown DA/L2 gas limit (which is a subset of the above)

Then the AVM starts teardown with the teardown limits set as start gas. So really the FPC needs to be checking in setup that the amount specified for teardown is sufficient to make sure it doesn't run OOG, but regardless context.transaction_fee() is always exclusive of what is actually done in teardown, as long as we don't run out of gas.

So yes, in effect, the FPC could overpay if the teardown limit is too high, but they ought to be charging the user for that overpayment.

let note_hashes = TokenNote::complete_refund(fpc_point, user_point, tx_fee);

// `compute_inner_note_hash` manually, without constructing the note
// `3` is the storage slot of the balances
context.push_note_hash(
pedersen_hash(
[PrivateToken::storage().balances.slot, note_hashes[0]],
GENERATOR_INDEX__INNER_NOTE_HASH
)
);
context.push_note_hash(
pedersen_hash(
[PrivateToken::storage().balances.slot, note_hashes[1]],
GENERATOR_INDEX__INNER_NOTE_HASH
)
let (fee_payer_note_content_hash, user_note_content_hash) = TokenNote::complete_refund(fee_payer_point, user_point, tx_fee);

// 2. Now we "manually" compute the inner note hashes.
let fee_payer_inner_note_hash = compute_inner_note_hash_from_preimage(
PrivateToken::storage().balances.slot,
fee_payer_note_content_hash
);
let user_inner_note_hash = compute_inner_note_hash_from_preimage(PrivateToken::storage().balances.slot, user_note_content_hash);

// 3. At last we emit the note hashes.
context.push_note_hash(fee_payer_inner_note_hash);
context.push_note_hash(user_inner_note_hash);
// --> Once the tx is settled user and fee recipient can add the notes to their pixies.
}

/// Internal ///
Expand Down
Loading
Loading