Swarm rather stable: update syncing by janos · Pull Request #1336 · ethersphere/swarm

janos · 2019-04-10T15:35:48Z

This is an experiment for changing update syncing functionality.

…date-syncing

janos · 2019-04-16T14:47:09Z

@zelig could you check latest changes in this pr?

zelig

excellent job @janos
approved pending mending /replacing skipped test (or you want annther pr?)

zelig · 2019-04-17T03:38:16Z

+// (with po peerPO) needs to be subscribed after kademlia neighbourhood depth
+// change from prevDepth to newDepth. Max argument limits the number of
+// proximity order bins. Returned values are slices of integers which represent
+// proximity order bins, the firs one to which additional subscriptions need to


firs -> first

zelig · 2019-04-17T03:53:14Z

+		},
+		{
+			po: 4, prevDepth: 0, newDepth: 4, // 0-16 -> 4-16
+			quitBins: intRange(0, 4),


odd one out using intRange

zelig · 2019-04-17T03:59:21Z

 }
+
+func (p *Peer) runUpdateSyncing() error {
+	timer := time.NewTimer(p.streamer.syncUpdateDelay)


why do you need this?
we used the delay globally in order to wait till healthy, not sure what is the purpose here

The purpose is to avoid very frequent updates when multiple peers are added added or removed in short period of time, like on swarm start or when there are connectivity issues. I think that this delay can be lowered, but I am still for keeping it.

zelig · 2019-04-17T04:02:17Z

+	t.Run("no new peers", func(t *testing.T) {
+		k := newTestKademlia(t, "00000000")
+
+		c, u := k.SubscribeToNeighbourhoodDepthChange()


more verbose varnames @frncmx?
like changeCand unsubscribe?

zelig · 2019-04-17T04:09:50Z

 	}
 }
+
+func (p *Peer) runUpdateSyncing() error {


maybe consider commenting these next 4 functions

janos · 2019-04-17T07:11:38Z

@zelig thanks for the review. I think that it would be better to fix the test in this pr, and I'll do it.

zelig · 2019-04-18T16:57:07Z

+func waitForSubscriptions(t *testing.T, r *Registry, ids ...enode.ID) {
+	t.Helper()
+
+	for reties := 0; reties < 100; reties++ {


reties -> retries

Thanks, corrected.

holisticode

Basically great work. I think it improves a lot on the problems we had with updateSyncing.

Please just make sure we actually have a test case for the original trigger to refactor this code (new peer enters nearest neighborhood without depth change and all subscriptions are made).

Also, there is an open question if we want to add the TestPureRetrieval test, which was added in the subs-by-peer branch (which was a precursor to this PR), to this PR, or if it should be added as a separate PR.

holisticode · 2019-04-25T21:31:05Z

+			}
+			waitForSubscriptions(t, pivotRegistry, ids...)
+
+			newDepth := pivotKademlia.NeighbourhoodDepth()


I think we should add here a check even if the depth did not change.

We started all this because we had situations where if a new peer was added and it did not change the depth, then no new subscriptions were added to this new node.

Is this tested somewhere? Should it be tested here or somewhere else?

Yes, good point. I will see to add this test case.

janos

Thanks Fabio, test TestPureRetrieval is great and probably is better to have its own PR.

janos · 2019-04-26T07:18:57Z

+			}
+			waitForSubscriptions(t, pivotRegistry, ids...)
+
+			newDepth := pivotKademlia.NeighbourhoodDepth()


Yes, good point. I will see to add this test case.

…date-syncing

…ngSubscriptions

nonsense · 2019-04-26T19:42:56Z

+		// nodes proximities from the pivot node
+		nodeProximities := make(map[string]int)
+		for _, id := range ids[1:] {
+			nodeProximities[id.String()] = chunk.Proximity(pivotKademlia.BaseAddr(), id.Bytes())


id is enode.ID, so id.Bytes() is not a bzz address. It appears that those proximities are not correct as a result.

Yes, you are right, and thanks for the PR.

nonsense · 2019-04-26T19:44:02Z

+	return subBins, quitBins
+}
+
+// subs returns the range to which proximity order bins syncing


comment seems to be wrong, at least the function name is different.

nonsense · 2019-04-26T19:47:55Z

+			}
+			// add new nodes to sync subscriptions check
+			for _, id := range ids {
+				nodeProximities[id.String()] = chunk.Proximity(pivotKademlia.BaseAddr(), id.Bytes())


Same comment about id.Bytes() here. I don't think this is an overlay bzz address, but rather an enode.ID.

nonsense · 2019-04-26T19:50:29Z

+	}
+
+	kad := p.streamer.delivery.kad
+	po := chunk.Proximity(network.NewAddr(p.Node()).Over(), kad.BaseAddr())


Having looked at network.NewAddr, I doubt this is used everywhere in Swarm, because our bzz addrs are different to enode.IDs, whereas this function is defined as:

func NewAddr(node *enode.Node) *BzzAddr { return &BzzAddr{OAddr: node.ID().Bytes(), UAddr: []byte(node.String())} }

I might be wrong, but it seems to me that network.NewAddr is used only in tests and simulations, and in production Swarm the BzzAddr is generated differently. See Swarm.Start where we actually override the Underlay Address, and we keep the previously set Overlay Address:

// update uaddr to correct enode newaddr := s.bzz.UpdateLocalAddr([]byte(srv.Self().String())) log.Info("Updated bzz local addr", "oaddr", fmt.Sprintf("%x", newaddr.OAddr), "uaddr", fmt.Sprintf("%s", newaddr.UAddr))

handleChunkDelivery also appears to be calculating proximity incorrectly based on enode.ID and not based on its BzzAddr:

case *ChunkDeliveryMsgRetrieval: msg = (*ChunkDeliveryMsg)(r) peerPO := chunk.Proximity(sp.ID().Bytes(), msg.Addr)

Good find, thanks for the fix in your PR.

zelig · 2019-04-29T03:28:39Z

 	r.peersMu.Lock()
 	r.peers[peer.ID()] = peer
 	metrics.GetOrRegisterGauge("registry.peers", nil).Update(int64(len(r.peers)))
+


this should not be hidden in setPeer but explicitly part of Run function

Yes, I agree, it is correct to have it in Run function. Changed.

zelig · 2019-04-29T03:36:04Z

+// to create new ones or quit existing ones based on the new neighbourhood depth
+// and if peer enters or leaves nearest neighbourhood by using
+// syncSubscriptionsDiff and updateSyncSubscriptions functions.
+func (p *Peer) runUpdateSyncing() {


this function should be called from Registry.Run.
There BzzAddr is available from BzzPeer
the remote overlay address should simply be passed as an argument to this function

BzzPeer is encapsulated by stream Peer, so that BzzAddr is available, in the latest change.

…Addr

nonsense

lgtm

…date-syncing

…s branch

janos added 2 commits April 10, 2019 17:32

swarm/storage/localstore: fix Put function call in Import

7013355

swarm/network, swarm/network/stream: new update syncing

f4bd03d

janos added the in progress label Apr 10, 2019

janos marked this pull request as ready for review April 11, 2019 08:21

zelig mentioned this pull request Apr 13, 2019

swarm: replace streamer updateSyncing with peer-based syncing #1325

Closed

swarm: merge branch 'swarm-rather-stable' into swarm-rather-stable-up…

3d61a43

…date-syncing

zelig assigned janos Apr 16, 2019

swarm/network: improve runUpdateSyncing

a809779

janos requested a review from zelig April 16, 2019 14:46

zelig approved these changes Apr 17, 2019

View reviewed changes

swarm/network: address pr comments

4455a0f

swarm/network/stream: add TestUpdateSyncingSubscriptions test

a87a7c5

janos changed the title ~~[WIP] Swarm rather stable: update syncing~~ Swarm rather stable: update syncing Apr 18, 2019

janos requested a review from zelig April 18, 2019 15:01

janos added ready for review and removed in progress labels Apr 18, 2019

janos requested review from frncmx, holisticode and nonsense April 18, 2019 15:01

zelig approved these changes Apr 25, 2019

View reviewed changes

swarm/network/stream: fix a typo in waitForSubscriptions test helper

f779414

holisticode suggested changes Apr 25, 2019

View reviewed changes

janos commented Apr 26, 2019

View reviewed changes

janos added 3 commits April 26, 2019 10:01

swarm: merge branch 'swarm-rather-stable' into swarm-rather-stable-up…

8784fb6

…date-syncing

swarm/network/stream: ensure no depth change check in TestUpdateSynci…

8571579

…ngSubscriptions

swarm/network/stream: add logs to runUpdateSyncing

b16cc7d

nonsense reviewed Apr 26, 2019

View reviewed changes

swarm/network/peer: update comment

a2a456b

zelig suggested changes Apr 29, 2019

View reviewed changes

janos added 3 commits April 29, 2019 09:53

swarm/network/stream: stream Peer encapsulates BzzPeer to provide Bzz…

4f8bc26

…Addr

swarm/network/stream: update handleQuitMsg

6655b99

swarm/network/stream: remove stream servers in quitSync

114eb4a

janos requested a review from zelig April 29, 2019 09:49

nonsense approved these changes Apr 29, 2019

View reviewed changes

janos added 2 commits April 29, 2019 12:14

swarm: merge branch 'swarm-rather-stable' into swarm-rather-stable-up…

e8ef29f

…date-syncing

swarm/network/stream: backport a SetNextBatch fix from simple-fetcher…

b1cb6ae

…s branch

zelig approved these changes Apr 29, 2019

View reviewed changes

holisticode mentioned this pull request Apr 29, 2019

swarm/network/stream: use bzzAddr for proximity calculation #1360

Closed

holisticode approved these changes Apr 29, 2019

View reviewed changes

nonsense merged commit 9c5be52 into swarm-rather-stable Apr 30, 2019

zelig mentioned this pull request May 3, 2019

syncing subscriptions should be requested when a peer connection is established #1329

Closed

acud deleted the swarm-rather-stable-update-syncing branch May 5, 2019 18:37

nonsense pushed a commit that referenced this pull request May 10, 2019

Swarm rather stable: update syncing (#1336)

09c7766

nonsense pushed a commit that referenced this pull request May 10, 2019

swarm/network: update syncing (#1336)

f26fa56

Conversation

janos commented Apr 10, 2019

Uh oh!

janos commented Apr 16, 2019

Uh oh!

zelig left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janos commented Apr 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

holisticode left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janos Apr 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nonsense left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

holisticode left a comment •

edited

Loading

janos Apr 29, 2019 •

edited

Loading