Skip to content

Commit ed56780

Browse files
holgerd77acolytec3am1r021jochem-brouwer
authored
Fix Client Best Peer Selection based on TD Check (#3950)
* Fix client best peer selection being based on a TD check * fix test * Increase test timeout * Make peer selection a bit more fine grained, to not unnecessarily drop pure PoW/Ethash networks * Small spelling fix * Another client integration miner test timeout increase * Removed unnecessary and misleading clique consensus config from client miner integration test setup * client: revert change in test * client: ensure pow test runs in pow mode * client: pick random eth handshaken peer instead of the first * client: fullsync integration tests, add comment/warning * client: miner test: restore timeout * client: ensure TD is taken into account on non-merge clique networks when selecting peer * client: longer miner test timeout --------- Co-authored-by: acolytec3 <[email protected]> Co-authored-by: Amir <[email protected]> Co-authored-by: Jochem Brouwer <[email protected]>
1 parent 6ac1223 commit ed56780

File tree

4 files changed

+122
-49
lines changed

4 files changed

+122
-49
lines changed

‎packages/client/src/sync/fullsync.ts

Lines changed: 26 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { Hardfork } from '@ethereumjs/common'
1+
import { ConsensusAlgorithm, Hardfork } from '@ethereumjs/common'
22
import { BIGINT_0, BIGINT_1, equalsBytes } from '@ethereumjs/util'
33

44
import { Event } from '../types.ts'
@@ -118,24 +118,38 @@ export class FullSynchronizer extends Synchronizer {
118118

119119
/**
120120
* Finds the best peer to sync with. We will synchronize to this peer's
121-
* blockchain. Returns null if no valid peer is found
121+
* blockchain. Returns null if no valid peer is found.
122122
*/
123123
async best(): Promise<Peer | undefined> {
124-
let best
125124
const peers = this.pool.peers.filter(this.syncable.bind(this))
126125
if (peers.length < this.config.minPeers && !this.forceSync) return
127-
for (const peer of peers) {
128-
if (peer.eth?.status !== undefined) {
129-
const td = peer.eth.status.td
130-
if (
131-
(!best && td >= this.chain.blocks.td) ||
132-
(best && best.eth && best.eth.status.td < td)
133-
) {
134-
best = peer
126+
127+
const consensus = this.config.chainCommon.consensusAlgorithm()
128+
129+
if (
130+
(consensus === ConsensusAlgorithm.Ethash || consensus === ConsensusAlgorithm.Clique) &&
131+
this.config.chainCommon.hardforkBlock(Hardfork.Paris) === null
132+
) {
133+
// For pure non-Merge HF Ethash/Clique chains we want to select the peer with the highest TD
134+
let best
135+
for (const peer of peers) {
136+
if (peer.eth?.status !== undefined) {
137+
const td = peer.eth.status.td
138+
if (
139+
(!best && td >= this.chain.blocks.td) ||
140+
(best && best.eth && best.eth.status.td < td)
141+
) {
142+
best = peer
143+
}
135144
}
136145
}
146+
return best
147+
} else {
148+
// Take a random peer which advertises the eth protocol (and did handshake with, `status !== undefined`)
149+
const peersWithEth = peers.filter((peer) => peer.eth?.status !== undefined)
150+
// If the array is empty, will return `peersWithEth[0]`, so `undefined`.
151+
return peersWithEth[Math.floor(Math.random() * peersWithEth.length)]
137152
}
138-
return best
139153
}
140154

141155
/**

‎packages/client/test/integration/fullsync.spec.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,15 @@ describe('should sync with best peer', async () => {
5050
await localServer.discover('remotePeer2', '127.0.0.3')
5151

5252
localService.config.events.on(Event.SYNC_SYNCHRONIZED, async () => {
53+
// TODO: this test, if `localService.chain.blocks.height !== BigInt(10)`
54+
// will NOT call the `destroy` methods. This will hang the entire test (and thus also CI)
55+
// Note: this `SYNC.SYNCHRONIZED` event can be called multiple times.
56+
// Therefore, even if the first peer chosen is not the best peer (height: 7)
57+
// it could be called afterwards when the next "best" peer chosen is actually height: 10
58+
// Tested locally, this seems to eventually always call destroy()
59+
// NOTE: we can set a timeout on the destroys, but this essentially fails the test
60+
// because somehow this synchronizer has not found the `height: 10` peer and synced
61+
// to it, while it still should sync to it
5362
if (localService.chain.blocks.height === BigInt(10)) {
5463
it('should sync with best peer', () => {
5564
assert.isTrue(true, 'synced with best peer')

‎packages/client/test/integration/miner.spec.ts

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -18,12 +18,6 @@ import type { EthereumClient } from '../../src/index.ts'
1818

1919
async function setupDevnet(prefundAddress: Address) {
2020
const addr = prefundAddress.toString().slice(2)
21-
const consensusConfig = {
22-
clique: {
23-
period: 1,
24-
epoch: 30000,
25-
},
26-
}
2721
const defaultChainData = {
2822
config: {
2923
chainId: 123456,
@@ -38,7 +32,6 @@ async function setupDevnet(prefundAddress: Address) {
3832
istanbulBlock: 0,
3933
berlinBlock: 0,
4034
londonBlock: 0,
41-
...consensusConfig,
4235
},
4336
nonce: '0x0',
4437
timestamp: '0x614b3731',
@@ -115,5 +108,5 @@ describe('should mine blocks while a peer stays connected to tip of chain', () =
115108
}
116109
})
117110
})
118-
}, 60000)
111+
}, 200000)
119112
})

‎packages/client/test/sync/fullsync.spec.ts

Lines changed: 86 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,66 @@ import { createBlock } from '@ethereumjs/block'
22
import * as td from 'testdouble'
33
import { assert, describe, it, vi } from 'vitest'
44

5+
import { Common } from '@ethereumjs/common'
6+
import type { PrefixedHexString } from '@ethereumjs/util'
57
import { Chain } from '../../src/blockchain/index.ts'
68
import { Config } from '../../src/config.ts'
79
import { Event } from '../../src/types.ts'
810
import { wait } from '../integration/util.ts'
911

12+
const powConfig = {
13+
name: 'testnet',
14+
chainId: 12345,
15+
defaultHardfork: 'byzantium',
16+
consensus: {
17+
type: 'pow',
18+
algorithm: 'ethash',
19+
},
20+
comment: 'PoW network [test]',
21+
url: '[TESTNET_URL]',
22+
genesis: {
23+
gasLimit: 1000000,
24+
difficulty: 1,
25+
nonce: '0xbb00000000000000' as PrefixedHexString,
26+
extraData: ('0x' + '00'.repeat(97)) as PrefixedHexString,
27+
},
28+
hardforks: [
29+
{
30+
name: 'chainstart',
31+
block: 0,
32+
},
33+
{
34+
name: 'homestead',
35+
block: 1,
36+
},
37+
{
38+
name: 'tangerineWhistle',
39+
block: 2,
40+
},
41+
{
42+
name: 'spuriousDragon',
43+
block: 3,
44+
},
45+
{
46+
name: 'byzantium',
47+
block: 4,
48+
},
49+
],
50+
bootstrapNodes: [],
51+
}
52+
53+
const powCommon = new Common({ chain: powConfig })
54+
const cliqueConfig = JSON.parse(JSON.stringify(powConfig))
55+
cliqueConfig.consensus = {
56+
type: 'poa',
57+
algorithm: 'clique',
58+
clique: {
59+
period: 15,
60+
epoch: 30000,
61+
},
62+
}
63+
const cliqueCommon = new Common({ chain: cliqueConfig })
64+
1065
describe('[FullSynchronizer]', async () => {
1166
const txPool: any = { removeNewBlockTxs: () => {}, checkRunState: () => {} }
1267
const execution: any = { run: () => {} }
@@ -88,36 +143,38 @@ describe('[FullSynchronizer]', async () => {
88143
await sync.close()
89144
})
90145

91-
it('should find best', async () => {
92-
const config = new Config({ accountCache: 10000, storageCache: 1000 })
93-
const pool = new PeerPool() as any
94-
const chain = await Chain.create({ config })
95-
const sync = new FullSynchronizer({
96-
config,
97-
interval: 1,
98-
pool,
99-
chain,
100-
txPool,
101-
execution,
102-
})
103-
;(sync as any).running = true
104-
const peers = [
105-
{ eth: { status: { td: BigInt(1) } }, inbound: false },
106-
{ eth: { status: { td: BigInt(2) } }, inbound: false },
107-
]
108-
;(sync as any).height = vi.fn((input) => {
109-
if (JSON.stringify(input) === JSON.stringify(peers[0]))
110-
return Promise.resolve(peers[0].eth.status.td)
111-
if (JSON.stringify(input) === JSON.stringify(peers[1]))
112-
return Promise.resolve(peers[1].eth.status.td)
146+
for (const common of [powCommon, cliqueCommon]) {
147+
it(`should find best (${common.consensusAlgorithm()})`, async () => {
148+
const config = new Config({ accountCache: 10000, storageCache: 1000, common })
149+
const pool = new PeerPool() as any
150+
const chain = await Chain.create({ config })
151+
const sync = new FullSynchronizer({
152+
config,
153+
interval: 1,
154+
pool,
155+
chain,
156+
txPool,
157+
execution,
158+
})
159+
;(sync as any).running = true
160+
const peers = [
161+
{ eth: { status: { td: BigInt(1) } }, inbound: false },
162+
{ eth: { status: { td: BigInt(2) } }, inbound: false },
163+
]
164+
;(sync as any).height = vi.fn((input) => {
165+
if (JSON.stringify(input) === JSON.stringify(peers[0]))
166+
return Promise.resolve(peers[0].eth.status.td)
167+
if (JSON.stringify(input) === JSON.stringify(peers[1]))
168+
return Promise.resolve(peers[1].eth.status.td)
169+
})
170+
;(sync as any).chain = { blocks: { td: BigInt(1) } }
171+
;(sync as any).pool = { peers }
172+
;(sync as any).forceSync = true
173+
assert.equal(await sync.best(), peers[1] as any, 'found best')
174+
await sync.stop()
175+
await sync.close()
113176
})
114-
;(sync as any).chain = { blocks: { td: BigInt(1) } }
115-
;(sync as any).pool = { peers }
116-
;(sync as any).forceSync = true
117-
assert.equal(await sync.best(), peers[1] as any, 'found best')
118-
await sync.stop()
119-
await sync.close()
120-
})
177+
}
121178

122179
it('should sync', async () => {
123180
const config = new Config({

0 commit comments

Comments
 (0)