Fix Initial Sync with 128 data columns subnets #14403

nalepae · 2024-08-30T12:42:08Z

Please read commit by commit, with commits message.

E2E on my local computer: ✅
Kurtosis 2 super nodes + 1 full node: ✅
Kurtosis 2 super nodes + 1 full node started at epoch 4: ✅

…d no `--bootstrap-node` flag is used. Before this commit, if the `--chain-config-file` flag is used and no `--bootstrap-node` flag is used, then bootnodes are (incorrectly) defaulted on `mainnet` ones.

"Peer connected" does not really reflect the fact that a new peer is actually connected. --> "New peer connection" is more clear. Also, instead of writing `0`, `1`or `2` for direction, now it's writted "Unknown", "Inbound", "Outbound".

… columns.

…if we have enough peers for this topic. Before this commit, new peers HAD to be found, even if current peers are eventually acceptable. For very small network, it used to lead to infinite search.

…function initially and every slot.

nisdas · 2024-09-03T09:41:40Z

beacon-chain/p2p/handshake.go

+						"direction":   conn.Stat().Direction.String(),
+						"multiAddr":   peerMultiaddrString(conn),
+						"activePeers": len(s.peers.Active()),
+					}).Debug("New peer disconnection")


I think Peer disconnected works better here. This disconnection handler applies to all peers not just newly joined peers

Fixed in bd34800.

nisdas · 2024-09-03T09:43:16Z

beacon-chain/p2p/pubsub_filter.go

+			"peerID":             peerID,
+			"subscriptionCounts": subsCount,
+			"subscriptionLimit":  pubsubSubscriptionRequestLimit,
+		}).Error("Too many incoming subscriptions, filtering them")


can we make this a debug ? This can be triggered by any peer, so they could simply causes your logs to be spammed/blown up by sending you rpc subscriptions

Fixed in ab0b0e8.

nisdas · 2024-09-03T09:43:50Z

beacon-chain/p2p/service.go

@@ -396,9 +397,14 @@ func (s *Service) AddPingMethod(reqFunc func(ctx context.Context, id peer.ID) er
 func (s *Service) pingPeers() {
 	s.pingMethodLock.RLock()
 	defer s.pingMethodLock.RUnlock()
+
+	localENR := s.dv5Listener.Self()
+	log.WithField("ENR", localENR).Info("New node record")


why are we logging this ?

Why not?

We log the very first ENR here:

INFO p2p: Started discovery v5 ENR=enr:-Me4QOjJD9yIdENgUqCEWvF8-i4ptgYCcE2_-585RTOkZmTbH09IuXNW5Y3hX_WG_-DEncEytbRry_tLe31BECrKPRGGAZG3Q7i0h2F0dG5ldHOIAAAAAAAAAACDY3NjBIRldGgykDdW2IZgAAA4AOH1BQAAAACCaWSCdjSCaXCErBAAEolzZWNwMjU2azGhA4Vo-9vK3iXroeg9TmKANgrXuntjWj6atypZL7bbpjBviHN5bmNuZXRzAIN0Y3CCMsiDdW RwgjLI

But it turns out this ENR is the real one for a very small amount of time.
This ENR is modified shortly after.

Logging the initial ENR without logging modified ones is quite misleading since the user will think that it is it actual ENR, while it is not any more the case.

My opinion is:

We should log all ENR changes, or

We should NOT log any ENR at all.

If you do want to log enr changes, pingPeers is the wrong place to do it. It is more accurate to do it in RefreshPersistentSubnets as that is where the enr is actually modified.

I agree.
However, currently pingPeers in only called by RefreshPersistentSubnets.
And the way RefreshPersistentSubnets is coded, we should add this log at three different locations, instead of only one.

I can of course do it.

I can also rename pingPeers into pingPeersAndLogEnr.

renaming would be better then

Fixed in bd82d71.

nisdas · 2024-09-03T09:56:05Z

beacon-chain/p2p/subnets.go

+		if firstLoop {
+			firstLoop = false
+
+			log.WithFields(logrus.Fields{


why not just log this out of the loop without having to use a firstLoop boolean

Because the peerCountForTopic is computed inside the loop.

Let me propose a maybe better solution.

Fixed in 5e2af9f.

nisdas · 2024-09-03T09:57:59Z

beacon-chain/p2p/subnets.go

+		peerCountForTopic := len(s.pubsub.ListPeers(topic))
+
+		// Compute how many peers we are missing to reach the threshold.
+		missingPeersCount := max(0, threshold-peerCountForTopic)


Overflow if peerCountForTopic is greater than threshold

Both threshold and peerCountForTopic are (signed) int.

==> It will work as intended, since max(0, <negative number>) == 0.

nisdas · 2024-09-03T10:04:35Z

beacon-chain/p2p/subnets.go

+	index uint64,
+	threshold int,
+) (bool, error) {
+	const batchSize = 200


could you make the batch bigger ? Ex: 1000 or 2000

Fixed in 0b39d26.

…rror to debug.

beacon-chain/sync/subscriber.go

Co-authored-by: Nishant Das <[email protected]>

* `pingPeers`: Add log with new ENR when modified. * `p2p Start`: Use idiomatic go error syntax. * P2P `start`: Fix error message. * Use not bootnodes at all if the `--chain-config-file` flag is used and no `--bootstrap-node` flag is used. Before this commit, if the `--chain-config-file` flag is used and no `--bootstrap-node` flag is used, then bootnodes are (incorrectly) defaulted on `mainnet` ones. * `validPeersExist`: Centralize logs. * `AddConnectionHandler`: Improve logging. "Peer connected" does not really reflect the fact that a new peer is actually connected. --> "New peer connection" is more clear. Also, instead of writing `0`, `1`or `2` for direction, now it's writted "Unknown", "Inbound", "Outbound". * Logging: Add 2 decimals for timestamt in text and JSON logs. * Improve "no valid peers" logging. * Improve "Some columns have no peers responsible for custody" logging. * `pubsubSubscriptionRequestLimit`: Increase to be consistent with data columns. * `sendPingRequest`: Improve logging. * `FindPeersWithSubnet`: Regularly recheck in our current set of peers if we have enough peers for this topic. Before this commit, new peers HAD to be found, even if current peers are eventually acceptable. For very small network, it used to lead to infinite search. * `subscribeDynamicWithSyncSubnets`: Use exactly the same subscription function initially and every slot. * Make deepsource happier. * Nishant's commend: Change peer disconnected log. * NIshant's comment: Change `Too many incoming subscription` log from error to debug. * `FindPeersWithSubnet`: Address Nishant's comment. * `batchSize`: Address Nishant's comment. * `pingPeers` ==> `pingPeersAndLogEnr`. * Update beacon-chain/sync/subscriber.go Co-authored-by: Nishant Das <[email protected]> --------- Co-authored-by: Nishant Das <[email protected]>

nalepae added 12 commits August 30, 2024 13:37

pingPeers: Add log with new ENR when modified.

50b4f4b

p2p Start: Use idiomatic go error syntax.

7514b7b

P2P start: Fix error message.

e2220b5

Use not bootnodes at all if the --chain-config-file flag is used an…

d95407f

…d no `--bootstrap-node` flag is used. Before this commit, if the `--chain-config-file` flag is used and no `--bootstrap-node` flag is used, then bootnodes are (incorrectly) defaulted on `mainnet` ones.

validPeersExist: Centralize logs.

2857344

AddConnectionHandler: Improve logging.

be836a9

"Peer connected" does not really reflect the fact that a new peer is actually connected. --> "New peer connection" is more clear. Also, instead of writing `0`, `1`or `2` for direction, now it's writted "Unknown", "Inbound", "Outbound".

Logging: Add 2 decimals for timestamt in text and JSON logs.

0abfab4

Improve "no valid peers" logging.

959ff24

Improve "Some columns have no peers responsible for custody" logging.

2d446a2

pubsubSubscriptionRequestLimit: Increase to be consistent with data…

edb0fc1

… columns.

sendPingRequest: Improve logging.

c12b239

FindPeersWithSubnet: Regularly recheck in our current set of peers …

7d02a3c

…if we have enough peers for this topic. Before this commit, new peers HAD to be found, even if current peers are eventually acceptable. For very small network, it used to lead to infinite search.

nalepae force-pushed the fix-peerdas-disc branch from 47f70d7 to 876e8f6 Compare September 3, 2024 08:50

nalepae changed the title ~~Fix peerdas disc~~ Fix Initial Sync with 128 data columns subnets Sep 3, 2024

nalepae added the peerDAS label Sep 3, 2024

subscribeDynamicWithSyncSubnets: Use exactly the same subscription …

eee87bb

…function initially and every slot.

nalepae force-pushed the fix-peerdas-disc branch from 876e8f6 to eee87bb Compare September 3, 2024 08:53

nalepae marked this pull request as ready for review September 3, 2024 09:24

nalepae requested a review from a team as a code owner September 3, 2024 09:24

nalepae requested review from terencechain, nisdas and james-prysm and removed request for a team September 3, 2024 09:24

Make deepsource happier.

0af4c2e

nisdas reviewed Sep 3, 2024

View reviewed changes

nalepae added 5 commits September 3, 2024 12:21

Nishant's commend: Change peer disconnected log.

bd34800

NIshant's comment: Change Too many incoming subscription log from e…

ab0b0e8

…rror to debug.

FindPeersWithSubnet: Address Nishant's comment.

5e2af9f

batchSize: Address Nishant's comment.

0b39d26

pingPeers ==> pingPeersAndLogEnr.

bd82d71

nisdas reviewed Sep 3, 2024

View reviewed changes

beacon-chain/sync/subscriber.go Outdated Show resolved Hide resolved

Update beacon-chain/sync/subscriber.go

5c0cc4c

Co-authored-by: Nishant Das <[email protected]>

nisdas approved these changes Sep 3, 2024

View reviewed changes

nalepae merged commit 751a7a0 into peerDAS Sep 3, 2024
13 of 16 checks passed

nalepae deleted the fix-peerdas-disc branch September 3, 2024 13:58

nalepae mentioned this pull request Sep 12, 2024

Fix CPU usage in small devnets #14446

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Initial Sync with 128 data columns subnets #14403

Fix Initial Sync with 128 data columns subnets #14403

nalepae commented Aug 30, 2024 •

edited

Loading

nisdas Sep 3, 2024

nalepae Sep 3, 2024

nisdas Sep 3, 2024

nalepae Sep 3, 2024

nisdas Sep 3, 2024

nalepae Sep 3, 2024

nisdas Sep 3, 2024

nalepae Sep 3, 2024

nalepae Sep 3, 2024

nisdas Sep 3, 2024

nalepae Sep 3, 2024

nisdas Sep 3, 2024

nalepae Sep 3, 2024

nalepae Sep 3, 2024

nisdas Sep 3, 2024

nalepae Sep 3, 2024

nisdas Sep 3, 2024

nalepae Sep 3, 2024

Fix Initial Sync with 128 data columns subnets #14403

Fix Initial Sync with 128 data columns subnets #14403

Conversation

nalepae commented Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalepae commented Aug 30, 2024 •

edited

Loading