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

Add initial MPT table layout#196

Merged
ed255 merged 1 commit into
masterfrom
feature/tables-update
May 13, 2022
Merged

Add initial MPT table layout#196
ed255 merged 1 commit into
masterfrom
feature/tables-update

Conversation

@ed255
Copy link
Copy Markdown
Contributor

@ed255 ed255 commented May 10, 2022

I've added the table layout of the current MPT circuit design, as well as a unified table proposal which is currently not considered in the MPT design.

The table layout information has been gathered from:

@ed255 ed255 requested review from a team, han0110 and miha-stopar May 10, 2022 14:29
@ed255
Copy link
Copy Markdown
Contributor Author

ed255 commented May 10, 2022

Note: I added the "unified table proposal" eventhough it's not part of the MPT circuit design in order to make it easier to have a discussion about it; and taking into account that if it's implemented, the MPT circuit design and implementation is only extended (but nothing existing is modified), and this extension is quite simple.

Some questions to discuss the "unified table proposal" would be:

  • Is it worth reducing the number of columns in the lookup table at the expense of adding new columns in the circuit?
  • Is it the same doing 4 lookups with different (overlapping subsets of source / destination) columns than doing a single lookup with all the columns?

@miha-stopar
Copy link
Copy Markdown
Contributor

Note: I added the "unified table proposal" eventhough it's not part of the MPT circuit design in order to make it easier to have a discussion about it; and taking into account that if it's implemented, the MPT circuit design and implementation is only extended (but nothing existing is modified), and this extension is quite simple.

Some questions to discuss the "unified table proposal" would be:

  • Is it worth reducing the number of columns in the lookup table at the expense of adding new columns in the circuit?
  • Is it the same doing 4 lookups with different (overlapping subsets of source / destination) columns than doing a single lookup with all the columns?

Thanks @ed255 !

From some quick checking, I think unifying the lookup columns in the MPT circuit would improve performance only if the columns in the state circuit are unified too (for example, having nonce_prev and balance_prev in the same column), because halo2 permutes both (source and destination) columns for each lookup (if it's different source, we get a new permutation of the destination column too).

But I can benchmark it at some point and then we can decide how to proceed. Also, I can benchmark how much of an impact a couple of additional columns in MPT has, but generally I am a bit concerned about the number of columns in MPT :)

@ed255
Copy link
Copy Markdown
Contributor Author

ed255 commented May 12, 2022

From some quick checking, I think unifying the lookup columns in the MPT circuit would improve performance only if the columns in the state circuit are unified too (for example, having nonce_prev and balance_prev in the same column), because halo2 permutes both (source and destination) columns for each lookup (if it's different source, we get a new permutation of the destination column too).

The current design of the state circuit puts nonce, balance, codeHash and value (from a storage update) in the same column named value; and nonce_prev, balance_prev, codeHash_prev and value_prev are in the same column as well but with Rotation(-1); so we have unified columns in the state circuit that allows us to do a single lookup for all MPT updates :)

But I can benchmark it at some point and then we can decide how to proceed. Also, I can benchmark how much of an impact a couple of additional columns in MPT has, but generally I am a bit concerned about the number of columns in MPT :)

Having a benchmark sounds great! And yeah, totally agree on being careful with adding columns :)

Copy link
Copy Markdown
Contributor

@miha-stopar miha-stopar left a comment

Choose a reason for hiding this comment

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

LGTM (I thought I approved already yesterday, but somehow doesn't seem so)

Comment thread specs/tables.md Outdated
@miha-stopar
Copy link
Copy Markdown
Contributor

The current design of the state circuit puts nonce, balance, codeHash and value (from a storage update) in the same column named value; and nonce_prev, balance_prev, codeHash_prev and value_prev are in the same column as well but with Rotation(-1); so we have unified columns in the state circuit that allows us to do a single lookup for all MPT updates :)

That's cool :)

@ed255 ed255 force-pushed the feature/tables-update branch from 032fce8 to 80c13d9 Compare May 12, 2022 13:16
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!

For the table part, I would think it as synthesized expression from MPT circuit, so having a unified table seems more reasonable to me.

Also I think the position of values doesn't matter, as long as we can get their expression, then there is no difference (the number of lookup matters). So using a single lookup to such unified table with tag also makes sense to me.

@miha-stopar
Copy link
Copy Markdown
Contributor

Also I think the position of values doesn't matter, as long as we can get their expression, then there is no difference (the number of lookup matters). So using a single lookup to such unified table with tag also makes sense to me.

Ah, yeah, you are right. If we have an expression, we have a RLC of the columns involved and that means only one permutation and commitment.

@ed255 ed255 merged commit 2e1b773 into master May 13, 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.

3 participants