Client: hive Cancun fixes #3099

jochem-brouwer · 2023-10-10T10:53:24Z

The goal of this PR is to fix most (all?) of failing hive cancun tests. Before this PR: 55 failing tests. After this PR: 30 failing tests

To run:

./hive --client ethereumjs --sim ethereum/engine --sim.parallelism 8 --sim.limit="engine-cancun"

(change parallelism and ensure hive is at latest master)

Fixes:

./hive --client ethereumjs --sim ethereum/engine --sim.parallelism 8 --sim.limit="engine-cancun/Bad Hash on NewPayload" 104e6e8, this fixes 3 tests.
Inconsistent Head in ForkchoiceState in b525804, this commit fixes 10 tests. If recursivelyFindParents hits genesis block then we should stop finding more parents, we now have a full reorg back to genesis (?)
Pre-Merge Fork Number > 0 : Off by one check in common f2aedca
Unique PayloadID - Withdrawals: Ensure that different withdrawals result in a different payload ID 14b60a4

Open failing tests (with notes):

Invalid PayloadAttributes: Missing BeaconRoot -> This test seems wrong, it FCUs without a parentBlockBeaconRoot but then expects us to report that blockhash as head hash
Invalid Missing Ancestor ReOrg -> have to do with reorgs, will open a separate PR for this.

…[no ci]

…nesis [no ci]

codecov · 2023-10-16T10:00:07Z

Codecov Report

Merging #3099 (53574ed) into master (d282e2e) will decrease coverage by 0.08%.
The diff coverage is 82.00%.

Additional details and impacted files

Flag	Coverage Δ
block	`88.78% <ø> (ø)`
blockchain	`92.27% <62.06%> (-0.35%)`	⬇️
client	`87.59% <90.00%> (-0.14%)`	⬇️
common	`98.19% <100.00%> (ø)`
ethash	`∅ <ø> (∅)`
evm	`71.87% <ø> (ø)`
rlp	`∅ <ø> (?)`
statemanager	`90.30% <ø> (ø)`
trie	`90.32% <ø> (ø)`
tx	`96.36% <ø> (ø)`
util	`86.97% <ø> (ø)`
vm	`76.21% <ø> (ø)`

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

…o ci]

jochem-brouwer · 2023-10-16T11:56:32Z

A client test will fail, this expects us to set finalized/safe to the genesis block if it is not yet set. I am not sure if this is correct. Remainder of this is ready for review, other failing hive tests have to do with reorgs, will open a separate PR for that.

jochem-brouwer · 2023-10-16T12:42:12Z

packages/common/src/common.ts

