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

cannot create multiple non-revertible update requests to the same public storage slot in a single TX #4736

Closed
just-mitch opened this issue Feb 23, 2024 · 2 comments · Fixed by #4750

Comments

@just-mitch
Copy link
Contributor

just-mitch commented Feb 23, 2024

Historically, if a user wrote to the same public storage slot multiple times in the same transaction, writes would be squashed such that only the most recent write would persist. This was accomplished by:

  • the simulator maintaining a map of accumulated storage update requests (see ContractStorageActionsCollector)
  • the outputs from the public kernel circuits being modified in TS to squash writes (see removeRedundantPublicDataWrites)

Note, these writes ought to be squashed by the public kernel itself. The reason this is possible without the squashing via the public kernel is that the base rollup circuit trusts that the writes it is provided are valid

However, with the introduction of non-revertible side effects, there are a few pieces of work needed to be able to write to the same public storage slot multiple times in a non-revertible manner.

  1. Expand removeRedundantPublicDataWrites to be squash writes from the non-revertible set
  2. When recombining revertible and non-revertible writes, also squash redundant writes
@LHerskind
Copy link
Contributor

@just-mitch can you add some context in here? What is the issue? We haven't had any issues with updating public storage as far as I know of?

@just-mitch just-mitch changed the title cannot write to public storage twice cannot create multiple non-revertible update requests to the same public storage slot in a single TX Feb 26, 2024
@just-mitch
Copy link
Contributor Author

@LHerskind Just updated the comment!

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
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants