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

feat: storage slot as point #7437

Closed
wants to merge 70 commits into from
Closed

Conversation

benesjan
Copy link
Contributor

@benesjan benesjan commented Jul 11, 2024

Implementing Mike's storage slot as point scheme https://hackmd.io/ZTSdUpDTTnKVFJFr9xcUBw

Notes for reviewer:

  1. There are a lot of small issues which will need to be tackled after this PR is merge and I track them with this issue.. I am not tackling them in this PR to keep the scope as small as possible (particularly the naming is messy),
  2. this PR does not yet properly handles nesting and the use of different generators for different steps of slot derivation --> this is to make the PR scope smaller + it is not essential for the code to work (it's needed to avoid potential collisions but I think it's ok to handle in the following PR given that we are still prototyping).
  3. There is currently an issue with generating a verification key for base rollup. To workaround this issue I have commented out the contents of compute_fee_payer_gas_token_balance_leaf_slot func. This makes the fees tests not pass.

Copy link
Contributor Author

benesjan commented Jul 11, 2024

@benesjan benesjan force-pushed the 07-11-feat_storage_slot_as_point branch 3 times, most recently from 223968e to c2a74f9 Compare July 17, 2024 12:43
@benesjan benesjan changed the base branch from master to 07-15-refactor_optimizing_da_cost_with_new_point_compression July 17, 2024 12:43
@benesjan benesjan force-pushed the 07-11-feat_storage_slot_as_point branch 2 times, most recently from c7f26c9 to 616d63f Compare July 17, 2024 13:04
Base automatically changed from 07-15-refactor_optimizing_da_cost_with_new_point_compression to master July 17, 2024 14:33
@benesjan benesjan force-pushed the 07-11-feat_storage_slot_as_point branch 4 times, most recently from 398558a to 7b1dfa0 Compare July 22, 2024 14:09
Copy link
Contributor

github-actions bot commented Jul 22, 2024

Changes to circuit sizes

Generated at commit: f2ef5b2525bde15572faf3f4e532f2f0b20fd71b, compared to commit: 2633aa91a97684f481a861aee0be8756c956135a

🧾 Summary (100% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
public_kernel_tail -42 ✅ -0.00% -250 ✅ -0.01%
rollup_base -49 ✅ -0.01% -404 ✅ -0.01%
public_kernel_teardown -48 ✅ -0.02% -256 ✅ -0.02%
public_kernel_setup -48 ✅ -0.03% -256 ✅ -0.02%
parity_root -144 ✅ -2.94% -944 ✅ -0.02%
rollup_root -112 ✅ -2.67% -720 ✅ -0.02%
rollup_merge -80 ✅ -2.88% -496 ✅ -0.02%
private_kernel_empty -48 ✅ -2.58% -273 ✅ -0.03%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
public_kernel_tail 965,054 (-42) -0.00% 4,644,848 (-250) -0.01%
rollup_base 341,345 (-49) -0.01% 3,835,877 (-404) -0.01%
public_kernel_teardown 241,730 (-48) -0.02% 1,618,002 (-256) -0.02%
public_kernel_setup 183,195 (-48) -0.03% 1,554,056 (-256) -0.02%
parity_root 4,749 (-144) -2.94% 4,171,794 (-944) -0.02%
rollup_root 4,081 (-112) -2.67% 3,144,641 (-720) -0.02%
rollup_merge 2,702 (-80) -2.88% 2,092,160 (-496) -0.02%
private_kernel_empty 1,811 (-48) -2.58% 1,041,821 (-273) -0.03%

@AztecBot
Copy link
Collaborator

AztecBot commented Jul 22, 2024

Docs Preview

Hey there! 👋 You can check your preview at https://66a23c87d9f4111689cb6bac--aztec-docs-dev.netlify.app

@benesjan benesjan force-pushed the 07-11-feat_storage_slot_as_point branch from 7b1dfa0 to 5d19b84 Compare July 22, 2024 15:51
Copy link
Contributor

@nventuro nventuro left a comment

Choose a reason for hiding this comment

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

Thanks for taking on this behemoth of a task! I left some initial feedback and questions, but eventually got stuck with the review since there's too many things I don't understand. In particular I'm puzzled about the changes to the rust macros and how we now handle different levels of nesting in Map, since there seem to be no changes to that file.

Given that this is such a large changeset, with multiple new concepts, small renames and the beginnings of future refactors, I strongly suggest investing some time in the PR description so that readers have context re. what is being done here, how, what blockers you encountered and how you're temporarily working around them, etc. As is, it can be quite hard to understand all of the changes here armed with just the hackmd document.

@benesjan benesjan force-pushed the 07-11-feat_storage_slot_as_point branch 2 times, most recently from ba2a3a7 to 2bb3861 Compare July 23, 2024 08:41
@benesjan
Copy link
Contributor Author

Given that this is such a large changeset, with multiple new concepts, small renames and the beginnings of future refactors, I strongly suggest investing some time in the PR description

@nventuro sorry, should have done that before asking you for a review. Added the description

@benesjan benesjan force-pushed the 07-11-feat_storage_slot_as_point branch 2 times, most recently from fcfee8c to ea382e6 Compare July 24, 2024 10:09
@benesjan benesjan force-pushed the 07-11-feat_storage_slot_as_point branch from 07b753e to c0261fb Compare July 25, 2024 08:56
@benesjan
Copy link
Contributor Author

This PR turned out to be unnecessary so I am closing it in favor of https://github.com/AztecProtocol/aztec-packages/pull/7618/files

@benesjan benesjan closed this Jul 26, 2024
@nventuro nventuro deleted the 07-11-feat_storage_slot_as_point branch July 26, 2024 13:53
@nventuro
Copy link
Contributor

We realized that in order to support partial notes for e.g. private refunds (#7226) in such a way that'd avoid the issues listed in #7297, we don't really need for the storage slot to be a Point. What we really want is to change the definition of note_hash so that it is not the hash of the slot plus hash of the content, but rather multi scalar multiplication of the slot and content (e.g. value, randomness, npk). This lets us create partial notes with an embededd slot, while retaining a Field slot.

The only thing a slot Point allows for is for partial slots, e.g. multi-nested maps in which some of the keys are private but some are public. We're not doing this because:

  1. we cant think of an actual usecase for this
  2. it introduces extra complexity in very large parts of the codebase (note storage, decoding logs, storage setup, etc)
  3. it is possible that usecases that needed this could still be supported with slightly different schemes (somehow avoid nested maps)
  4. doing msm instead of poseidon for the storage slot results in a larger gate count, which we'd have to pay for even if not using partial notes
  5. if we really found a use case that needs this, it would be possible to add the feature later in aztec-nr 2.0, and have pxe support both the current and new scheme. Contracts could e.g. publicly announce they're using the new scheme, and PXE would interpret their logs differently.
  6. we are not sure how we'd present an api for this given that it mixes map and notes, so it'd be quite hard to use and easy to misuse

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants