feat: EIP-3074 Implementation #1
Conversation
`AUTH` opcode handler
feat: `AUTH` updates
chore: Share `secp256k1` code
| }; | ||
|
|
||
| // If the memory region is longer than 65 bytes, pull the commit from the next [1, 32] bytes. | ||
| let commit = (mem_slice.len() > 65).then(|| { |
There was a problem hiding this comment.
in general, the spec doesn't specify the behavior if the memory region is less than 97 bytes. should we add that to the spec? i wonder what would be the desired behavior there - immediately return 0?
There was a problem hiding this comment.
This is mostly to copy the behavior of Go's copy intrinsic, where it can perform partial reads. It would be nice to have some clarity here!
Immediately returning 0 makes sense, assuming the commit must be present (or at the very least, 0'd out explicitly).
| let signature = { | ||
| // Place the yParity value, located at the first byte, in the last byte. | ||
| let mut sig = [0u8; 65]; | ||
| sig[0..64].copy_from_slice(&mem_slice[1..65]); |
There was a problem hiding this comment.
we don't check that 65 <= mem_slice.len() before doing this, idk if that's necessary or if this will error on its own
There was a problem hiding this comment.
Yeah this will panic, but we should handle gracefully. The spec doesn't mention what to do if the length passed to AUTH is < 65 unless I missed it. Should we halt here and consume all gas?
The above memory expansion only covers the length passed to the AUTH op, so you could make it panic with this setup.
There was a problem hiding this comment.
The way that i would interpret it is that if the length is less than 65 bytes, then there would be 0 bytes in the sig and if it was more than 65 bytes it would truncate. Tradeoffs in upgrade flexibility (not enshrining ecdsa) vs developer footguns
There was a problem hiding this comment.
Yeah it isn't clearly stated, but I believe the general behavior for EVM operations is that unexpanded memory is always considered 0 (but you need to pay for the expansion). So in this case, if the length is less than 65 bytes you would grab as many bytes as allowed, fill in zeros for the rest, then charge for the filled in zeros and consider that region "expanded".
Overall the code needs refactors but works for now.
There was a problem hiding this comment.
Looks pretty good to me. I think the gas accounting is missing specific rules for AUTHCALL though, but not certain since didn't dig too deeply into call_inner. The EIP is a bit cheeky and removes the call stipend when value is non-zero.
| @@ -24,6 +24,7 @@ pub enum InstructionResult { | |||
| OpcodeNotFound, | |||
| CallNotAllowedInsideStatic, | |||
| StateChangeDuringStaticCall, | |||
| ActiveAccountUnsetAuthCall, | |||
There was a problem hiding this comment.
| ActiveAccountUnsetAuthCall, | |
| UnauthorizedAuthCall, |
just a proposal, maybe read a bit easier
| let signature = { | ||
| // Place the yParity value, located at the first byte, in the last byte. | ||
| let mut sig = [0u8; 65]; | ||
| sig[0..64].copy_from_slice(&mem_slice[1..65]); |
There was a problem hiding this comment.
Yeah it isn't clearly stated, but I believe the general behavior for EVM operations is that unexpanded memory is always considered 0 (but you need to pay for the expansion). So in this case, if the length is less than 65 bytes you would grab as many bytes as allowed, fill in zeros for the rest, then charge for the filled in zeros and consider that region "expanded".
Thanks for review! Yeah, gas accounting is not yet to-spec. @tynes has a PR open for this @ #6 |
authcall: gas schedule
Overview
Contains an implementation of the EIP-3074 specification.