Remove all batches related to a peer on disconnect #5969

pawanjay176 · 2024-06-20T07:43:46Z

Issue Addressed

N/A

Proposed Changes

In #5680 , we decoupled a peer disconnect from an rpc error to simplify the logic in sync a bit.
The peer disconnect message from the network was responsible for removing the peer from all the sync data structures.
The rpc error was responsible for re-requesting the failed batch/lookup.

We assumed that an rpc error would always go together with a peer disconnect, however we learned the hard way that there are many edge cases where the rpc error never gets generated when the peer disconnects. e.g. the peer manager disconnects a peer that has its rpc response mid-flight, peer disconnects before a rpc request reaches the network etc. There are many other cases we may not know about.
This leads to multiple bugs in sync where sync stalls because a batch/lookup never gets re-requested on peer disconnects.

#5967 was an attempt at adding timeouts to the rpc requests sent from sync that trigger re-requests when the timer elapses. However, as @AgeManning noted, this would lead to many more edge cases.

This PR triggers re-request of batches/lookups on peer disconnects partially reverting #5680 . This isn't the cleanest but considering that we cannot always rely on lighthouse_network to generate rpc errors to go with peer disconnects, this seems the best option.

I intend to test this extensively on mainnet before merging

dapplion · 2024-06-20T07:58:33Z

For block lookups we should iterate the active requests here and fail them

lighthouse/beacon_node/network/src/sync/network_context.rs

Lines 120 to 123 in f1d88ba

    
           blocks_by_root_requests: FnvHashMap<SingleLookupReqId, ActiveBlocksByRootRequest>, 
        
           /// A mapping of active BlobsByRoot requests, including both current slot and parent lookups. 
        
           blobs_by_root_requests: FnvHashMap<SingleLookupReqId, ActiveBlobsByRootRequest<T::EthSpec>>,

Plus add tests to ensure that lookups can handle getting late errors for a requests that has already been failed for disconnection. Current lookup code is quite strict in that it expects a single event per network request.

dapplion · 2024-06-20T13:01:40Z

Note that sync doesn't need to check if the peer is connected before sending a request. Consider these two cases:

Case 1: (Receive disconnect after request)

Network: peer A disconnects
Sync: sends request X to peer A
Sync: receives disconnect event for peer A -> Fails request X

Case 2: (Receive disconnect before request)

Network: peer A disconnects
Sync: receives disconnect event for peer A -> Removes peer A from pool of available peers
Sync: sends request X to peer some other peer

michaelsproul · 2024-06-23T23:00:39Z

I've re-targeted this at release-v5.2.1. See:

Release v5.2.1 (final branch) #5984

AgeManning · 2024-06-24T05:23:42Z

I think we can probably also now remove the RPC Error logic we added. I think its not that necessary if we are now relying on the PeerDisconnection message. i.e I think we can remove this entire function: https://github.com/sigp/lighthouse/blob/stable/beacon_node/lighthouse_network/src/rpc/handler.rs#L356

AgeManning · 2024-06-24T05:24:02Z

I can also remove this extra code in a future PR if we prefer to test this standalone?

pawanjay176 · 2024-06-24T10:40:35Z

@AgeManning removed in cd17f9f
I have tested this PR on mainnet for forward sync, backfill and lookups. I think its in a good state now.

@dapplion I changed lookups peer_disconnected behaviour such that it drops a lookup only if there are no peers to drive the request forward or if there are no existing downloaded components.
This would mean that there could be parent requests in lookups that have no associated peer. I think this is okay, lmk if you disagree.

AgeManning

Nice!!

beacon_node/network/src/sync/network_context.rs

beacon_node/network/src/sync/block_lookups/mod.rs

beacon_node/network/src/sync/network_context.rs

jxs

Thanks for driving this Pawan!

beacon_node/network/src/sync/network_context.rs

beacon_node/network/src/sync/block_lookups/mod.rs

beacon_node/network/src/sync/backfill_sync/mod.rs

beacon_node/network/src/sync/range_sync/chain.rs

beacon_node/network/src/sync/block_lookups/tests.rs

…n-disconnect-revert

