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

Base rollup validates public reads at start state and not current state #2521

Open
rahul-kothari opened this issue Sep 26, 2023 · 2 comments
Assignees
Labels
bug Something isn't working

Comments

@rahul-kothari
Copy link
Contributor

Imagine you have a transaction that does some public writes and reads the same data later. Then for the base rollup to validate, it must validate that kernel tx not with the start state, but with the current state (after the writes).

Yet here we validate with the start state.

As a result any tx that does both reads and writes to the same data would fail at the base rollup with Membership check failed: base_rollup_circuit: validate_public_data_reads index i

This blocks #2167

@rahul-kothari rahul-kothari added the bug Something isn't working label Sep 26, 2023
@LHerskind
Copy link
Contributor

Spoke with @dbanks12, and the full solution need to handle ordering of public state changes. Until we have that, we can however do a workaround where we are simply skipping the check in the kernel for now.

Why can we do this? While it would be insecure to do, we can do it for the sandbox since the simulator is trusted to follow the rules and enforce the correct ordering.

@rahul-kothari
Copy link
Contributor Author

rahul-kothari commented Sep 27, 2023

In #2559, Commenting out the corresponding check in base rollup until this is fixed.

rahul-kothari added a commit that referenced this issue Sep 29, 2023
Part of #2167 - create the private flow for uniswap

This is already a large PR so omitted failure cases and public flow.
* UniswapPortal.sol -> has a separate flow for public and private.
Updated relevant tests too
* Uniswap aztec-nr contract -> just the private flow for now. 
* Deleted the sandbox example
* Updated e2e uniswap test with the private flow. Failure cases and
public flow will be in a separate PR
* Update the canary test
* 
**Also comments out base rollup check on public data reads as explained
in #2521**
just-mitch added a commit that referenced this issue Mar 1, 2024
Enable and test public refunds. 

Specifically, when Alice uses an FPC to pay her fees, and she *overpays*
(e.g. pays 3 bananas when only 1 is required), she has a public balance
transmitted to her as part of TX teardown (2 bananas in the example).

In order to accomplish this, we needed to fix #4736. 

Thus, this PR squashes writes performed in the non-revertible phases of
a TX.

Further, when all phases were completed, we squash across phases when we
recombine the `end` and `endNonRevertible`.

This presented a problem, because on current master that "squash across
phases" being performed during recombine in TS would need to be mirrored
in the base rollup.

Instead of doing that, this PR changes the private inputs to the base
rollup circuit to accept a `CombinedAccumulatedData`, i.e. effects that
have already been recombined. This affords the reduction in
gates/opcodes in the base_rollup.

This approach does not introduce any new trust assumptions because as
mentioned in #4736, the base rollup circuit [trusts that the writes it
receives are
valid](#2521 (comment)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Todo
Development

No branches or pull requests

3 participants