Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Client: hive Cancun fixes #3099

Merged
merged 18 commits into from
Oct 23, 2023
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
104e6e8
client/engine: newPayload match spec invalid blockhash [no ci]
jochem-brouwer Oct 10, 2023
b525804
client/engine: add check for genesis block in recursivelyFindParents …
jochem-brouwer Oct 10, 2023
3ba3799
Merge branch 'master' into hive-cancun
jochem-brouwer Oct 10, 2023
9660573
blockchain/client: ensure safe/finalized blocks have to be set [no ci]
jochem-brouwer Oct 11, 2023
ce1f9d0
blockchain: accept putting the same genesis block as in blockchain ge…
jochem-brouwer Oct 12, 2023
77f9f53
client: default to genesis if safe/finalized is not set in chain [no ci]
jochem-brouwer Oct 12, 2023
a616855
client: fix log on chain pointers [no ci]
jochem-brouwer Oct 12, 2023
0b3cc67
Merge branch 'master' into hive-cancun
jochem-brouwer Oct 16, 2023
f2aedca
common: fix off-by-one check [no ci]
jochem-brouwer Oct 16, 2023
14b60a4
client/miner: ensure different payloadID for different withdrawals [n…
jochem-brouwer Oct 16, 2023
3c2f820
blockchain: fix test
jochem-brouwer Oct 16, 2023
fd3ce1e
Fix common test
acolytec3 Oct 17, 2023
5bc2fac
Merge branch 'master' into hive-cancun
jochem-brouwer Oct 20, 2023
80c2624
client: update forkchoice test
jochem-brouwer Oct 20, 2023
3f85b82
Merge branch 'master' into hive-cancun
jochem-brouwer Oct 22, 2023
da48eca
client: address review
jochem-brouwer Oct 22, 2023
586868b
client: ensure chain head updates correctly
jochem-brouwer Oct 22, 2023
53574ed
client: reorder blocks, headers assignments and avoid duplicate VmHea…
gabrocheleau Oct 23, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 29 additions & 5 deletions packages/blockchain/src/blockchain.ts
Original file line number Diff line number Diff line change
Expand Up @@ -312,13 +312,31 @@
*/
async getIteratorHead(name = 'vm'): Promise<Block> {
return this.runWithLock<Block>(async () => {
// if the head is not found return the genesis hash
const hash = this._heads[name] ?? this.genesisBlock.hash()
const block = await this.getBlock(hash)
return block
return (await this.getHead(name, false))!
})
}

/**
* This method differs from `getIteratorHead`. If the head is not found, it returns `undefined`.
* @param name - Optional name of the iterator head (default: 'vm')
* @returns
*/
async getIteratorHeadSafe(name = 'vm'): Promise<Block | undefined> {
return this.runWithLock<Block | undefined>(async () => {
return this.getHead(name, true)
})
}

Check warning on line 328 in packages/blockchain/src/blockchain.ts

View check run for this annotation

Codecov / codecov/patch

packages/blockchain/src/blockchain.ts#L325-L328

Added lines #L325 - L328 were not covered by tests

private async getHead(name: string, returnUndefinedIfNotSet: boolean = false) {
const headHash = this._heads[name]
if (headHash === undefined && returnUndefinedIfNotSet) {
return undefined
}

Check warning on line 334 in packages/blockchain/src/blockchain.ts

View check run for this annotation

Codecov / codecov/patch

packages/blockchain/src/blockchain.ts#L333-L334

Added lines #L333 - L334 were not covered by tests
const hash = this._heads[name] ?? this.genesisBlock.hash()
const block = await this.getBlock(hash)
return block
}

/**
* Returns the latest header in the canonical chain.
*/
Expand Down Expand Up @@ -450,7 +468,13 @@

// we cannot overwrite the Genesis block after initializing the Blockchain
if (isGenesis) {
throw new Error('Cannot put a genesis block: create a new Blockchain')
if (equalsBytes(this.genesisBlock.hash(), block.hash())) {
// Try to re-put the exisiting genesis block, accept this
return
}

Check warning on line 474 in packages/blockchain/src/blockchain.ts

View check run for this annotation

Codecov / codecov/patch

packages/blockchain/src/blockchain.ts#L472-L474

Added lines #L472 - L474 were not covered by tests
throw new Error(
'Cannot put a different genesis block than current blockchain genesis: create a new Blockchain'
)
}

const { header } = block
Expand Down
2 changes: 1 addition & 1 deletion packages/blockchain/test/index.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -844,7 +844,7 @@ describe('initialization tests', () => {
} catch (e: any) {
assert.equal(
e.message,
'Cannot put a genesis block: create a new Blockchain',
'Cannot put a different genesis block than current blockchain genesis: create a new Blockchain',
'putting a genesis block did throw (otherGenesisBlock not found in chain)'
)
}
Expand Down
32 changes: 20 additions & 12 deletions packages/client/src/blockchain/chain.ts
Original file line number Diff line number Diff line change
Expand Up @@ -294,18 +294,26 @@ export class Chain {
height: BIGINT_0,
}

headers.latest = await this.getCanonicalHeadHeader()
blocks.latest = await this.getCanonicalHeadBlock()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

blocks.finalized = (await this.getCanonicalFinalizedBlock()) ?? null
blocks.safe = (await this.getCanonicalSafeBlock()) ?? null
blocks.vm = await this.getCanonicalVmHead()

headers.latest = blocks.latest.header
// 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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

should be cleaner and simlarly for safe

headers.finalized = blocks.finalized.header
} else {
headers.finalized = null
}
if (blocks.safe !== null) {
headers.safe = blocks.safe.header
} else {
headers.safe = null
}
headers.vm = (await this.getCanonicalVmHead()).header

blocks.latest = await this.getCanonicalHeadBlock()
blocks.finalized = await this.getCanonicalFinalizedBlock()
blocks.safe = await this.getCanonicalSafeBlock()
blocks.vm = await this.getCanonicalVmHead()

headers.height = headers.latest.number
blocks.height = blocks.latest.header.number

Expand Down Expand Up @@ -513,17 +521,17 @@ export class Chain {
/**
* Gets the latest block in the canonical chain
*/
async getCanonicalSafeBlock(): Promise<Block> {
async getCanonicalSafeBlock(): Promise<Block | undefined> {
holgerd77 marked this conversation as resolved.
Show resolved Hide resolved
if (!this.opened) throw new Error('Chain closed')
return this.blockchain.getIteratorHead('safe')
return this.blockchain.getIteratorHeadSafe('safe')
}

/**
* Gets the latest block in the canonical chain
*/
async getCanonicalFinalizedBlock(): Promise<Block> {
async getCanonicalFinalizedBlock(): Promise<Block | undefined> {
if (!this.opened) throw new Error('Chain closed')
return this.blockchain.getIteratorHead('finalized')
return this.blockchain.getIteratorHeadSafe('finalized')
}

/**
Expand Down
8 changes: 4 additions & 4 deletions packages/client/src/execution/vmexecution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -229,12 +229,12 @@ export class VMExecution extends Execution {
): Promise<void> {
return this.runWithLock<void>(async () => {
const vmHeadBlock = blocks[blocks.length - 1]
const chainPointers: [string, Block | null][] = [
const chainPointers: [string, Block][] = [
['vmHeadBlock', vmHeadBlock],
// if safeBlock is not provided, the current safeBlock of chain should be used
// which is genesisBlock if it has never been set for e.g.
['safeBlock', safeBlock ?? this.chain.blocks.safe],
['finalizedBlock', finalizedBlock ?? this.chain.blocks.finalized],
['safeBlock', safeBlock ?? this.chain.blocks.safe ?? this.chain.genesis],
['finalizedBlock', finalizedBlock ?? this.chain.blocks.finalized ?? this.chain.genesis],
]

let isSortedDesc = true
Expand All @@ -258,7 +258,7 @@ export class VMExecution extends Execution {

if (isSortedDesc === false) {
throw Error(
`headBlock=${vmHeadBlock?.header.number} should be >= safeBlock=${safeBlock?.header.number} should be >= finalizedBlock=${finalizedBlock?.header.number}`
`headBlock=${chainPointers[0][1].header.number} should be >= safeBlock=${chainPointers[1][1]?.header.number} should be >= finalizedBlock=${chainPointers[2][1]?.header.number}`
)
}
// skip emitting the chain update event as we will manually do it
Expand Down
20 changes: 19 additions & 1 deletion packages/client/src/miner/pendingBlock.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { Hardfork } from '@ethereumjs/common'
import { BlobEIP4844Transaction } from '@ethereumjs/tx'
import {
Address,
BIGINT_1,
BIGINT_2,
TypeOutput,
Expand Down Expand Up @@ -130,6 +131,22 @@ export class PendingBlock {
toType(parentBeaconBlockRoot!, TypeOutput.Uint8Array) ?? zeros(32)
const coinbaseBuf = toType(coinbase ?? zeros(20), TypeOutput.Uint8Array)

let withdrawalsBuf = zeros(0)

if (withdrawals !== undefined) {
const withdrawalsBufTemp: Uint8Array[] = []
for (const withdrawal of withdrawals) {
const indexBuf = bigIntToUnpaddedBytes(toType(withdrawal.index ?? 0, TypeOutput.BigInt))
const validatorIndex = bigIntToUnpaddedBytes(
toType(withdrawal.validatorIndex ?? 0, TypeOutput.BigInt)
)
const address = toType(withdrawal.address ?? Address.zero(), TypeOutput.Uint8Array)
const amount = bigIntToUnpaddedBytes(toType(withdrawal.amount ?? 0, TypeOutput.BigInt))
withdrawalsBufTemp.push(concatBytes(indexBuf, validatorIndex, address, amount))
}
withdrawalsBuf = concatBytes(...withdrawalsBufTemp)
}

const payloadIdBytes = toBytes(
keccak256(
concatBytes(
Expand All @@ -138,7 +155,8 @@ export class PendingBlock {
timestampBuf,
gasLimitBuf,
parentBeaconBlockRootBuf,
coinbaseBuf
coinbaseBuf,
withdrawalsBuf
)
).subarray(0, 8)
)
Expand Down
3 changes: 3 additions & 0 deletions packages/client/src/rpc/error-code.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,6 @@ export const validEngineCodes = [
UNSUPPORTED_FORK,
UNKNOWN_PAYLOAD,
]

// Errors for the ETH protocol
export const INVALID_BLOCK = -39001
21 changes: 18 additions & 3 deletions packages/client/src/rpc/helpers.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { BIGINT_0, bigIntToHex, bytesToHex, intToHex } from '@ethereumjs/util'

import { INTERNAL_ERROR, INVALID_PARAMS } from './error-code'
import { INTERNAL_ERROR, INVALID_BLOCK, INVALID_PARAMS } from './error-code'

import type { Chain } from '../blockchain'
import type { Block } from '@ethereumjs/block'
Expand Down Expand Up @@ -73,6 +73,7 @@ export const getBlockByOption = async (blockOpt: string, chain: Chain) => {
}

let block: Block
let tempBlock: Block | undefined // Used in `safe` and `finalized` blocks
const latest = chain.blocks.latest ?? (await chain.getCanonicalHeadBlock())

switch (blockOpt) {
Expand All @@ -83,10 +84,24 @@ export const getBlockByOption = async (blockOpt: string, chain: Chain) => {
block = latest
break
case 'safe':
block = chain.blocks.safe ?? (await chain.getCanonicalSafeBlock())
tempBlock = chain.blocks.safe ?? (await chain.getCanonicalSafeBlock())
if (tempBlock === null || tempBlock === undefined) {
throw {
message: 'Unknown block',
code: INVALID_BLOCK,
}
}
block = tempBlock
break
case 'finalized':
block = chain.blocks.finalized ?? (await chain.getCanonicalFinalizedBlock())
tempBlock = chain.blocks.finalized ?? (await chain.getCanonicalFinalizedBlock())
if (tempBlock === null || tempBlock === undefined) {
throw {
message: 'Unknown block',
code: INVALID_BLOCK,
}
}
block = tempBlock
break
default: {
const blockNumber = BigInt(blockOpt)
Expand Down
15 changes: 14 additions & 1 deletion packages/client/src/rpc/modules/engine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,11 @@ const recursivelyFindParents = async (
)
parentBlocks.push(block)

if (block.isGenesis()) {
// In case we hit the genesis block we should stop finding additional parents
break
}

// throw error if lookups have exceeded maxDepth
if (parentBlocks.length > maxDepth) {
throw Error(`recursivelyFindParents lookups deeper than maxDepth=${maxDepth}`)
Expand Down Expand Up @@ -998,6 +1003,7 @@ export class Engine {
const newPayloadRes = await this.newPayload(params)
if (newPayloadRes.status === Status.INVALID_BLOCK_HASH) {
newPayloadRes.status = Status.INVALID
newPayloadRes.latestValidHash = null
}
return newPayloadRes
}
Expand All @@ -1015,6 +1021,7 @@ export class Engine {
const newPayloadRes = await this.newPayload(params)
if (newPayloadRes.status === Status.INVALID_BLOCK_HASH) {
newPayloadRes.status = Status.INVALID
newPayloadRes.latestValidHash = null
}
return newPayloadRes
}
Expand Down Expand Up @@ -1225,7 +1232,13 @@ export class Engine {
}
}
this.service.txPool.removeNewBlockTxs(blocks)
} else {

const isPrevSynced = this.chain.config.synchronized
this.config.updateSynchronizedState(headBlock.header)
if (!isPrevSynced && this.chain.config.synchronized) {
this.service.txPool.checkRunState()
}
} else if (!headBlock.isGenesis()) {
// even if the vmHead is same still validations need to be done regarding the correctness
// of the sequence and canonical-ity
try {
Expand Down
2 changes: 1 addition & 1 deletion packages/common/src/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

throw Error('HF determined by block number is lower than the minimum total difficulty HF')
}
}
Expand Down