Squashed commit of the following: commit 0c8efc1 Merge: 62167fb 758b58c Author: Michael Sproul <michael@sigmaprime.io> Date: Tue Jun 25 11:21:39 2024 +1000 Merge remote-tracking branch 'origin/release-v5.2.1' into rpc-error-on-disconnect-revert commit 62167fb Author: Pawan Dhananjay <pawandhananjay@gmail.com> Date: Tue Jun 25 02:30:53 2024 +0530 Fix test commit b930c7c Author: Pawan Dhananjay <pawandhananjay@gmail.com> Date: Tue Jun 25 01:58:47 2024 +0530 Cleanup commit dfaf238 Author: Pawan Dhananjay <pawandhananjay@gmail.com> Date: Tue Jun 25 01:56:29 2024 +0530 Revert pretty response types commit 3e1c41a Author: Pawan Dhananjay <pawandhananjay@gmail.com> Date: Tue Jun 25 01:45:20 2024 +0530 Fix issue raised by lion commit 6ead176 Author: Pawan Dhananjay <pawandhananjay@gmail.com> Date: Mon Jun 24 18:13:24 2024 +0530 Remove redundant test commit cd17f9f Author: Pawan Dhananjay <pawandhananjay@gmail.com> Date: Mon Jun 24 15:39:34 2024 +0530 Remove poll_close on rpc behaviour commit bc10fb2 Author: Pawan Dhananjay <pawandhananjay@gmail.com> Date: Mon Jun 24 15:25:56 2024 +0530 Fix tests commit a8f64f2 Author: Pawan Dhananjay <pawandhananjay@gmail.com> Date: Mon Jun 24 13:03:30 2024 +0530 Remove lookup if it cannot progress commit c7fd21d Author: Pawan Dhananjay <pawandhananjay@gmail.com> Date: Mon Jun 24 10:50:37 2024 +0530 Fix lints commit a38caf4 Author: Pawan Dhananjay <pawandhananjay@gmail.com> Date: Fri Jun 21 21:43:41 2024 +0530 fmt commit 312be2c Author: Pawan Dhananjay <pawandhananjay@gmail.com> Date: Fri Jun 21 21:43:33 2024 +0530 Pretty response types commit 70af7d1 Author: Pawan Dhananjay <pawandhananjay@gmail.com> Date: Fri Jun 21 18:02:40 2024 +0530 Allow lookups to continue in case of disconnections commit 9d90e39 Author: Pawan Dhananjay <pawandhananjay@gmail.com> Date: Fri Jun 21 17:34:01 2024 +0530 Cleanup map entries after disconnect commit 92a6e77 Author: Pawan Dhananjay <pawandhananjay@gmail.com> Date: Thu Jun 20 12:50:19 2024 +0530 Remove all batches related to a peer on disconnect

Co-authored-by: João Oliveira <hello@jxs.pt>

dapplion · 2024-06-25T14:11:16Z

@pawanjay176 Added some comments relevant to the API expectations of lookup sync. I think they are helpful to understand bug sources and overall behaviour

Add comments about expected event api pawanjay176/lighthouse#10

Add comments about expected event api

jimmygchen · 2024-06-26T23:24:35Z

@mergify queue

mergify · 2024-06-26T23:24:39Z

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at bf4cbd3

Remove all batches related to a peer on disconnect

92a6e77

pawanjay176 added v5.2.1 Patch release for v5.2.0 do-not-merge labels Jun 20, 2024

pawanjay176 mentioned this pull request Jun 20, 2024

Resend timed out requests in sync #5967

Closed

pawanjay176 added 4 commits June 21, 2024 17:34

Cleanup map entries after disconnect

9d90e39

Allow lookups to continue in case of disconnections

70af7d1

Pretty response types

312be2c

fmt

a38caf4

michaelsproul changed the base branch from unstable to release-v5.2.1 June 23, 2024 23:00

pawanjay176 added ready-for-review The code is ready for review and removed do-not-merge labels Jun 24, 2024

pawanjay176 requested review from AgeManning and dapplion June 24, 2024 04:30

Fix lints

c7fd21d

pawanjay176 added 3 commits June 24, 2024 13:03

Remove lookup if it cannot progress

a8f64f2

Fix tests

bc10fb2

Remove poll_close on rpc behaviour

cd17f9f

AgeManning approved these changes Jun 24, 2024

View reviewed changes

Remove redundant test

6ead176

dapplion reviewed Jun 24, 2024

View reviewed changes

beacon_node/network/src/sync/network_context.rs Outdated Show resolved Hide resolved

beacon_node/network/src/sync/network_context.rs Show resolved Hide resolved

pawanjay176 added 4 commits June 25, 2024 01:45

Fix issue raised by lion

3e1c41a

Revert pretty response types

dfaf238

Cleanup

b930c7c

Fix test

62167fb

dapplion reviewed Jun 24, 2024

View reviewed changes

beacon_node/network/src/sync/block_lookups/mod.rs Outdated Show resolved Hide resolved

beacon_node/network/src/sync/network_context.rs Show resolved Hide resolved

jxs approved these changes Jun 24, 2024

View reviewed changes

Merge remote-tracking branch 'origin/release-v5.2.1' into rpc-error-o…

0c8efc1

…n-disconnect-revert

michaelsproul mentioned this pull request Jun 25, 2024

Release v5.2.1 (testing branch) #5989

Merged

pawanjay176 and others added 5 commits June 25, 2024 02:42

Apply suggestions from joao

26614f1

Co-authored-by: João Oliveira <hello@jxs.pt>

Fix log

ec90bf0

update request status on no peers found

e79c71a

Do not remove lookup after peer disconnection

caee7c8

Add comments about expected event api

804f36d

dapplion and others added 4 commits June 25, 2024 16:17

Update single_block_lookup.rs

9b2e9e0

Update mod.rs

cd68550

Merge branch 'rpc-error-on-disconnect-revert' into 5969-review

5b0fbd9

Merge pull request #10 from dapplion/5969-review

59468f8

Add comments about expected event api

jimmygchen added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Jun 26, 2024

mergify bot added a commit that referenced this pull request Jun 26, 2024

Merge of #5969

f62061b

This was referenced Jun 26, 2024

merge queue: embarking release-v5.2.1 (758b58c) and [#5969 + #5993] together #6003

Closed

Attempt to continue lookups after adding peers #5993

Merged

mergify bot merged commit bf4cbd3 into sigp:release-v5.2.1 Jun 26, 2024
28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove all batches related to a peer on disconnect #5969

Remove all batches related to a peer on disconnect #5969

pawanjay176 commented Jun 20, 2024

dapplion commented Jun 20, 2024

dapplion commented Jun 20, 2024 •

edited

Loading

michaelsproul commented Jun 23, 2024 •

edited

Loading

AgeManning commented Jun 24, 2024

AgeManning commented Jun 24, 2024

pawanjay176 commented Jun 24, 2024 •

edited

Loading

AgeManning left a comment

jxs left a comment

dapplion commented Jun 25, 2024

jimmygchen commented Jun 26, 2024

mergify bot commented Jun 26, 2024 •

edited

Loading

Remove all batches related to a peer on disconnect #5969

Remove all batches related to a peer on disconnect #5969

Conversation

pawanjay176 commented Jun 20, 2024

Issue Addressed

Proposed Changes

dapplion commented Jun 20, 2024

dapplion commented Jun 20, 2024 • edited Loading

michaelsproul commented Jun 23, 2024 • edited Loading

AgeManning commented Jun 24, 2024

AgeManning commented Jun 24, 2024

pawanjay176 commented Jun 24, 2024 • edited Loading

AgeManning left a comment

Choose a reason for hiding this comment

jxs left a comment

Choose a reason for hiding this comment

dapplion commented Jun 25, 2024

jimmygchen commented Jun 26, 2024

mergify bot commented Jun 26, 2024 • edited Loading

✅ The pull request has been merged automatically

dapplion commented Jun 20, 2024 •

edited

Loading

michaelsproul commented Jun 23, 2024 •

edited

Loading

pawanjay176 commented Jun 24, 2024 •

edited

Loading

mergify bot commented Jun 26, 2024 •

edited

Loading