fix: close peer manager before sending goodbye to all peers

[nflaig]

Motivation

Closes #5642

Description

Closes peer manager before sending goodbye to all peers, see #5642 (comment) for rationale.

I looked at the code and this should not cause any issues as closing peer manager only removes intervals, event listeners and closes discv5.

[github-actions]

Performance Report

✔️ no performance regression detected

Full benchmark results

Benchmark suite	Current: 3fa430f	Previous: 0640e06	Ratio
getPubkeys - index2pubkey - req 1000 vs - 250000 vc	570.98 us/op	722.55 us/op	0.79
getPubkeys - validatorsArr - req 1000 vs - 250000 vc	46.334 us/op	50.966 us/op	0.91
BLS verify - blst-native	1.2115 ms/op	1.2099 ms/op	1.00
BLS verifyMultipleSignatures 3 - blst-native	2.4757 ms/op	2.4622 ms/op	1.01
BLS verifyMultipleSignatures 8 - blst-native	5.3007 ms/op	5.2961 ms/op	1.00
BLS verifyMultipleSignatures 32 - blst-native	19.082 ms/op	18.973 ms/op	1.01
BLS aggregatePubkeys 32 - blst-native	26.011 us/op	25.404 us/op	1.02
BLS aggregatePubkeys 128 - blst-native	102.27 us/op	99.279 us/op	1.03
getAttestationsForBlock	56.902 ms/op	51.664 ms/op	1.10
isKnown best case - 1 super set check	247.00 ns/op	250.00 ns/op	0.99
isKnown normal case - 2 super set checks	244.00 ns/op	245.00 ns/op	1.00
isKnown worse case - 16 super set checks	242.00 ns/op	242.00 ns/op	1.00
CheckpointStateCache - add get delete	5.2180 us/op	5.2940 us/op	0.99
validate gossip signedAggregateAndProof - struct	2.8052 ms/op	2.7449 ms/op	1.02
validate gossip attestation - struct	1.3242 ms/op	1.3171 ms/op	1.01
pickEth1Vote - no votes	1.2743 ms/op	1.2706 ms/op	1.00
pickEth1Vote - max votes	8.9160 ms/op	8.6658 ms/op	1.03
pickEth1Vote - Eth1Data hashTreeRoot value x2048	8.9790 ms/op	8.5978 ms/op	1.04
pickEth1Vote - Eth1Data hashTreeRoot tree x2048	14.821 ms/op	13.923 ms/op	1.06
pickEth1Vote - Eth1Data fastSerialize value x2048	804.00 us/op	764.24 us/op	1.05
pickEth1Vote - Eth1Data fastSerialize tree x2048	7.1019 ms/op	4.6987 ms/op	1.51
bytes32 toHexString	513.00 ns/op	488.00 ns/op	1.05
bytes32 Buffer.toString(hex)	346.00 ns/op	352.00 ns/op	0.98
bytes32 Buffer.toString(hex) from Uint8Array	620.00 ns/op	547.00 ns/op	1.13
bytes32 Buffer.toString(hex) + 0x	428.00 ns/op	369.00 ns/op	1.16
Object access 1 prop	0.19700 ns/op	0.16300 ns/op	1.21
Map access 1 prop	0.16000 ns/op	0.15400 ns/op	1.04
Object get x1000	6.8980 ns/op	6.4560 ns/op	1.07
Map get x1000	0.60400 ns/op	0.60600 ns/op	1.00
Object set x1000	67.881 ns/op	52.820 ns/op	1.29
Map set x1000	53.615 ns/op	43.186 ns/op	1.24
Return object 10000 times	0.24710 ns/op	0.23750 ns/op	1.04
Throw Error 10000 times	4.3124 us/op	4.2003 us/op	1.03
fastMsgIdFn sha256 / 200 bytes	3.5370 us/op	3.4860 us/op	1.01
fastMsgIdFn h32 xxhash / 200 bytes	274.00 ns/op	279.00 ns/op	0.98
fastMsgIdFn h64 xxhash / 200 bytes	399.00 ns/op	416.00 ns/op	0.96
fastMsgIdFn sha256 / 1000 bytes	11.225 us/op	11.820 us/op	0.95
fastMsgIdFn h32 xxhash / 1000 bytes	405.00 ns/op	430.00 ns/op	0.94
fastMsgIdFn h64 xxhash / 1000 bytes	492.00 ns/op	518.00 ns/op	0.95
fastMsgIdFn sha256 / 10000 bytes	103.71 us/op	102.70 us/op	1.01
fastMsgIdFn h32 xxhash / 10000 bytes	1.9660 us/op	1.9350 us/op	1.02
fastMsgIdFn h64 xxhash / 10000 bytes	1.4180 us/op	1.4120 us/op	1.00
enrSubnets - fastDeserialize 64 bits	1.3580 us/op	1.4150 us/op	0.96
enrSubnets - ssz BitVector 64 bits	507.00 ns/op	508.00 ns/op	1.00
enrSubnets - fastDeserialize 4 bits	170.00 ns/op	167.00 ns/op	1.02
enrSubnets - ssz BitVector 4 bits	508.00 ns/op	504.00 ns/op	1.01
prioritizePeers score -10:0 att 32-0.1 sync 2-0	117.30 us/op	108.56 us/op	1.08
prioritizePeers score 0:0 att 32-0.25 sync 2-0.25	149.12 us/op	149.56 us/op	1.00
prioritizePeers score 0:0 att 32-0.5 sync 2-0.5	191.89 us/op	184.06 us/op	1.04
prioritizePeers score 0:0 att 64-0.75 sync 4-0.75	356.63 us/op	326.56 us/op	1.09
prioritizePeers score 0:0 att 64-1 sync 4-1	408.75 us/op	387.94 us/op	1.05
array of 16000 items push then shift	1.6445 us/op	1.6208 us/op	1.01
LinkedList of 16000 items push then shift	8.9190 ns/op	8.7260 ns/op	1.02
array of 16000 items push then pop	114.61 ns/op	89.386 ns/op	1.28
LinkedList of 16000 items push then pop	8.9220 ns/op	8.5470 ns/op	1.04
array of 24000 items push then shift	2.3906 us/op	2.3338 us/op	1.02
LinkedList of 24000 items push then shift	9.0090 ns/op	8.8320 ns/op	1.02
array of 24000 items push then pop	82.932 ns/op	78.095 ns/op	1.06
LinkedList of 24000 items push then pop	8.7020 ns/op	8.5960 ns/op	1.01
intersect bitArray bitLen 8	13.801 ns/op	12.992 ns/op	1.06
intersect array and set length 8	93.681 ns/op	76.948 ns/op	1.22
intersect bitArray bitLen 128	44.566 ns/op	43.307 ns/op	1.03
intersect array and set length 128	1.3255 us/op	1.0523 us/op	1.26
Buffer.concat 32 items	2.9620 us/op	2.8600 us/op	1.04
Uint8Array.set 32 items	2.2480 us/op	2.6600 us/op	0.85
transfer serialized Status (84 B)	2.0370 us/op	2.1710 us/op	0.94
copy serialized Status (84 B)	1.6670 us/op	1.7290 us/op	0.96
transfer serialized SignedVoluntaryExit (112 B)	2.0460 us/op	2.2310 us/op	0.92
copy serialized SignedVoluntaryExit (112 B)	1.9390 us/op	1.7660 us/op	1.10
transfer serialized ProposerSlashing (416 B)	3.2660 us/op	2.7450 us/op	1.19
copy serialized ProposerSlashing (416 B)	3.3820 us/op	2.9390 us/op	1.15
transfer serialized Attestation (485 B)	3.3900 us/op	2.6330 us/op	1.29
copy serialized Attestation (485 B)	3.3200 us/op	2.8340 us/op	1.17
transfer serialized AttesterSlashing (33232 B)	3.4950 us/op	3.1610 us/op	1.11
copy serialized AttesterSlashing (33232 B)	7.7240 us/op	6.4320 us/op	1.20
transfer serialized Small SignedBeaconBlock (128000 B)	3.6810 us/op	3.3160 us/op	1.11
copy serialized Small SignedBeaconBlock (128000 B)	17.662 us/op	13.398 us/op	1.32
transfer serialized Avg SignedBeaconBlock (200000 B)	3.9490 us/op	3.8530 us/op	1.02
copy serialized Avg SignedBeaconBlock (200000 B)	30.959 us/op	20.728 us/op	1.49
transfer serialized BlobsSidecar (524380 B)	4.1600 us/op	3.1600 us/op	1.32
copy serialized BlobsSidecar (524380 B)	175.00 us/op	162.67 us/op	1.08
transfer serialized Big SignedBeaconBlock (1000000 B)	3.5930 us/op	3.2430 us/op	1.11
copy serialized Big SignedBeaconBlock (1000000 B)	246.61 us/op	334.35 us/op	0.74
pass gossip attestations to forkchoice per slot	2.6966 ms/op	2.7074 ms/op	1.00
forkChoice updateHead vc 100000 bc 64 eq 0	2.0869 ms/op	2.0839 ms/op	1.00
forkChoice updateHead vc 600000 bc 64 eq 0	13.247 ms/op	12.041 ms/op	1.10
forkChoice updateHead vc 1000000 bc 64 eq 0	23.236 ms/op	23.433 ms/op	0.99
forkChoice updateHead vc 600000 bc 320 eq 0	17.084 ms/op	17.425 ms/op	0.98
forkChoice updateHead vc 600000 bc 1200 eq 0	85.324 ms/op	82.391 ms/op	1.04
forkChoice updateHead vc 600000 bc 64 eq 1000	20.980 ms/op	20.712 ms/op	1.01
forkChoice updateHead vc 600000 bc 64 eq 10000	23.373 ms/op	22.637 ms/op	1.03
forkChoice updateHead vc 600000 bc 64 eq 300000	31.449 ms/op	31.471 ms/op	1.00
computeDeltas	3.5612 ms/op	3.4441 ms/op	1.03
computeProposerBoostScoreFromBalances	1.8247 ms/op	1.7941 ms/op	1.02
altair processAttestation - 250000 vs - 7PWei normalcase	3.6174 ms/op	2.2630 ms/op	1.60
altair processAttestation - 250000 vs - 7PWei worstcase	3.9789 ms/op	4.1173 ms/op	0.97
altair processAttestation - setStatus - 1/6 committees join	141.86 us/op	160.38 us/op	0.88
altair processAttestation - setStatus - 1/3 committees join	286.37 us/op	326.01 us/op	0.88
altair processAttestation - setStatus - 1/2 committees join	376.29 us/op	423.07 us/op	0.89
altair processAttestation - setStatus - 2/3 committees join	482.10 us/op	529.87 us/op	0.91
altair processAttestation - setStatus - 4/5 committees join	660.87 us/op	732.29 us/op	0.90
altair processAttestation - setStatus - 100% committees join	784.00 us/op	863.09 us/op	0.91
altair processBlock - 250000 vs - 7PWei normalcase	20.268 ms/op	19.676 ms/op	1.03
altair processBlock - 250000 vs - 7PWei normalcase hashState	25.346 ms/op	27.022 ms/op	0.94
altair processBlock - 250000 vs - 7PWei worstcase	51.088 ms/op	52.374 ms/op	0.98
altair processBlock - 250000 vs - 7PWei worstcase hashState	67.992 ms/op	67.690 ms/op	1.00
phase0 processBlock - 250000 vs - 7PWei normalcase	2.1449 ms/op	2.1362 ms/op	1.00
phase0 processBlock - 250000 vs - 7PWei worstcase	30.030 ms/op	29.766 ms/op	1.01
altair processEth1Data - 250000 vs - 7PWei normalcase	536.77 us/op	591.62 us/op	0.91
getExpectedWithdrawals 250000 eb:1,eth1:1,we:0,wn:0,smpl:15	7.9100 us/op	8.4080 us/op	0.94
getExpectedWithdrawals 250000 eb:0.95,eth1:0.1,we:0.05,wn:0,smpl:219	31.581 us/op	28.059 us/op	1.13
getExpectedWithdrawals 250000 eb:0.95,eth1:0.3,we:0.05,wn:0,smpl:42	10.329 us/op	11.063 us/op	0.93
getExpectedWithdrawals 250000 eb:0.95,eth1:0.7,we:0.05,wn:0,smpl:18	7.5610 us/op	8.5360 us/op	0.89
getExpectedWithdrawals 250000 eb:0.1,eth1:0.1,we:0,wn:0,smpl:1020	78.118 us/op	103.63 us/op	0.75
getExpectedWithdrawals 250000 eb:0.03,eth1:0.03,we:0,wn:0,smpl:11777	653.77 us/op	738.66 us/op	0.89
getExpectedWithdrawals 250000 eb:0.01,eth1:0.01,we:0,wn:0,smpl:16384	914.73 us/op	945.04 us/op	0.97
getExpectedWithdrawals 250000 eb:0,eth1:0,we:0,wn:0,smpl:16384	893.05 us/op	944.23 us/op	0.95
getExpectedWithdrawals 250000 eb:0,eth1:0,we:0,wn:0,nocache,smpl:16384	2.3235 ms/op	2.5559 ms/op	0.91
getExpectedWithdrawals 250000 eb:0,eth1:1,we:0,wn:0,smpl:16384	1.5857 ms/op	1.7676 ms/op	0.90
getExpectedWithdrawals 250000 eb:0,eth1:1,we:0,wn:0,nocache,smpl:16384	3.9244 ms/op	4.4739 ms/op	0.88
Tree 40 250000 create	329.11 ms/op	337.13 ms/op	0.98
Tree 40 250000 get(125000)	182.86 ns/op	203.34 ns/op	0.90
Tree 40 250000 set(125000)	979.62 ns/op	992.90 ns/op	0.99
Tree 40 250000 toArray()	16.939 ms/op	23.676 ms/op	0.72
Tree 40 250000 iterate all - toArray() + loop	22.253 ms/op	23.708 ms/op	0.94
Tree 40 250000 iterate all - get(i)	69.845 ms/op	81.862 ms/op	0.85
MutableVector 250000 create	10.499 ms/op	11.138 ms/op	0.94
MutableVector 250000 get(125000)	6.2550 ns/op	6.2370 ns/op	1.00
MutableVector 250000 set(125000)	236.91 ns/op	292.70 ns/op	0.81
MutableVector 250000 toArray()	2.5765 ms/op	3.7693 ms/op	0.68
MutableVector 250000 iterate all - toArray() + loop	2.6597 ms/op	3.5122 ms/op	0.76
MutableVector 250000 iterate all - get(i)	1.4733 ms/op	1.5488 ms/op	0.95
Array 250000 create	2.4212 ms/op	3.1029 ms/op	0.78
Array 250000 clone - spread	1.0527 ms/op	1.0942 ms/op	0.96
Array 250000 get(125000)	0.53300 ns/op	0.54600 ns/op	0.98
Array 250000 set(125000)	0.59500 ns/op	0.63500 ns/op	0.94
Array 250000 iterate all - loop	88.389 us/op	105.85 us/op	0.84
effectiveBalanceIncrements clone Uint8Array 300000	28.149 us/op	36.549 us/op	0.77
effectiveBalanceIncrements clone MutableVector 300000	327.00 ns/op	320.00 ns/op	1.02
effectiveBalanceIncrements rw all Uint8Array 300000	165.11 us/op	169.52 us/op	0.97
effectiveBalanceIncrements rw all MutableVector 300000	78.376 ms/op	79.809 ms/op	0.98
phase0 afterProcessEpoch - 250000 vs - 7PWei	110.01 ms/op	115.54 ms/op	0.95
phase0 beforeProcessEpoch - 250000 vs - 7PWei	41.816 ms/op	44.318 ms/op	0.94
altair processEpoch - mainnet_e81889	299.22 ms/op	331.06 ms/op	0.90
mainnet_e81889 - altair beforeProcessEpoch	49.466 ms/op	71.388 ms/op	0.69
mainnet_e81889 - altair processJustificationAndFinalization	17.472 us/op	18.892 us/op	0.92
mainnet_e81889 - altair processInactivityUpdates	5.3461 ms/op	5.1671 ms/op	1.03
mainnet_e81889 - altair processRewardsAndPenalties	67.270 ms/op	49.696 ms/op	1.35
mainnet_e81889 - altair processRegistryUpdates	3.1850 us/op	2.8790 us/op	1.11
mainnet_e81889 - altair processSlashings	541.00 ns/op	638.00 ns/op	0.85
mainnet_e81889 - altair processEth1DataReset	629.00 ns/op	705.00 ns/op	0.89
mainnet_e81889 - altair processEffectiveBalanceUpdates	1.1913 ms/op	1.2588 ms/op	0.95
mainnet_e81889 - altair processSlashingsReset	4.8760 us/op	4.9450 us/op	0.99
mainnet_e81889 - altair processRandaoMixesReset	4.6020 us/op	5.0720 us/op	0.91
mainnet_e81889 - altair processHistoricalRootsUpdate	788.00 ns/op	1.3220 us/op	0.60
mainnet_e81889 - altair processParticipationFlagUpdates	2.4130 us/op	5.1570 us/op	0.47
mainnet_e81889 - altair processSyncCommitteeUpdates	833.00 ns/op	732.00 ns/op	1.14
mainnet_e81889 - altair afterProcessEpoch	119.48 ms/op	126.64 ms/op	0.94
phase0 processEpoch - mainnet_e58758	331.88 ms/op	419.58 ms/op	0.79
mainnet_e58758 - phase0 beforeProcessEpoch	107.50 ms/op	190.70 ms/op	0.56
mainnet_e58758 - phase0 processJustificationAndFinalization	16.184 us/op	32.517 us/op	0.50
mainnet_e58758 - phase0 processRewardsAndPenalties	52.827 ms/op	78.813 ms/op	0.67
mainnet_e58758 - phase0 processRegistryUpdates	8.2500 us/op	15.317 us/op	0.54
mainnet_e58758 - phase0 processSlashings	491.00 ns/op	1.4340 us/op	0.34
mainnet_e58758 - phase0 processEth1DataReset	518.00 ns/op	1.6720 us/op	0.31
mainnet_e58758 - phase0 processEffectiveBalanceUpdates	939.67 us/op	1.7791 ms/op	0.53
mainnet_e58758 - phase0 processSlashingsReset	3.6030 us/op	9.8430 us/op	0.37
mainnet_e58758 - phase0 processRandaoMixesReset	4.7350 us/op	11.827 us/op	0.40
mainnet_e58758 - phase0 processHistoricalRootsUpdate	699.00 ns/op	1.8140 us/op	0.39
mainnet_e58758 - phase0 processParticipationRecordUpdates	4.1200 us/op	11.853 us/op	0.35
mainnet_e58758 - phase0 afterProcessEpoch	95.350 ms/op	114.29 ms/op	0.83
phase0 processEffectiveBalanceUpdates - 250000 normalcase	1.2217 ms/op	1.4476 ms/op	0.84
phase0 processEffectiveBalanceUpdates - 250000 worstcase 0.5	1.4575 ms/op	1.6061 ms/op	0.91
altair processInactivityUpdates - 250000 normalcase	19.245 ms/op	32.683 ms/op	0.59
altair processInactivityUpdates - 250000 worstcase	21.251 ms/op	28.364 ms/op	0.75
phase0 processRegistryUpdates - 250000 normalcase	6.8120 us/op	8.0280 us/op	0.85
phase0 processRegistryUpdates - 250000 badcase_full_deposits	217.79 us/op	268.56 us/op	0.81
phase0 processRegistryUpdates - 250000 worstcase 0.5	116.16 ms/op	126.41 ms/op	0.92
altair processRewardsAndPenalties - 250000 normalcase	55.134 ms/op	68.237 ms/op	0.81
altair processRewardsAndPenalties - 250000 worstcase	44.148 ms/op	66.482 ms/op	0.66
phase0 getAttestationDeltas - 250000 normalcase	6.0378 ms/op	6.9815 ms/op	0.86
phase0 getAttestationDeltas - 250000 worstcase	6.3198 ms/op	6.6758 ms/op	0.95
phase0 processSlashings - 250000 worstcase	3.4515 ms/op	4.1016 ms/op	0.84
altair processSyncCommitteeUpdates - 250000	163.92 ms/op	188.68 ms/op	0.87
BeaconState.hashTreeRoot - No change	252.00 ns/op	329.00 ns/op	0.77
BeaconState.hashTreeRoot - 1 full validator	46.707 us/op	54.817 us/op	0.85
BeaconState.hashTreeRoot - 32 full validator	499.04 us/op	511.89 us/op	0.97
BeaconState.hashTreeRoot - 512 full validator	5.4384 ms/op	5.4587 ms/op	1.00
BeaconState.hashTreeRoot - 1 validator.effectiveBalance	60.521 us/op	64.516 us/op	0.94
BeaconState.hashTreeRoot - 32 validator.effectiveBalance	856.38 us/op	917.89 us/op	0.93
BeaconState.hashTreeRoot - 512 validator.effectiveBalance	11.145 ms/op	12.179 ms/op	0.92
BeaconState.hashTreeRoot - 1 balances	48.359 us/op	49.807 us/op	0.97
BeaconState.hashTreeRoot - 32 balances	417.75 us/op	488.40 us/op	0.86
BeaconState.hashTreeRoot - 512 balances	4.1507 ms/op	4.6945 ms/op	0.88
BeaconState.hashTreeRoot - 250000 balances	71.946 ms/op	75.573 ms/op	0.95
aggregationBits - 2048 els - zipIndexesInBitList	16.123 us/op	18.932 us/op	0.85
regular array get 100000 times	34.872 us/op	41.191 us/op	0.85
wrappedArray get 100000 times	32.361 us/op	40.435 us/op	0.80
arrayWithProxy get 100000 times	15.400 ms/op	17.372 ms/op	0.89
ssz.Root.equals	542.00 ns/op	608.00 ns/op	0.89
byteArrayEquals	534.00 ns/op	552.00 ns/op	0.97
shuffle list - 16384 els	6.7983 ms/op	6.8656 ms/op	0.99
shuffle list - 250000 els	96.361 ms/op	102.53 ms/op	0.94
processSlot - 1 slots	8.5420 us/op	8.8260 us/op	0.97
processSlot - 32 slots	1.2996 ms/op	1.3445 ms/op	0.97
getEffectiveBalanceIncrementsZeroInactive - 250000 vs - 7PWei	33.260 ms/op	36.833 ms/op	0.90
getCommitteeAssignments - req 1 vs - 250000 vc	2.8478 ms/op	2.9121 ms/op	0.98
getCommitteeAssignments - req 100 vs - 250000 vc	4.0408 ms/op	4.1207 ms/op	0.98
getCommitteeAssignments - req 1000 vs - 250000 vc	4.3898 ms/op	4.4535 ms/op	0.99
RootCache.getBlockRootAtSlot - 250000 vs - 7PWei	4.5600 ns/op	4.6900 ns/op	0.97
state getBlockRootAtSlot - 250000 vs - 7PWei	832.09 ns/op	608.17 ns/op	1.37
computeProposers - vc 250000	10.419 ms/op	10.644 ms/op	0.98
computeEpochShuffling - vc 250000	100.71 ms/op	102.21 ms/op	0.99
getNextSyncCommittee - vc 250000	171.77 ms/op	175.05 ms/op	0.98
computeSigningRoot for AttestationData	13.582 us/op	13.191 us/op	1.03
hash AttestationData serialized data then Buffer.toString(base64)	2.4169 us/op	2.5016 us/op	0.97
toHexString serialized data	1.1524 us/op	1.1660 us/op	0.99
Buffer.toString(base64)	316.00 ns/op	349.85 ns/op	0.90

by benchmarkbot/action

[nflaig]

This PR does not resolve the issue, see #5642 (comment).

I think it might not be a correct change as closing the peer manager removes onLibp2pPeerDisconnect listener

lodestar/packages/beacon-node/src/network/peers/peerManager.ts

Lines 206 to 209 in 0640e06

	this.libp2p.services.components.events.removeEventListener(
	Libp2pEvent.connectionClose,
	this.onLibp2pPeerDisconnect
	);

which should take care that we are not pinging peers

lodestar/packages/beacon-node/src/network/peers/peerManager.ts

Lines 633 to 634 in 0640e06

	// remove the ping and status timer for the peer
	this.connectedPeers.delete(peer.toString());

But there is also another listener for handling new connections onLibp2pPeerConnect

lodestar/packages/beacon-node/src/network/peers/peerManager.ts

Line 205 in 0640e06

this.libp2p.services.components.events.removeEventListener(Libp2pEvent.connectionOpen, this.onLibp2pPeerConnect);

It seems more logical to me that we first stop accepting and handling new connections and then disconnect existing peers.

Maybe @wemeetagain or anyone else more familiar with that part of the code has some thoughts

[nflaig]

Looks like this change causes an issue in Sim tests, the job hangs for ~6 hours until it is aborted.

[nflaig]

