Feature/eip 3074 #6944
Feature/eip 3074 #6944
Conversation
| @@ -2095,6 +2104,90 @@ private CallResult ExecuteCall<TTracingInstructions>(EvmState vmState, ReadOnlyM | |||
| break; | |||
| } | |||
| } | |||
| case Instruction.AUTH: | |||
There was a problem hiding this comment.
This is pretty stack heavy (x3 stackalloc, x3 byte[], several other locals), and fairly large. As stack costs are paid upfront at the method level and the Jit will bail on optimizing if a method gets too big, this will cause knock on effects to all tx whether or not they call AUTH.
Could it instead be moved out to a method InstructionAuth; see InstructionLog, InstructionSLoad, InstructionSStore, InstructionRevert, etc as examples
There was a problem hiding this comment.
Yes I wanted to refactor it to separate method. Do you think the zero padding and byte[] alloc should be done with stackalloc instead?
There was a problem hiding this comment.
Can look at that later, is more the method bloat of ExecuteCode that bothers me since its already vastly long which means the Jit is way passed trying to optimize local variables; so every one added even if just an int causes more space to be reserved and zeroed at the start of the method which adds to every call even if the code that uses that local is never run.
It will get even worse if going to add a try/catch for _ecdsa.RecoverPublicKey. Whereas if you move it to another method then the Jit will optimize that hard since its short and it won't add to the stack clearing and code size of the general dispatch method.
There was a problem hiding this comment.
try/catch will not help with _ecdsa.RecoverPublicKey since it will throw a CSE.
Should i use [SkipLocalsInit] flag for the new method you think, or better to let JIT handle zeroing?
| @@ -48,6 +50,16 @@ public Signature Sign(PrivateKey privateKey, Hash256 message) | |||
| return signature; | |||
| } | |||
|
|
|||
| public PublicKey? TryRecoverPublicKey(ReadOnlySpan<byte> r, ReadOnlySpan<byte> s, byte v, Hash256 message) | |||
| Span<byte> sigData = stackalloc byte[64]; | ||
| Span<byte> commit = stackalloc byte[32]; | ||
| //Skip init flag is active so we have to iterate all data | ||
| for (int i = 0; i < 97; i++) | ||
| { | ||
| byte data = 0; | ||
| if (i < b) | ||
| data = memData[i]; | ||
| switch (i) | ||
| { | ||
| case 0: | ||
| yParity = data; | ||
| break; | ||
| case >= 1 and <= 64: | ||
| sigData[i - 1] = data; | ||
| break; | ||
| default: | ||
| commit[i - 65] = data; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Can you just break up original Span to smaller ones and use https://learn.microsoft.com/en-us/dotnet/api/system.memoryextensions.reverse?view=net-8.0 on them, instead of allocating new ones?
| byte[] chainId = _chainId.PadLeft(32); | ||
| byte[] authorityNonce = _state.GetNonce(authority).PaddedBytes(32); | ||
| //TODO is ExecutingAccount correct when DELEGATECALL and CALLCODE? | ||
| byte[] invokerAddress = vmState.Env.ExecutingAccount.Bytes.PadLeft(32); |
There was a problem hiding this comment.
I think those don't need to be allocated at all, they only should be written to msg Span in correct places.
…ethermind into feature/eip-3074
| if (!stack.PopUInt256(out UInt256 offset)) return EvmExceptionType.StackUnderflow; | ||
| if (!stack.PopUInt256(out UInt256 length)) return EvmExceptionType.StackUnderflow; |
There was a problem hiding this comment.
We have
SkipInit(out UInt256 a);
SkipInit(out UInt256 b);
SkipInit(out UInt256 c);
SkipInit(out UInt256 result);
SkipInit(out StorageCell storageCell);
Can those be reused rather that allocating more on stack? Passing them by ref here? This can be also used for other methods in VM?
There was a problem hiding this comment.
Not really as it will allocate more stack to pass the parameters to the method; abi calling convetion means only have about 2 registers for an instance generic (first register is this pointer, second is generic type)
There was a problem hiding this comment.
I assume AUTH and AUTHCALL won't be so heavily used that the stack amount should be significant on them
There was a problem hiding this comment.
One UInt256 is 32 bytes, but passing it by ref should be only 8 bytes, no?
| byte[] chainId = _chainId.PadLeft(32); | ||
| UInt256 nonce = _state.GetNonce(authority); | ||
| byte[] invokerAddress = invoker.Bytes; | ||
| Span<byte> msg = stackalloc byte[1 + 32 * 4]; | ||
| msg[0] = Eip3074Constants.AuthMagic; | ||
| for (int i = 0; i < 32; i++) | ||
| { | ||
| int offset = i + 1; | ||
| msg[offset] = chainId[i]; | ||
| msg[32 + 32 - i] = (byte)(nonce[i / 8] >> (8 * (i % 8))); | ||
|
|
||
| if (i < 12) | ||
| msg[offset + 64] = 0; | ||
| else if (i - 12 < invokerAddress.Length) | ||
| msg[offset + 64] = invokerAddress[i - 12]; | ||
|
|
||
| if (i < commit.Length) | ||
| msg[offset + 96] = commit[i]; | ||
| } |
There was a problem hiding this comment.
This is probably suboptimal, you can write chainid directly to message rather than creating byte array, similar you can probably SIMD and not for some other bytes.
| for (int i = 0; i < 97; i++) | ||
| { | ||
| byte data = 0; | ||
| if (i < length) | ||
| data = memData[i]; | ||
| switch (i) | ||
| { | ||
| case 0: | ||
| yParity = data; | ||
| break; | ||
| case >= 1 and <= 64: | ||
| sigData[i - 1] = data; | ||
| break; | ||
| default: | ||
| commit[i - 65] = data; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
looks SIMD-able. @benaadams ?
Note that AUTHCALL gas rules are different than normal CALL
https://eips.ethereum.org/EIPS/eip-3074
Changes
Introduces opcodes:
AUTH
AUTHCALL
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Remarks