Update EIP 3074 AUTHCALL #1867

jochem-brouwer · 2022-04-26T14:33:19Z

This updates the AUTHCALL EIP. Changes were made: ethereum/EIPs#4967

codecov · 2022-04-27T20:25:47Z

Codecov Report

Merging #1867 (41d3e25) into develop (d62b7f4) will decrease coverage by 0.05%.
The diff coverage is 81.81%.

Flag	Coverage Δ
block	`86.15% <ø> (ø)`
blockchain	`82.42% <ø> (ø)`
client	`77.72% <ø> (ø)`
common	`95.01% <ø> (ø)`
devp2p	`82.50% <ø> (-0.14%)`	⬇️
ethash	`90.81% <ø> (ø)`
statemanager	`84.87% <ø> (ø)`
trie	`80.19% <ø> (-0.50%)`	⬇️
tx	`92.13% <ø> (ø)`
util	`87.29% <ø> (ø)`
vm	`80.35% <81.81%> (-0.04%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

packages/vm/src/evm/opcodes/functions.ts

ryanio · 2022-04-29T18:05:55Z

looks good. is it possible to add code coverage for the newly added/missed lines? just for completeness sake

packages/vm/tests/api/EIPs/eip-3074-authcall.spec.ts

holgerd77 · 2022-05-04T09:36:22Z

This would need a cherry-picked re-submission against the latest rebased develop from #1873.

jochem-brouwer · 2022-05-08T16:46:34Z

When writing tests, I found that the new spec is not entirely clear regarding the gas schedule, so have to wait on the remarks on that. I will expand the test suite to cover the new lines.

jochem-brouwer · 2022-05-13T21:52:57Z

Clarified: ethereum/EIPs#5093

This is also what we do at this point.

holgerd77 · 2022-05-16T10:01:30Z

Clarified: ethereum/EIPs#5093

This is also what we do at this point.

Thanks for the update, so just to make sure we are on the same page: do you still plan to write additional tests here (as announced above) or is the PR now ready and just needs a final branch/conflict update?

jochem-brouwer · 2022-05-16T14:09:30Z

Yes, there need to be a few more tests added to cover the new EIP rules 😄 👍

holgerd77 · 2022-05-17T10:54:20Z

Let's maybe wait here on merging until the initial EVM/VM refactor PR #1862 is settled and merge to not complicate any further over there. This one already needed so many patient reworks that we really should prioritize over there. 🙂

holgerd77 · 2022-05-17T10:54:55Z

(given this a "Blocked" label for now for this reason, can be removed once the above PR is merged into develop)

jochem-brouwer · 2022-05-26T00:59:19Z

I can remove blocked label now right @holgerd77 ?

holgerd77 · 2022-05-26T10:20:23Z

Thanks, yes, rebased this via UI (locally tested) and will review. 🙂

holgerd77 · 2022-05-26T10:50:52Z

@SamWilsn would it eventually be possible for you to do a review here?

holgerd77 · 2022-06-03T09:11:04Z

Rebased via UI after local test rebase.

holgerd77

A bit more superficial review than I would have wished, but I think I got the essence of it and this should be sufficient.

Great, thanks Jochem, will merge!

holgerd77 · 2022-06-03T09:32:43Z

packages/vm/src/evm/opcodes/functions.ts

+      const yParity = bufferToBigInt(mem[31])
+      const r = mem.slice(32, 64)
+      const s = mem.slice(64, 96)
+      const commit = mem.slice(96, 128)


Ok, so this is this new authority + signature in memory change.

holgerd77 · 2022-06-03T09:33:20Z

packages/vm/src/evm/opcodes/functions.ts

+      let mem = runState.memory.read(Number(memOffset), Number(memLength))
+      if (mem.length < 128) {
+        mem = setLengthRight(mem, 128)
+      }


"If length is greater than 128, the extra bytes are ignored. Bytes outside the range (in the event length is less than 128) are treated as if they had been zeroes."

👍

holgerd77 · 2022-06-03T09:35:37Z

packages/vm/tests/api/EIPs/eip-3074-authcall.spec.ts

+    PUSH32,
+    addressBuffer,
+    AUTH,
+  ])


I will trust the PR author that these test updates are correct. 😅 😀

holgerd77 · 2022-06-03T09:36:29Z

packages/vm/tests/api/EIPs/eip-3074-authcall.spec.ts

+    const result = await vm.runTx({ tx, block })
+    const buf = result.execResult.returnValue
+    st.ok(buf.equals(zeros(32)), 'auth puts 0')
+  })


Nice additional test cases! 👍

* vm: update eip 3074 * vm: eip3074 test fixes start * vm: fix eip3074 tests * vm: expand eip3074 tests * vm: add eip 3074 tests * vm: address 128n -> BigInt(128)

SamWilsn · 2022-06-03T16:40:00Z

packages/vm/src/evm/opcodes/functions.ts

+        mem = setLengthRight(mem, 128)
+      }
+
+      const yParity = BigInt(mem[31])


Hm, I don't think this line is correct. EIP-3074 says that:

memory[offset : offset+32 ] - yParity

So yParity could, in theory, be larger than one byte. As specified, signature verification would fail in that case, but with this implementation, it looks like the higher bytes are ignored?

@jochem-brouwer could you have a look?

* vm: update eip 3074 * vm: eip3074 test fixes start * vm: fix eip3074 tests * vm: expand eip3074 tests * vm: add eip 3074 tests * vm: address 128n -> BigInt(128)

jochem-brouwer added PR state: WIP package: vm target: develop labels Apr 26, 2022

jochem-brouwer added PR state: needs review and removed PR state: WIP labels Apr 27, 2022

ryanio reviewed Apr 29, 2022

View reviewed changes

packages/vm/src/evm/opcodes/functions.ts Outdated Show resolved Hide resolved

ryanio reviewed Apr 29, 2022

View reviewed changes

packages/vm/tests/api/EIPs/eip-3074-authcall.spec.ts Outdated Show resolved Hide resolved

holgerd77 force-pushed the develop branch from 929b2ff to 6df7b56 Compare May 4, 2022 09:22

jochem-brouwer added PR state: WIP and removed PR state: needs review labels May 13, 2022

jochem-brouwer marked this pull request as draft May 13, 2022 21:17

jochem-brouwer force-pushed the eip-3074-updated branch from 9b94c43 to 4072576 Compare May 16, 2022 15:53

jochem-brouwer marked this pull request as ready for review May 16, 2022 19:25

jochem-brouwer added PR state: needs review and removed PR state: WIP labels May 16, 2022

jochem-brouwer force-pushed the eip-3074-updated branch from f13738e to 1e2caf9 Compare May 16, 2022 19:28

holgerd77 added the PR/Issue state: blocked label May 17, 2022

jochem-brouwer added PR/Issue state: blocked and removed PR/Issue state: blocked labels May 18, 2022

jochem-brouwer removed the PR/Issue state: blocked label May 26, 2022

jochem-brouwer requested review from ryanio and holgerd77 May 26, 2022 00:59

holgerd77 force-pushed the eip-3074-updated branch from 1e2caf9 to 73c682d Compare May 26, 2022 10:19

jochem-brouwer added 6 commits June 3, 2022 11:10

vm: update eip 3074

4f898b5

vm: eip3074 test fixes start

5913ada

vm: fix eip3074 tests

d2537e7

vm: expand eip3074 tests

59b12b8

vm: add eip 3074 tests

620b865

vm: address 128n -> BigInt(128)

41d3e25

holgerd77 force-pushed the eip-3074-updated branch from 73c682d to 41d3e25 Compare June 3, 2022 09:10

holgerd77 approved these changes Jun 3, 2022

View reviewed changes

holgerd77 merged commit a7b1648 into develop Jun 3, 2022

holgerd77 deleted the eip-3074-updated branch June 3, 2022 09:37

holgerd77 mentioned this pull request Jun 3, 2022

Develop rebased on 3e4e7be #1928

Closed

g11tech pushed a commit that referenced this pull request Jun 3, 2022

Update EIP 3074 AUTHCALL (#1867)

c1f15c5

* vm: update eip 3074 * vm: eip3074 test fixes start * vm: fix eip3074 tests * vm: expand eip3074 tests * vm: add eip 3074 tests * vm: address 128n -> BigInt(128)

SamWilsn reviewed Jun 3, 2022

View reviewed changes

holgerd77 pushed a commit that referenced this pull request Jun 8, 2022

Update EIP 3074 AUTHCALL (#1867)

4d1b99b

* vm: update eip 3074 * vm: eip3074 test fixes start * vm: fix eip3074 tests * vm: expand eip3074 tests * vm: add eip 3074 tests * vm: address 128n -> BigInt(128)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update EIP 3074 AUTHCALL #1867

Update EIP 3074 AUTHCALL #1867

jochem-brouwer commented Apr 26, 2022 •

edited

Loading

codecov bot commented Apr 27, 2022 •

edited

Loading

ryanio commented Apr 29, 2022

holgerd77 commented May 4, 2022

jochem-brouwer commented May 8, 2022

jochem-brouwer commented May 13, 2022

holgerd77 commented May 16, 2022

jochem-brouwer commented May 16, 2022

holgerd77 commented May 17, 2022

holgerd77 commented May 17, 2022

jochem-brouwer commented May 26, 2022

holgerd77 commented May 26, 2022

holgerd77 commented May 26, 2022

holgerd77 commented Jun 3, 2022

holgerd77 left a comment •

edited

Loading

holgerd77 Jun 3, 2022

holgerd77 Jun 3, 2022

holgerd77 Jun 3, 2022

holgerd77 Jun 3, 2022

SamWilsn Jun 3, 2022

holgerd77 Jun 7, 2022

Update EIP 3074 AUTHCALL #1867

Update EIP 3074 AUTHCALL #1867

Conversation

jochem-brouwer commented Apr 26, 2022 • edited Loading

codecov bot commented Apr 27, 2022 • edited Loading

Codecov Report

ryanio commented Apr 29, 2022

holgerd77 commented May 4, 2022

jochem-brouwer commented May 8, 2022

jochem-brouwer commented May 13, 2022

holgerd77 commented May 16, 2022

jochem-brouwer commented May 16, 2022

holgerd77 commented May 17, 2022

holgerd77 commented May 17, 2022

jochem-brouwer commented May 26, 2022

holgerd77 commented May 26, 2022

holgerd77 commented May 26, 2022

holgerd77 commented Jun 3, 2022

holgerd77 left a comment • edited Loading

Choose a reason for hiding this comment

holgerd77 Jun 3, 2022

Choose a reason for hiding this comment

holgerd77 Jun 3, 2022

Choose a reason for hiding this comment

holgerd77 Jun 3, 2022

Choose a reason for hiding this comment

holgerd77 Jun 3, 2022

Choose a reason for hiding this comment

SamWilsn Jun 3, 2022

Choose a reason for hiding this comment

holgerd77 Jun 7, 2022

Choose a reason for hiding this comment

jochem-brouwer commented Apr 26, 2022 •

edited

Loading

codecov bot commented Apr 27, 2022 •

edited

Loading

holgerd77 left a comment •

edited

Loading