Client: New Mechanism to Keep Peer Latest Block Updated #3354

holgerd77 · 2024-04-11T11:42:59Z

This PR adds a periodical check if the ETH status message exchanged by peers in the client is getting too old for then triggering a new handshake which should (theoretically) update the status information.

~~Not yet working, will continue tomorrow. Feedback welcome!~~

Initial description outdated, see updated post below

holgerd77 · 2024-04-15T14:39:39Z

Yeah, ok, I think this should basically do the job! 🙂

Took me several iterations to get this a bit to the point, the base implementation is now pretty clean I would say. So a more differentiated request on the latest/best block by a peer is now directly integrated into the peer.latest() method.

The peer remembers the last block from the best block request, on first round this is the block from STATUS (to get a first reference point, since there is no block number until then).

On second round onwards, a request is done either towards a block number from config.syncTargetHeight, or a forward calculated potential value going from the last best block and taking timestamps and the 12s slot period into account, whichever one is higher.

This logic should work both pre and post merge. I only tested this pre-merge on mainnet, this is then actually the first time we have something like a constant sync target height update on mainnet (since the latest() call is also used to update this syncTargetHeight in case this is higher than the existing one).

Two caveats remain:

Atm it is not safe to call this "out of bounds" in a regular interval on non-idle peers in peer pool, since this disturbs Fetcher requests as long as we do not support request IDs (so that ETH requests remain ordered, introduced with eth/66, we implemented only the "dummy" way of just reading this but throwing away). I have "solved" this now by hacking this into the different fetchers directly, within a Fetcher request() call, to make sure that ETH calls from getBlockHeaders() and from the fetcher calls do not interfer. This already works astonishingly well, also the interval of these calls seems bearable. So from my judgement despite being locally hacky I think we could live with this for practical reasons.
Along the work I refactored the location of the latest() method from being associated with the Sync module to move into the Peer. This was getting obvious that this is the right choice, all calling was going into peer already before and now with the per-peer-saving of the best block and stuff it became even more apparent that the method was misplaced in Sync (also considering that this update actually has nothing to do with the sync activity of the client). So I would very much like to keep this refactoring. Unfortunately this triggers a couple of tests to fail, mainly in the sync tests. This is solely due to the necessity of some changing in the mocking. Unfortunately I am not such a mocking expert, I seriously tried for 1 1/2 hours but did not get this to work. 😏 I worked my way through vi.mock(), but with no success yet (basically peer.latest() now needs to be mocked instead of sync.latest(). @scorbajio since you have done a lot with mocking, maybe you can also take a look, if you have the ressources? Feel free to directly push to this branch if you do!

holgerd77 · 2024-04-15T14:45:02Z

packages/client/src/net/peer/peer.ts

-    } else if (protocol.name === 'les') {
-      this.les = <BoundLesProtocol>bound
+    } else {
+      throw new Error(`addProtocol: ${protocol.name} protocol not supported`)
    }


This looks wild but is only a small local refactor making things a bit cleaner. 🙂

holgerd77 · 2024-04-15T14:47:09Z

packages/client/src/net/peerpool.ts

+        p.idle = true
+      }
+    }
+  }


I generally would think it would make more sense to have all (or: most, so except for the first one) latest() updates being triggered from here. Atm this is not very effective though, since we can only call on idle peers (see reasoning in separate comment) and mostly peers are not idle and so this is not getting called on peers very often in the current status quo.

…ly on protocol binding, small local refactor

…sendStatus for devp2p protocols

…reparation)

…to move from Sync -> Peer

…atest() call on peers in peer pool

…nymore)

…m-to-redo-peer-handshake-for-status-update

…e-for-status-update

codecov · 2024-04-18T00:10:05Z

Codecov Report

Attention: Patch coverage is 57.51634% with 65 lines in your changes are missing coverage. Please review.

Project coverage is 84.95%. Comparing base (4be68d2) to head (0117083).
Report is 14 commits behind head on master.

❗ Current head 0117083 differs from pull request most recent head 4e7bd84. Consider uploading reports for the commit 4e7bd84 to get more accurate results

Additional details and impacted files

Flag	Coverage Δ
block	`?`
blockchain	`?`
client	`84.72% <57.51%> (-0.20%)`	⬇️
common	`?`
devp2p	`?`
evm	`?`
genesis	`?`
statemanager	`?`
trie	`?`
tx	`85.82% <ø> (+0.11%)`	⬆️
util	`?`
vm	`?`
wallet	`87.25% <ø> (ø)`

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

…e-for-status-update

g11tech

lgtm

jochem-brouwer

Got one question, LGTM in general

jochem-brouwer · 2024-04-22T16:57:33Z

packages/client/src/net/peer/peer.ts

+      const bestHeaderNum = this.eth!.updatedBestHeader.number
+      const nowSec = Math.floor(Date.now() / 1000)
+      const diffSec = nowSec - Number(this.eth!.updatedBestHeader.timestamp)
+      const SLOT_TIME = 12


What if a slot is missed? Then this block is definitely not there 🤔

Good point, I initially planned to do 2-3 more commits fine tuning the existing solution a bit with a somewhat more adaptive algorithm (basically remember the last unsuccessful try (or remove on success again) and if present then gradually downgrade the request (so: try a lower block number)).

Guess we likely can/want to do this in a small follow-up PR, should be not such a big effort.

