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-7702: remove chain_id in authorization tuple #9152

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

fjl
Copy link
Contributor

@fjl fjl commented Dec 18, 2024

This is a counterproposal to #9143. Since the chain ID is already available in the context of the transaction, there is no need to specify it again in each authorization tuple. To preserve the special case of an authorization which is valid on all chains (chain_id == 0), a new boolean field unchained is added to the tuple in place of chain_id.

@fjl fjl requested a review from eth-bot as a code owner December 18, 2024 14:02
@github-actions github-actions bot added c-update Modifies an existing proposal s-review This EIP is in Review t-core labels Dec 18, 2024
@eth-bot
Copy link
Collaborator

eth-bot commented Dec 18, 2024

File EIPS/eip-7702.md

Requires 1 more reviewers from @adietrichs, @lightclient, @SamWilsn, @vbuterin

@eth-bot eth-bot added the a-review Waiting on author to review label Dec 18, 2024
Copy link
Contributor

@smartprogrammer93 smartprogrammer93 left a comment

Choose a reason for hiding this comment

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

Makes sense


```python
assert auth.chain_id < 2**64
assert auth.unchained == 0 || auth.unchained == 1
Copy link
Member

Choose a reason for hiding this comment

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

This specific check looks like the yParity check (where yParity is also either 0 or 1).

Suggested change
assert auth.unchained == 0 || auth.unchained == 1
assert auth.unchained < 2**8

There should be an added check for each tuple to validate if the auth.unchained is 0 or 1 (if not, skip the tuple).

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think this line should be reverted. This section has to do with decoding txs and I think 1 byte is the highest resolution we want here. I think many languages represent boolean as 1 byte value despite it only needing 1 bit of information.

We would verify unchained is 0 or 1 in the tuple validation.

Copy link
Contributor

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

From Reth side we don't think this makes full sense / is an improvement.

With this idea:

  • When I'm signing, I need to sign auth_chain_id which I need to pull from the transaction context, depending on whether the unchained flag is set or not.
  • This seems more complex for not changing any functionality, or improving security.

I understand that this is a counterproposal to U256 chainId because plausibly U256 as as an auth parameter makes the auth list big in terms of data?

@fjl
Copy link
Contributor Author

fjl commented Dec 19, 2024

I'm proposing this just because it's weird to duplicate a potentially large chainID in every authorization tuple.
The chainID value will always be zero or equal to the tx chainID.

From an implementation point of view, the chainID is already an input to the signing function. This does not change with the PR, you still have to supply the chainID when signing an authorization. We just leave out the chainID in the encoded authorization object that goes into the transaction, and replace it with a single bit instead, which says if chainID was zero.

@gurukamath
Copy link

Would this leave any room for replaying a chain specific authorisation tuple on a different chain?

@fjl
Copy link
Contributor Author

fjl commented Dec 19, 2024

No, because the signed-over data still contains the chainID, and it will be verified with the chainID of the executing chain.

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.

8 participants