Skip to content

refactor: TokenWithRefunds optimizations #7628

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.

Sign up for GitHub

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

Merged
benesjan merged 1 commit into from
Jul 31, 2024
+19 −44
Merged

refactor: TokenWithRefunds optimizations #7628

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
63 changes: 19 additions & 44 deletions noir-projects/noir-contracts/contracts/token_with_refunds_contract/src/types/token_note.nr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
use dep::aztec::{
generators::{Ga1 as G_amt, Ga2 as G_npk, Ga3 as G_rnd},
prelude::{AztecAddress, NoteHeader, NoteInterface, PrivateContext},
prelude::{NoteHeader, NoteInterface, PrivateContext},
protocol_types::{constants::GENERATOR_INDEX__NOTE_NULLIFIER, point::Point, scalar::Scalar, hash::poseidon2_hash},
note::utils::compute_note_hash_for_consumption, oracle::unsafe_rand::unsafe_rand,
keys::getters::get_nsk_app, note::note_getter_options::PropertySelector
};
use dep::std::field::bn254::decompose;
use dep::std::embedded_curve_ops::multi_scalar_mul;
use dep::std::{embedded_curve_ops::multi_scalar_mul, hash::from_field_unsafe};

trait OwnedNote {
fn new(amount: U128, owner_npk_m_hash: Field) -> Self;
Expand Down Expand Up @@ -72,8 +71,9 @@ impl NoteInterface<TOKEN_NOTE_LEN, TOKEN_NOTE_BYTES_LEN> for TokenNote {


fn compute_note_hiding_point(self) -> Point {
let (npk_lo, npk_hi) = decompose(self.npk_m_hash);
let (random_lo, random_hi) = decompose(self.randomness);
// We use the unsafe version because the multi_scalar_mul will constrain the scalars.
let npk_m_hash_scalar = from_field_unsafe(self.npk_m_hash);
let randomness_scalar = from_field_unsafe(self.randomness);
Comment on lines +75 to +76

@nventuro nventuro Jul 26, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you know why these two scalars are not constructed the same way as the amoutn scalar?

@nventuro nventuro Jul 26, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If they all were constructed the same way, then we could have a fn that takes some values and does msm over them generically, instead of having to inline it manually in every place and deal with the unsafe values

@benesjan benesjan Jul 29, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I would say this is because the npk_m_hash and randomness can span the whole Field so it does not fit only into the lower limb of a scalar. I think Mitch originally wrote this.

@just-mitch do you know if we are really guaranteed that the amount is smaller then lower limb of a grumpkin scalar? Is it the case that it would simply be too huge of an amount of token that you considered it unrealistic to occur?

If this would occur it would brick the contract for the user as MSM would try to constrain the limb and as a result it would fail to generate the proof. Given that we can use from_field_unsafe (which doesn't add constraints) here it's probably better to just call it for amount as well (what if we had token with too a lot of decimals?).

@nventuro nventuro Jul 29, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It'd be fine to constrain token balances to be uint128s - we'd just need to do that consistently. Many DeFi systems use even smaller amounts (e.g. Balancer uses 112 bits).

@nventuro nventuro Jul 29, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

At any rate this function would likely be incorporated into eg #7585

// We compute the note hiding point as `G_amt * amount + G_npk * npk_m_hash + G_rnd * randomness` instead
// of using pedersen or poseidon2 because it allows us to privately add and subtract from amount in public
// by leveraging homomorphism.
Expand All @@ -83,14 +83,8 @@ impl NoteInterface<TOKEN_NOTE_LEN, TOKEN_NOTE_BYTES_LEN> for TokenNote {
lo: self.amount.to_integer(),
hi: 0
},
Scalar {
lo: npk_lo,
hi: npk_hi
},
Scalar {
lo: random_lo,
hi: random_hi,
}]
npk_m_hash_scalar,
randomness_scalar]
)
}
}
Expand Down Expand Up @@ -208,45 +202,28 @@ impl PrivatelyRefundable for TokenNote {
fn generate_refund_points(fee_payer_npk_m_hash: Field, user_npk_m_hash: Field, funded_amount: Field, user_randomness: Field, fee_payer_randomness: Field) -> (Point, Point) {
// 1. To be able to multiply generators with randomness and npk_m_hash using barretneberg's (BB) blackbox
// function we first need to convert the fields to high and low limbs.
// TODO(#7606): replace decompose with from_field_unsafe
let (fee_payer_randomness_lo, fee_payer_randomness_hi) = decompose(fee_payer_randomness);
let (fee_payer_npk_m_hash_lo, fee_payer_npk_m_hash_hi) = decompose(fee_payer_npk_m_hash);
// We use the unsafe version because the multi_scalar_mul will constrain the scalars.
let fee_payer_randomness_scalar = from_field_unsafe(fee_payer_randomness);
let fee_payer_npk_m_hash_scalar = from_field_unsafe(fee_payer_npk_m_hash);

// 2. Now that we have correct representationsn of fee payer and randomness we can compute
// `G_npk * fee_payer_npk + G_rnd * randomness`.
let incomplete_fee_payer_point = multi_scalar_mul(
[G_npk, G_rnd],
[Scalar {
lo: fee_payer_npk_m_hash_lo,
hi: fee_payer_npk_m_hash_hi
},
Scalar {
lo: fee_payer_randomness_lo,
hi: fee_payer_randomness_hi
}]
[fee_payer_npk_m_hash_scalar, fee_payer_randomness_scalar]
);

// 3. We do the necessary conversion for values relevant for the sponsored user point.
let (user_randomness_lo, user_randomness_hi) = decompose(user_randomness);
// We use the unsafe version because the multi_scalar_mul will constrain the scalars.
let funded_amount_scalar = from_field_unsafe(funded_amount);
// TODO(#7324), TODO(#7323): using npk_m_hash here is vulnerable in 2 ways described in the linked issues.
let (user_npk_lo, user_npk_hi) = decompose(user_npk_m_hash);
let (funded_amount_lo, funded_amount_hi) = decompose(funded_amount);
let user_npk_m_hash_scalar = from_field_unsafe(user_npk_m_hash);
let user_randomness_scalar = from_field_unsafe(user_randomness);

// 4. We compute `G_amt * funded_amount + G_npk * user_npk_m_hash + G_rnd * randomness`.
let incomplete_user_point = multi_scalar_mul(
[G_amt, G_npk, G_rnd],
[Scalar {
lo: funded_amount_lo,
hi: funded_amount_hi
},
Scalar {
lo: user_npk_lo,
hi: user_npk_hi
},
Scalar {
lo: user_randomness_lo,
hi: user_randomness_hi
}]
[funded_amount_scalar, user_npk_m_hash_scalar, user_randomness_scalar]
);

// 5. At last we return the points.
Expand All @@ -255,15 +232,13 @@ impl PrivatelyRefundable for TokenNote {

fn complete_refund(incomplete_fee_payer_point: Point, incomplete_user_point: Point, transaction_fee: Field) -> (Point, Point) {
// 1. We convert the transaction fee to high and low limbs to be able to use BB API.
let (transaction_fee_lo, transaction_fee_hi) = decompose(transaction_fee);
// We use the unsafe version because the multi_scalar_mul will constrain the scalars.
let transaction_fee_scalar = from_field_unsafe(transaction_fee);

// 2. We compute the fee point as `G_amt * transaction_fee`
let fee_point = multi_scalar_mul(
[G_amt],
[Scalar {
lo: transaction_fee_lo,
hi: transaction_fee_hi,
}]
[transaction_fee_scalar]
);

// 3. Now we leverage homomorphism to privately add the fee to fee payer point and subtract it from
Expand Down