holgerd77 added PR state: WIP type: enhancement package: devp2p package: client labels Apr 11, 2024

holgerd77 force-pushed the client-add-mechanism-to-redo-peer-handshake-for-status-update branch from cb049b2 to 4d42c1a Compare April 15, 2024 14:02

holgerd77 changed the title ~~Client: Mechanism to redo Peer Handshake in PeerPool to update Status Information~~ Client: New Mechanism to Keep Peer Latest Block Updated Apr 15, 2024

holgerd77 commented Apr 15, 2024

View reviewed changes

holgerd77 force-pushed the client-add-mechanism-to-redo-peer-handshake-for-status-update branch from 8ea54b6 to 877bafa Compare April 16, 2024 10:49

holgerd77 added 16 commits April 16, 2024 13:21

Client -> Peer: add lastEthStatusUpdate property to Peer, set initial…

6a2b266

…ly on protocol binding, small local refactor

Redo handshake in client peer pool periodically, add force option to …

4dd9bf8

…sendStatus for devp2p protocols

Some basic refinement of peer pool periodic request logic (refactor p…

e268f9a

…reparation)

Client: consolidate latest() method in base Synchronizer class

b053277

Client: add preparatory updatedBestHeader to Peer, refactor latest() …

72a28d1

…to move from Sync -> Peer

Client: add potential best header num differentiated logic to Peer, l…

f29c3c6

…atest() call on peers in peer pool

Client: add Fetcher hack to call peer.latest()

6ea23b0

Various cleanups

fabf53c

Client: Fix lightsync.spec.ts tests

ccf9366

Some clean-ups

4cd01aa

Client: Fix beaconsync.spec.ts tests

5777c42

Client: Fix fullsync.spec.tst tests

9ea28d8

Client: Fix snapsync.spec.ts tests

9a6ae27

Client: Fix fetcher tests

5c42d73

Client: Fix eth syncing.spec.ts tests

9e60a09

Client: Fix integration tests

d6c2a6c

holgerd77 force-pushed the client-add-mechanism-to-redo-peer-handshake-for-status-update branch from 877bafa to d6c2a6c Compare April 16, 2024 11:21

holgerd77 and others added 3 commits April 16, 2024 13:39

Client: Backup lightsync integration tests (lightsync not supported a…

58cb2b4

…nymore)

Lightsync tests deprecation note

c53fa4b

Merge remote-tracking branch 'origin/master' into client-add-mechanis…

20bf36f

…m-to-redo-peer-handshake-for-status-update

acolytec3 and others added 2 commits April 17, 2024 16:28

Make lightsync tests run (but fail)

fe9a4ec

Merge branch 'master' into client-add-mechanism-to-redo-peer-handshak…

0117083

…e-for-status-update

holgerd77 added 2 commits April 22, 2024 10:40

Merge branch 'master' into client-add-mechanism-to-redo-peer-handshak…

7d877f2

…e-for-status-update

Client: Removed doubled lightsync integration test file

4e8aee6

holgerd77 added PR state: needs review and removed PR state: WIP package: devp2p labels Apr 22, 2024

Merge branch 'master' into client-add-mechanism-to-redo-peer-handshak…

4e7bd84

…e-for-status-update

g11tech approved these changes Apr 22, 2024

View reviewed changes

holgerd77 merged commit 46d09ca into master Apr 22, 2024
34 checks passed

holgerd77 deleted the client-add-mechanism-to-redo-peer-handshake-for-status-update branch April 22, 2024 12:46

jochem-brouwer reviewed Apr 22, 2024

View reviewed changes

scorbajio mentioned this pull request Apr 25, 2024

Snap sync moving root/healing implementation #3332

Open

scorbajio mentioned this pull request Aug 30, 2024

client: Best Peer Selection for Snap Sync Stalls due to Peer Status Issue in Devp2p #3222

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Client: New Mechanism to Keep Peer Latest Block Updated #3354

Client: New Mechanism to Keep Peer Latest Block Updated #3354

holgerd77 commented Apr 11, 2024 •

edited

Loading

holgerd77 commented Apr 15, 2024

holgerd77 Apr 15, 2024

holgerd77 Apr 15, 2024

codecov bot commented Apr 18, 2024 •

edited

Loading

g11tech left a comment

jochem-brouwer left a comment

jochem-brouwer Apr 22, 2024

holgerd77 Apr 23, 2024 •

edited

Loading

Client: New Mechanism to Keep Peer Latest Block Updated #3354

Client: New Mechanism to Keep Peer Latest Block Updated #3354

Conversation

holgerd77 commented Apr 11, 2024 • edited Loading

holgerd77 commented Apr 15, 2024

holgerd77 Apr 15, 2024

Choose a reason for hiding this comment

holgerd77 Apr 15, 2024

Choose a reason for hiding this comment

codecov bot commented Apr 18, 2024 • edited Loading

Codecov Report

g11tech left a comment

Choose a reason for hiding this comment

jochem-brouwer left a comment

Choose a reason for hiding this comment

jochem-brouwer Apr 22, 2024

Choose a reason for hiding this comment

holgerd77 Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

holgerd77 commented Apr 11, 2024 •

edited

Loading

codecov bot commented Apr 18, 2024 •

edited

Loading

holgerd77 Apr 23, 2024 •

edited

Loading