Skip to content
This repository was archived by the owner on Jul 5, 2024. It is now read-only.

feat: upgrade RwTable and add aux#94

Merged
ed255 merged 15 commits into
privacy-ethereum:masterfrom
scroll-tech:rw_table/add_aux
Jan 25, 2022
Merged

feat: upgrade RwTable and add aux#94
ed255 merged 15 commits into
privacy-ethereum:masterfrom
scroll-tech:rw_table/add_aux

Conversation

@scroll-dev
Copy link
Copy Markdown
Contributor

@scroll-dev scroll-dev commented Jan 17, 2022

upgrade RwTable layout according to https://hackmd.io/VCGteyZEQFKUjHA_A72f1Q

  1. increase RW table column and add aux
  2. use fixed position for "value"&"value_prev" for different RW, allowing consistent codes/logic
  3. change "TxRefund"&"AccountDestructed" from "write_only_persistent" to "write_with_reversion"
  4. update tests

@ed255
Copy link
Copy Markdown
Contributor

ed255 commented Jan 17, 2022

I've taken a look at this PR and I have a question: could you explain which tag in the RW table will use the aux1 and aux2 columns, and what values will be put there? Thanks!

@0xmountaintop
Copy link
Copy Markdown
Contributor

0xmountaintop commented Jan 17, 2022

I've taken a look at this PR and I have a question: could you explain which tag in the RW table will use the aux1 and aux2 columns, and what values will be put there? Thanks!

Hi @ed255! As in https://hackmd.io/VCGteyZEQFKUjHA_A72f1Q#Data-kinds, only AccountStorage will use aux. And:

  • aux1 for tx_id
  • aux2 for committed_value(original_value_in_tx).

Copy link
Copy Markdown
Contributor

@han0110 han0110 left a comment

Choose a reason for hiding this comment

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

LGTM!

Also want to give some more context on why we remove write_only_persistent: It's a premature optimization for those states (e.g. TxRefund and AccountDestructed) which can't affect future execution, to reduce the amount of reversion write.

But in practice reversion will be costly by gas/rw or gas/step, so we don't need to worry about them. Also such optimization might not be future proof, since we don't know if in the future EVM execution depends on such state or not.

# - key3
# - key4
# - value
# - value_prev
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

just one small question, seems value_prev not present in table of page: https://hackmd.io/VCGteyZEQFKUjHA_A72f1Q

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.

The table in the note is for State circuit itself. When it outputs to rw_table, it can get access to value_prev by rotation directly instead of having it in witness in the same row.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

make sense Thx!

Copy link
Copy Markdown
Collaborator

@DreamWuGit DreamWuGit left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks !

@0xmountaintop
Copy link
Copy Markdown
Contributor

tests fail. need to fix.

@0xmountaintop
Copy link
Copy Markdown
Contributor

fixed

Copy link
Copy Markdown
Contributor

@ed255 ed255 left a comment

Choose a reason for hiding this comment

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

LGTM!

@ed255 ed255 merged commit 1e3b596 into privacy-ethereum:master Jan 25, 2022
@0xmountaintop 0xmountaintop deleted the rw_table/add_aux branch January 27, 2022 04:36
@ed255 ed255 mentioned this pull request Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants