Skip to content

[EIP-3074] Move to two-opcode version #2

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

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

Conversation

adietrichs
Copy link

@adietrichs adietrichs commented Mar 12, 2021

Updated the EIP to reflect the new AUTH, AUTHCALL iteration.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Since this is your first pull request, we kindly remind you to check out EIP-1 for guidance.

EIPS/eip-3074.md Outdated
@@ -1,6 +1,6 @@
---
eip: 3074
title: Native Sponsored Transactions
title: AUTHORIZE and AUTHORIZEDCALL opcodes

Choose a reason for hiding this comment

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

Suggested change
title: AUTHORIZE and AUTHORIZEDCALL opcodes
title: AUTHORIZEDCALL opcode

I prefer this, because it is the "meat" of the EIP. AUTHORIZE is really just a helper op.

Copy link
Author

Choose a reason for hiding this comment

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

I think I prefer "AUTH and AUTHCALL opcodes", because those might grow to a family of opcodes in the future

EIPS/eip-3074.md Outdated
- **Callee** - the target of the call from `CALLFROM`.
| Variable | Type | Initial Value |
| ------------------- | --------- |:------------------------------------------------------------------------------------ |
| `authorizedAccount` | `address` | `0x0000000000000000000000000000000000000000` |

Choose a reason for hiding this comment

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

Does 0x0000000000000000000000000000000000000000 == 0000000000000000000000000000000000000000000000000000000000000000? Because we wouldn't want a change in address format to break things.

Copy link
Author

Choose a reason for hiding this comment

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

I think one alternative would be to have it be an optional address (or address pointer), so that we don't have to use the zero address to indicate no authorized account

Choose a reason for hiding this comment

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

Once pushed on the stack, addresses are extended to 32 bytes, right?

Choose a reason for hiding this comment

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

@SamWilsn yes, I'm just trying to think if there is any weirdness that could occur.

EIPS/eip-3074.md Outdated
| ---------- | --------- |
| `top - 0` | `valid` |
| `top - 1` | `success` |
The arguments (`yParity`, `r`, `s`) are interpreted as an ECDSA signature of the form `secp256k1(keccak256(TYPE || abi.encode(invoker, commit)))`, where:

Choose a reason for hiding this comment

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

Suggested change
The arguments (`yParity`, `r`, `s`) are interpreted as an ECDSA signature of the form `secp256k1(keccak256(TYPE || abi.encode(invoker, commit)))`, where:
The arguments (`yParity`, `r`, `s`) are interpreted as an ECDSA signature on the secp256k1 curve over the message `keccak256(TYPE || rlp([invoker, commit]))`, where:

Choose a reason for hiding this comment

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

At minimum, I'd like to use rlp(...) instead of abi.encode(...).

Choose a reason for hiding this comment

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

or by hand >:(

Copy link
Author

Choose a reason for hiding this comment

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

Changed it to specifying it by hand.

EIPS/eip-3074.md Outdated
- `commit` is one of the arguments passed into `AUTHORIZE` and is a 32-byte value that can be used to commit to specific additional validity condition in the invoker's pre-processing logic.

If the signature is valid, the `signerAddress` is recovered. If `signerAddress != tx.origin`, the context variable `authorizedAccount` is set to `signerAddress`.
In any other case, i.e. if the signature is invalid or `signerAddress == tx.origin`, `authorizedAccount` is reset to the zero address.

Choose a reason for hiding this comment

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

Would rather have a bullet point list that explains what an "invalid signature" is.

@adietrichs adietrichs marked this pull request as ready for review March 12, 2021 23:57

`success` must be a one in all other cases.
The gas cost for `AUTH` is `3000`. This is the same cost as for the `ecrecover` precompile.

Choose a reason for hiding this comment

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

Does calling into the ecrecover precompile cost 3000 gas total, or is it 3000 plus the cost of a call?

If it's 3000+call, I think this needs to be a bit more expensive.

Copy link
Author

Choose a reason for hiding this comment

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

Looked into this. Turns out the full cost of an ecrecover (via STATICCALL) after Berlin will just be this 3000 gas plus a single WARM_STORAGE_READ_COST (100) as per EIP-2929. AUTH has none of the static call overhead. Both ORIGIN and CALLER have a gas cost of 2, so I think any extra cost here should also be on the order of ~10 gas at most - at which point a clean 3000 might just be preferable...

Copy link
Author

Choose a reason for hiding this comment

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

Ah okay, I did actually find one extra step that is somewhat expensive, namely the hashing for the signed message. That would natively have a gas cost of 48. So I feel like the realistic values for AUTH gas cost could be 3000 / 3050 / 3100.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants