Implement EIP 7685 #3372

acolytec3 · 2024-04-25T16:54:47Z

packages/block/src/block.ts

jochem-brouwer

Left some comments. I think we need a sorting mechanism for any EIP which implements this.

For 6110:
Each deposit accumulated in the block must appear in the EIP-7685 requests list in the order they appear in the logs

For 7002:
It should be in the order it is pulled out of the queue (FIFO)

So this is not lexicographic, but in the order of how they are executed or in whatever order they get added to the FIFO queue.

packages/block/src/block.ts

packages/common/src/eips.ts

g11tech · 2024-04-28T12:52:36Z

Left some comments. I think we need a sorting mechanism for any EIP which implements this.

We discussed this on call (@acolytec3 and me), 7685 should have a code block to gather the requests from other EIPs in the sequence ordered by type and then there should be an exact match of requests in blockbody/request root.

g11tech · 2024-04-28T17:00:31Z

packages/vm/src/runBlock.ts

@@ -285,6 +285,12 @@ export async function runBlock(this: VM, opts: RunBlockOpts): Promise<RunBlockRe
    preimages: result.preimages,
  }

+  if (this.common.isActivatedEIP(7685)) {


we can do request field existence check even before we run the block

Also we need to pop requests out before we verify stateroot (and call verkle verify state): i think in apply block with a fn called accumulateRequests or something (and use the same fn in buildblock's build, which should return requests in result which we should return in above results:RunBlockResult

g11tech · 2024-04-29T16:29:48Z

packages/vm/src/runBlock.ts

+
+  // TODO: Add in code to accumulate partial withdrawals (EIP-7002)
+
+  if (requests.length > 1) {


ideally we should just make sure that we pop and push the requests in the type sequence` but i guess we can let it be for the moment

I agree that this check is not necessary if we check the requests in the right order above.

g11tech

lgtm

jochem-brouwer

Looks good so far! I got a few comments. I like the tests 👍

jochem-brouwer · 2024-04-29T22:56:13Z

packages/block/src/block.ts

+    if (clRequests !== undefined && clRequests.length > 1) {
+      for (let x = 1; x < clRequests.length; x++) {
+        if (clRequests[x].type < clRequests[x - 1].type)
+          throw new Error('requests are not sorted in ascending order')


Shouldn't this check be moved into the constructor? It is possible to create a block which does not satisfy this condition by using fromValuesArray or fromRlpSerializedBlock.

Fair point. Will move

jochem-brouwer · 2024-04-29T22:58:29Z

packages/block/src/types.ts

@@ -286,4 +302,5 @@ export type ExecutionPayload = {
  parentBeaconBlockRoot?: PrefixedHexString | string // QUANTITY, 64 Bits
  // VerkleExecutionWitness is already a hex serialized object
  executionWitness?: VerkleExecutionWitness | null // QUANTITY, 64 Bits, null implies not available
+  // TODO: Determine if we need the requestsRoot here


I would assume so!

@g11tech , agree?

I found this in the CL specs: https://github.com/ethereum/consensus-specs/blob/7bf43d1bc4fdb91059f0e6f4f7f0f3349b144950/specs/electra/beacon-chain.md#executionpayload

It is not updated yet (it is on old 6110 / 7002 which did not yet use 7685), but I would strongly assume that thus the requestsRoot is added.

packages/block/test/eip7685block.spec.ts

packages/common/src/eips.ts

packages/util/src/requests.ts

packages/vm/src/buildBlock.ts

jochem-brouwer · 2024-04-29T23:06:36Z

packages/vm/src/runBlock.ts

-        )
+  } else {
+    if (this.common.isActivatedEIP(7685)) {
+      const valid = await block.requestsTrieIsValid()


Shouldn't we do this check before running all txs?

If the generate option is called, the requestsRoot passed in the block will be disregarded and the correct one derived so checking before we validate the generate flag doesn't make sense. We validate it here if we are re-executing a block to ascertain its validity.

If generate is called we generate the requestsRoot and put that in the block (this is correct). However, this check is done in case we are not using generate (so generate is false). The block.requestsTrieIsValid is called on whatever block is provided - this itself, should not change (so this can be done before running any txs). However what you also want to check is that the block.genRequestsTrieRoot(requests) (so not block.requests, but the requests we just have accumulated in accumulateRequests(this)) equals the provided block.header.requestsRoot. If this is not the case, then the block thus has a different requests root and the block is incorrect.

Ah, that makes sense. Will push an update shortly.

packages/vm/src/runBlock.ts

jochem-brouwer · 2024-04-29T23:09:50Z

packages/vm/src/runBlock.ts

+
+  // TODO: Add in code to accumulate partial withdrawals (EIP-7002)
+
+  if (requests.length > 1) {


I agree that this check is not necessary if we check the requests in the right order above.

holgerd77

Looks absolutely amazing 🤩, also agree with Jochem, test cases added are really fabulous and already covering such an amazingly broad scope of the added functionality!

Some small comments but nothing major to be added from my review here.

holgerd77 · 2024-04-30T08:50:28Z

packages/util/src/requests.ts

+
+export class CLRequest implements CLRequestType {
+  type: number
+  bytes: Uint8Array = new Uint8Array()


Short nit: I guess new Uint8Array() is not needed here?

Probably not. It was just easier than logging out random bytes to the console and then putting it in. Will adjust.

Guess comment is misplaced here? 😅

Indeed, meant to put that one on your question about randomBytes. This is why I shouldn't respond to code reviews on mobile.

holgerd77 · 2024-04-30T09:23:37Z

packages/block/src/block.ts

@@ -417,6 +472,7 @@ export class Block {
    uncleHeaders: BlockHeader[] = [],
    withdrawals?: Withdrawal[],
    opts: BlockOptions = {},
+    requests?: CLRequest[],
    executionWitness?: VerkleExecutionWitness | null
  ) {


Just to note here that we need to be a bit careful with such parameter order switches (so: putting requests before executionWitness) for backwards compatibility reasons (we already have released a block library version with executionWitness included. Guess in this case it should be ok though and does make sense to have this in the "correct" order from the beginning.

holgerd77 · 2024-04-30T09:41:54Z

packages/client/src/rpc/modules/eth.ts

@@ -140,6 +140,8 @@ const jsonRpcBlock = async (
    blobGasUsed: header.blobGasUsed,
    excessBlobGas: header.excessBlobGas,
    parentBeaconBlockRoot: header.parentBeaconBlockRoot,
+    requestsRoot: header.requestsRoot,
+    requests: block.requests?.map((req) => bytesToHex(req.serialize())),


Side note: I do not see executionWitness in here? Is this forgotten as well?

(in doubt not for this PR)

holgerd77 · 2024-04-30T09:44:58Z

packages/common/src/eips.ts

+    status: Status.Draft,
+    // TODO: Set correct minimum hardfork
+    minimumHardfork: Hardfork.Cancun,
+    requiredEIPs: [],


Guess we can (minimally) add 3675 (consensus to PoS Upgrade) here if we want to.

Yeah, but also not so relevant eventually.

holgerd77 · 2024-04-30T10:09:04Z

packages/vm/src/runBlock.ts

+
+  // TODO: Add in code to accumulate deposits (EIP-6110)
+
+  // TODO: Add in code to accumulate partial withdrawals (EIP-7002)


Simply for my understand: so here a call into read_withdrawal_requests() from https://eips.ethereum.org/EIPS/eip-7002#execution-layer ("System Call" section) would take place, right?

Side question for @acolytec3, since you likely have better imagination on this: how does this affect execution with e.g. RPCStateManager? Does this still work (do not yet get the pieces together in my head)?

To the question for me, I don't "think" this should have any impact. We already track contract state updates in a local cache in the RPCStateManager so it should keep track of any changes we make in the processing of requests.

@holgerd77 Yes exactly here the call to read_withdrawal_requests is made. For 6110 I am not sure if all the logic is in here or not (but I am not super deep into 6110, @scorbajio might have better insights here)

For 6110, deposits are retrieved from the block receipt logs for any logs whose address is DEPOSIT_CONTRACT_ADDRESS and also from the block body requests field for any requests that are typed as deposit requests.

packages/vm/test/api/EIPs/eip-7685.spec.ts

holgerd77

Will take this in now to "break the PR dependency cascade", guess we can eventually fine tune last things along both practical integrations.

acolytec3 added 5 commits April 25, 2024 11:26

Add request type to util

3735c1a

Add requests to block

237e527

Add requests root validation

352cd93

Add ordering checks

975ab38

Add note on requestsRoot non-determinism

0f06222

acolytec3 added package: block package: common package: util labels Apr 25, 2024

acolytec3 added 4 commits April 25, 2024 15:29

Rework request structure to use interface and base class

74369ba

Make requests optional

c1b725e

Update ordering test

387af7c

Improve tests and remove unnecessary rlp encoding

43ed67d

scorbajio reviewed Apr 25, 2024

View reviewed changes

packages/block/src/block.ts Outdated Show resolved Hide resolved

jochem-brouwer reviewed Apr 25, 2024

View reviewed changes

packages/block/src/block.ts Outdated Show resolved Hide resolved

packages/block/src/block.ts Show resolved Hide resolved

Reorder requests order [no ci]

643e96e

holgerd77 added the type: feature label Apr 26, 2024

holgerd77 reviewed Apr 26, 2024

View reviewed changes

packages/common/src/eips.ts Outdated Show resolved Hide resolved

acolytec3 added 3 commits April 26, 2024 13:05

Add vm.runBlock

70ba18f

add tests with requests

a5264d9

Add buildblock changes and tests

0bf6387

acolytec3 added package: vm package: evm labels Apr 26, 2024

acolytec3 added 8 commits April 26, 2024 13:54

lint

b118e5d

remove sorting function

040f779

Add order check for requests when generating trie

fc191bb

More fixes

b203722

remove generic

6d7e676

Merge remote-tracking branch 'origin/master' into eip-7685 [no ci]

ad255da

in flight fromValuesArray changes [no ci]

de4964c

update min hardfork to cancun [no ci]

39aa2e0

acolytec3 added 5 commits April 26, 2024 21:37

Throw on invalid requestsRoot [no ci]

2781708

add scaffolding for pending requests in pendingBlock

52823c2

Merge remote-tracking branch 'origin/master' into eip-7685 [no ci]

d6f70ad

Update fromRPC constructors and toJSON methods

a18cd5a

Add requests to JsonRpcBlock

96ecc0a

g11tech reviewed Apr 28, 2024

View reviewed changes

update runBlock/buildBlock and tests

71be096

g11tech reviewed Apr 29, 2024

View reviewed changes

g11tech previously approved these changes Apr 29, 2024

View reviewed changes

acolytec3 added 2 commits April 29, 2024 12:48

Merge remote-tracking branch 'origin/master' into eip-7685

9914dc8

Remove obsolete references

1aa3bb3

acolytec3 dismissed g11tech’s stale review via 1aa3bb3 April 29, 2024 16:50

acolytec3 marked this pull request as ready for review April 29, 2024 16:51

acolytec3 requested a review from g11tech April 29, 2024 16:51

g11tech previously approved these changes Apr 29, 2024

View reviewed changes

fix hex typing

cd23545

acolytec3 dismissed g11tech’s stale review via cd23545 April 29, 2024 17:04

Merge remote-tracking branch 'origin/master' into eip-7685

0d6feaf

g11tech previously approved these changes Apr 29, 2024

View reviewed changes

Check for 7685 before adding requests

6f07126

acolytec3 dismissed g11tech’s stale review via 6f07126 April 29, 2024 19:18

jochem-brouwer reviewed Apr 29, 2024

View reviewed changes

address feedback

9a8258f

holgerd77 reviewed Apr 30, 2024

View reviewed changes

address feedback

acd9c39

holgerd77 approved these changes Apr 30, 2024

View reviewed changes

holgerd77 merged commit 55a8e66 into master Apr 30, 2024
33 of 34 checks passed

holgerd77 deleted the eip-7685 branch April 30, 2024 18:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement EIP 7685 #3372

Implement EIP 7685 #3372

acolytec3 commented Apr 25, 2024 •

edited

jochem-brouwer left a comment

g11tech commented Apr 28, 2024

g11tech Apr 28, 2024 •

edited

g11tech Apr 29, 2024

jochem-brouwer Apr 29, 2024

g11tech left a comment

jochem-brouwer left a comment

jochem-brouwer Apr 29, 2024

acolytec3 Apr 30, 2024

jochem-brouwer Apr 29, 2024

acolytec3 Apr 30, 2024 •

edited

jochem-brouwer Apr 30, 2024

jochem-brouwer Apr 29, 2024

acolytec3 Apr 30, 2024

jochem-brouwer Apr 30, 2024

acolytec3 Apr 30, 2024

jochem-brouwer Apr 29, 2024

holgerd77 left a comment

holgerd77 Apr 30, 2024

acolytec3 Apr 30, 2024

holgerd77 Apr 30, 2024

acolytec3 Apr 30, 2024

holgerd77 Apr 30, 2024

holgerd77 Apr 30, 2024

holgerd77 Apr 30, 2024

holgerd77 Apr 30, 2024

holgerd77 Apr 30, 2024

acolytec3 Apr 30, 2024

jochem-brouwer Apr 30, 2024

scorbajio May 1, 2024

holgerd77 left a comment


		// TODO: Add in code to accumulate partial withdrawals (EIP-7002)

		if (requests.length > 1) {


		// TODO: Add in code to accumulate deposits (EIP-6110)

		// TODO: Add in code to accumulate partial withdrawals (EIP-7002)

Implement EIP 7685 #3372

Implement EIP 7685 #3372

Conversation

acolytec3 commented Apr 25, 2024 • edited

jochem-brouwer left a comment

Choose a reason for hiding this comment

g11tech commented Apr 28, 2024

g11tech Apr 28, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

g11tech left a comment

Choose a reason for hiding this comment

jochem-brouwer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acolytec3 Apr 30, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holgerd77 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holgerd77 left a comment

Choose a reason for hiding this comment

acolytec3 commented Apr 25, 2024 •

edited

g11tech Apr 28, 2024 •

edited

acolytec3 Apr 30, 2024 •

edited