Closing as this PR does not fix #5642.

[dapplion]

Looks like this change causes an issue in Sim tests, the job hangs for ~6 hours until it is aborted.

hmm but this looked like a reasonable change right? We don't want to dial more peers when goodbye-ing existing. Can you double check why that happens?

[nflaig]

hmm but this looked like a reasonable change right?

I think the change makes sense but will look into the code again and make sure I've not missed anything

Can you double check why that happens?

I went through the sim test logs but there is nothing that peaks out or looks different from other runs. I have rebased the branch against unstable and the sim tests pass now.

[nflaig]

Took another look at this and the difference of changing the order is really insignificant and I am still not 100% sure if removing onLibp2pPeerDisconnect listener has any unwanted side effects.

lodestar/packages/beacon-node/src/network/peers/peerManager.ts

Line 628 in 34d8955

private onLibp2pPeerDisconnect = (evt: CustomEvent<Connection>): void => {

I see three options

1. keep as is, in this case it can happen that we dial a new peer after sending goodbyes

Aug-11 12:55:20.644[network]       verbose: peer connected peer=16...1nnKGp, direction=outbound, status=OPEN
Aug-11 12:55:20.645[network]         debug: Req  dialing peer method=ping, version=1, encoding=ssz_snappy, client=Unknown, peer=16...1nnKGp, requestId=795
Aug-11 12:55:20.645[network]         debug: Req  dialing peer method=status, version=1, encoding=ssz_snappy, client=Unknown, peer=16...1nnKGp, requestId=796
Aug-11 12:55:20.646[network]         debug: Dialed discovered peer peer=16...1nnKGp

goodbye-before-close.log

2. close before goodbye, in this case we do not call onLibp2pPeerDisconnect (does it matter during shutdown?)

close-before-goodbye.log

3. close before goodye and keep onLibp2pPeerDisconnect listener, in this case we might get best of both worlds, keeping the listener also doesn't seem to matter, process still exits cleanly. It is questionable though if we even want to emit event like NetworkEvent.peerDisconnected during shutdown

lodestar/packages/beacon-node/src/network/peers/peerManager.ts

Line 637 in 34d8955

this.networkEventBus.emit(NetworkEvent.peerDisconnected, {peer: peer.toString()});

close-before-goodbye-keep-onLibp2pPeerDisconnect-listener.txt

I don't feel like this must be changed as it has worked for so long like this and doesn't resolve issue #5642.

Keeping as is until I understand the whole networking code a bit better, changes are in branch unstable...nflaig/close-peermanager-before-goodbye, will not open a PR for now unless someone thinks it's really worth to change current behavior.