@@ -384,7 +384,7 @@ export class Common {
      if (mergeIndex >= 0 && td !== undefined && td !== null) {
        if (hfIndex >= mergeIndex && BigInt(hfs[mergeIndex].ttd!) > td) {
          throw Error('Maximum HF determined by total difficulty is lower than the block number HF')
-        } else if (hfIndex < mergeIndex && BigInt(hfs[mergeIndex].ttd!) <= td) {
+        } else if (hfIndex < mergeIndex && BigInt(hfs[mergeIndex].ttd!) < td) {


Ok, this change triggers a Common test. I am pretty sure this is ok, we should check for < not <=?

This feels like it should be fine to me. I guess theoretically our hardfork could have the same total difficulty as the merge TTD if the intent is for them to occur on the same exact block. @g11tech, would you agree?

Ah and I forgot to note: this change was triggered by a hive test with thus this exact scenario of hitting the TTD exactly.

acolytec3

LGTM. I've previously verified that the specified hive tests do indeed pass.

acolytec3 · 2023-10-17T12:20:50Z

packages/common/src/common.ts

@@ -384,7 +384,7 @@ export class Common {
      if (mergeIndex >= 0 && td !== undefined && td !== null) {
        if (hfIndex >= mergeIndex && BigInt(hfs[mergeIndex].ttd!) > td) {
          throw Error('Maximum HF determined by total difficulty is lower than the block number HF')
-        } else if (hfIndex < mergeIndex && BigInt(hfs[mergeIndex].ttd!) <= td) {
+        } else if (hfIndex < mergeIndex && BigInt(hfs[mergeIndex].ttd!) < td) {


This feels like it should be fine to me. I guess theoretically our hardfork could have the same total difficulty as the merge TTD if the intent is for them to occur on the same exact block. @g11tech, would you agree?

jochem-brouwer · 2023-10-21T11:18:05Z

Hmm, I have reran the client tests twice now since I thought this would be a random timeout, but maybe this is not the case (have just retriggered, if it does not pass then I should investigate)

jochem-brouwer · 2023-10-21T12:41:10Z

Ok, tests seems failing into a context which I thought was not relevant, will take a look!

g11tech · 2023-10-22T09:17:50Z

packages/client/src/blockchain/chain.ts

    // finalized and safe are always blocks since they have to have valid execution
    // before they can be saved in chain
-    headers.finalized = (await this.getCanonicalFinalizedBlock()).header
-    headers.safe = (await this.getCanonicalSafeBlock()).header
+    if (blocks.finalized !== null) {


Suggested change

if (blocks.finalized !== null) {

headers.finalized = blocks.finalized?.header ?? null

should be cleaner and simlarly for safe

jochem-brouwer · 2023-10-22T16:39:10Z

packages/client/src/blockchain/chain.ts

-    headers.finalized = (await this.getCanonicalFinalizedBlock()).header
-    headers.safe = (await this.getCanonicalSafeBlock()).header
+    headers.finalized = blocks.finalized?.header ?? null
+    headers.safe = blocks.safe?.header ?? null
    headers.vm = (await this.getCanonicalVmHead()).header

    blocks.latest = await this.getCanonicalHeadBlock()


Ok, so in this file if one first does gets all the blocks and then the headers, then a sync test fails. If one updates this to the original (see 586868b ) then it passes. Test in question is integration/lightclient.spec.ts

…d call

holgerd77 · 2023-10-23T07:58:01Z

Reading from the thread it seems this can be merged and will therefore do. If I accidentally merged with some todo still left please eventually just do in a small follow-up PR, but seems convenient to have this in.

holgerd77

LGTM

holgerd77 · 2023-10-23T07:59:16Z

(so: pretty cool work with so many Hive fixes! 🤩 🎉)

jochem-brouwer · 2023-10-23T07:59:16Z

Great, thanks 😄 🙏

packages/client/src/blockchain/chain.ts

client/engine: newPayload match spec invalid blockhash [no ci]

104e6e8

jochem-brouwer added PR state: WIP package: client labels Oct 10, 2023

jochem-brouwer marked this pull request as draft October 10, 2023 10:53

jochem-brouwer added 7 commits October 10, 2023 15:10

client/engine: add check for genesis block in recursivelyFindParents …

b525804

…[no ci]

Merge branch 'master' into hive-cancun

3ba3799

blockchain/client: ensure safe/finalized blocks have to be set [no ci]

9660573

blockchain: accept putting the same genesis block as in blockchain ge…

ce1f9d0

…nesis [no ci]

client: default to genesis if safe/finalized is not set in chain [no ci]

77f9f53

client: fix log on chain pointers [no ci]

a616855

Merge branch 'master' into hive-cancun

0b3cc67

jochem-brouwer added 3 commits October 16, 2023 13:07

common: fix off-by-one check [no ci]

f2aedca

client/miner: ensure different payloadID for different withdrawals [n…

14b60a4

…o ci]

blockchain: fix test

3c2f820

jochem-brouwer marked this pull request as ready for review October 16, 2023 11:56

jochem-brouwer added PR state: needs review eval: needs discussion labels Oct 16, 2023

jochem-brouwer requested a review from g11tech October 16, 2023 11:57

jochem-brouwer commented Oct 16, 2023

View reviewed changes

acolytec3 and others added 3 commits October 17, 2023 08:44

Fix common test

fd3ce1e

Merge branch 'master' into hive-cancun

5bc2fac

client: update forkchoice test

80c2624

jochem-brouwer removed PR state: WIP eval: needs discussion labels Oct 20, 2023

acolytec3 previously approved these changes Oct 21, 2023

View reviewed changes

jochem-brouwer marked this pull request as draft October 21, 2023 13:22

jochem-brouwer added PR state: WIP and removed PR state: needs review labels Oct 21, 2023

g11tech reviewed Oct 22, 2023

View reviewed changes

jochem-brouwer added 3 commits October 22, 2023 15:49

Merge branch 'master' into hive-cancun

3f85b82

client: address review

da48eca

client: ensure chain head updates correctly

586868b

jochem-brouwer dismissed acolytec3’s stale review via 586868b October 22, 2023 16:13

jochem-brouwer marked this pull request as ready for review October 22, 2023 16:37

jochem-brouwer added PR state: needs review and removed PR state: WIP labels Oct 22, 2023

jochem-brouwer commented Oct 22, 2023

View reviewed changes

client: reorder blocks, headers assignments and avoid duplicate VmHea…

53574ed

…d call

holgerd77 approved these changes Oct 23, 2023

View reviewed changes

holgerd77 merged commit ff14395 into master Oct 23, 2023
43 checks passed

holgerd77 deleted the hive-cancun branch October 23, 2023 07:58

jochem-brouwer added package: blockchain package: common labels Oct 23, 2023

holgerd77 added type: bug type: enhancement package: common and removed package: common labels Oct 23, 2023

holgerd77 reviewed Oct 23, 2023

View reviewed changes

packages/client/src/blockchain/chain.ts Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Client: hive Cancun fixes #3099

Client: hive Cancun fixes #3099

jochem-brouwer commented Oct 10, 2023 •

edited

Loading

codecov bot commented Oct 16, 2023 •

edited

Loading

jochem-brouwer commented Oct 16, 2023

jochem-brouwer Oct 16, 2023

acolytec3 Oct 17, 2023

jochem-brouwer Oct 21, 2023

acolytec3 left a comment

acolytec3 Oct 17, 2023

jochem-brouwer commented Oct 21, 2023

jochem-brouwer commented Oct 21, 2023

g11tech Oct 22, 2023

jochem-brouwer Oct 22, 2023

holgerd77 commented Oct 23, 2023

holgerd77 left a comment

holgerd77 commented Oct 23, 2023

jochem-brouwer commented Oct 23, 2023

	if (blocks.finalized !== null) {
	headers.finalized = blocks.finalized?.header ?? null

Client: hive Cancun fixes #3099

Client: hive Cancun fixes #3099

Conversation

jochem-brouwer commented Oct 10, 2023 • edited Loading

codecov bot commented Oct 16, 2023 • edited Loading

Codecov Report

jochem-brouwer commented Oct 16, 2023

jochem-brouwer Oct 16, 2023

Choose a reason for hiding this comment

acolytec3 Oct 17, 2023

Choose a reason for hiding this comment

jochem-brouwer Oct 21, 2023

Choose a reason for hiding this comment

acolytec3 left a comment

Choose a reason for hiding this comment

acolytec3 Oct 17, 2023

Choose a reason for hiding this comment

jochem-brouwer commented Oct 21, 2023

jochem-brouwer commented Oct 21, 2023

g11tech Oct 22, 2023

Choose a reason for hiding this comment

jochem-brouwer Oct 22, 2023

Choose a reason for hiding this comment

holgerd77 commented Oct 23, 2023

holgerd77 left a comment

Choose a reason for hiding this comment

holgerd77 commented Oct 23, 2023

jochem-brouwer commented Oct 23, 2023

jochem-brouwer commented Oct 10, 2023 •

edited

Loading

codecov bot commented Oct 16, 2023 •

edited

Loading