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

Update EIP-4200: Allow RJUMPV table sizes of up to 256 #6546

Merged
merged 4 commits into from
May 11, 2023

Conversation

Philogy
Copy link
Contributor

@Philogy Philogy commented Feb 22, 2023

Since RJUMPV table sizes of 0 are disallowed I'm suggesting a change that would make table sizes start at 1, meaning a count byte of 0 would indicate a table of size 1 and a count byte 0xff would indicate a table size of 256. This would remove an unnecessary piece of validation logic and allow tables that can map the full range of a 4-bit case value without 0xff resulting in the default path.

@github-actions github-actions bot added c-update Modifies an existing proposal s-review This EIP is in Review t-core labels Feb 22, 2023
@eth-bot
Copy link
Collaborator

eth-bot commented Feb 22, 2023

✅ All reviewers have approved.

@gumb0
Copy link
Member

gumb0 commented Feb 24, 2023

Originally we argued against this idea to avoid the spec stating "number count really means count + 1", which seemed quirky.

But we could also reframe it to mean max index of the table.

@chfast
Copy link
Member

chfast commented Feb 27, 2023

I think this is a good change. So far I was mostly focused on the encoding but realized having 256 element jump table should be very useful for implementing any bytecode interpreter.

@shemnon
Copy link
Contributor

shemnon commented Feb 27, 2023

Perhaps instead of size it becomes "max index" - so jumpv of 1 has two indexes, 0 and 1.

Also, by that logic what's the value of jumpv 1, shouldn't 2 be the minimum? (please no, accept the inconsistency).

The big advantage I see is that we can do a full byte mapping to do things such as decoding large jump tables, with a full byte. Although nybbles (4-bits) will probably be more useful for sparser tables.

While we could do this pre-change it would require fallthrough handling of 0xFF.

@Philogy
Copy link
Contributor Author

Philogy commented Feb 28, 2023

Originally we argued against this idea to avoid the spec stating "number count really means count + 1", which seemed quirky.

But we could also reframe it to mean max index of the table.

I agree that this definition can be confusing although I don't think that's a good enough reason not to include this change. I also think putting it in terms of "the byte represents the max index" is a good idea.

@axic
Copy link
Member

axic commented Mar 1, 2023

I agree with the change.

EIPS/eip-4200.md Outdated Show resolved Hide resolved
@eth-bot eth-bot added the a-review Waiting on author to review label Mar 7, 2023
Co-authored-by: Andrei Maiboroda <andrei@ethereum.org>
@gumb0
Copy link
Member

gumb0 commented Mar 7, 2023

Some more fixes needed:

Specification, line 50 should say:

3. `RJUMPV max_index relative_offset+` pops a value (`case`) from the stack, and sets the `PC` to `PC_post_instruction + ((case > max_index) ? 0 : relative_offset[case])`.

Test cases, Invalid cases, remove line 138:

- `RJUMPV` with table size 0

Test cases, Execution, line 149 should say:

- `RJUMPV 0 relative_offset`

Line 155:

- `case` outside of table bounds (`case > max_index`, fallback case)

EIPS/eip-4200.md Outdated Show resolved Hide resolved
@lightclient
Copy link
Member

This makes sense to me.

Co-authored-by: Andrei Maiboroda <andrei@ethereum.org>
@lightclient
Copy link
Member

bumping @gumb0 @axic @chfast

@eth-bot eth-bot enabled auto-merge (squash) May 11, 2023 14:18
Copy link
Collaborator

@eth-bot eth-bot left a comment

Choose a reason for hiding this comment

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

All Reviewers Have Approved; Performing Automatic Merge...

@eth-bot eth-bot merged commit 9cbdeb4 into ethereum:master May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-review Waiting on author to review c-update Modifies an existing proposal s-review This EIP is in Review t-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants