[bee #48] upload syncs to closest - add SyncPeer method #97

acud · 2020-04-15T11:05:11Z

Imports Distance functionality from swarm to resolve certain ambiguities regarding which peer to route to when using only po function.

as part of #48

nolash · 2020-04-16T12:58:29Z

pkg/swarm/distance.go

+
+// DistanceRaw returns the distance between address x and address y in big-endian binary format using the distance metric defined in the swarm specfication
+// Fails if not all addresses are of equal length
+func DistanceRaw(x, y []byte) ([]byte, error) {


DistanceBytes?

nolash · 2020-04-16T12:59:19Z

pkg/swarm/distance.go

+// DistanceCmp compares x and y to a in terms of the distance metric defined in the swarm specfication
+// it returns:
+// 	1 if x is closer to a than y
+// 	0 if x and y are equally far apart from (this means that x and y are the same address)


nolash · 2020-04-16T13:00:34Z

pkg/swarm/distance.go

+	return ProxCmp(a, y, x), nil
+}
+
+// ProxCmp compares the distances x->a and y->a


It is not clear why you need both of these. It seems that all the DistanceCmp adds is a length check. If so, perhaps this function should not be exported.

unrelated, but the original implementer chose to swap x and y when calling ProxCmp...(L47)

i hope no one used this....

nolash · 2020-04-16T13:03:34Z

pkg/swarm/distance_test.go

+		{
+			x:      MustParseHexAddress("9100000000000000000000000000000000000000000000000000000000000000").Bytes(),
+			y:      MustParseHexAddress("8200000000000000000000000000000000000000000000000000000000000000").Bytes(),
+			z:      MustParseHexAddress("1200000000000000000000000000000000000000000000000000000000000000").Bytes(),


Why not use same letters as in implementation for clarity? z -> y a -> x x -> y

you tell me... its your implementation :D

nolash · 2020-04-16T13:06:37Z

pkg/topology/full/full.go

@@ -130,6 +133,34 @@ func (d *Driver) ChunkPeer(addr swarm.Address) (peerAddr swarm.Address, err erro
 	return swarm.Address{}, topology.ErrNotFound
 }

+func (d *Driver) SyncPeer(addr swarm.Address) (peerAddr swarm.Address, err error) {


Papers, please

nolash · 2020-04-16T13:08:23Z

pkg/topology/full/full.go

+
+	cpeer := d.base // start checking closest from _self_
+	for _, peer := range connectedPeers {
+		switch d := swarm.ProxCmp(addr.Bytes(), cpeer.Bytes(), peer.Address.Bytes()); d {


I find it more readable to split such in two lines. Feel free to ignore this message.

nolash · 2020-04-16T13:10:15Z

pkg/topology/full/full.go

+		}
+	}
+
+	// check if node is actually the closest one to the chunk


check if self...?

i can english

nolash · 2020-04-16T13:11:26Z

pkg/topology/full/full.go

+
+	// check if node is actually the closest one to the chunk
+	if cpeer.Equal(d.base) {
+		return swarm.Address{}, topology.ErrWantSelf


topology.Narcissus

you mean topology.LoneGunman

nolash · 2020-04-16T13:16:58Z

pkg/topology/full/full_test.go

@@ -186,6 +186,83 @@ func TestAddPeer(t *testing.T) {
 	})
 }

+// TestSyncPeer tests that SyncPeer method returns closest connected peer


... for any given chunk

nolash · 2020-04-16T13:19:49Z

pkg/topology/topology.go

@@ -25,3 +27,7 @@ type PeerAdder interface {
 type ChunkPeerer interface {
 	ChunkPeer(addr swarm.Address) (peerAddr swarm.Address, err error)
 }
+
+type SyncPeerer interface {


"Peerer" ... is that even a word?

go convention, let's not get into this

janos · 2020-04-16T12:42:04Z

pkg/swarm/distance.go

+// Distance returns the distance between address x and address y as a (comparable) big integer using the distance metric defined in the swarm specification
+// Fails if not all addresses are of equal length


Comments with full sentences.

Applies to all comments in this package.

acud · 2020-04-16T16:00:13Z

@janos i pulled the closestPeer functionality in full.go to a separate function, as this will be needed for ChunkPeer method too.

acud · 2020-04-16T16:01:10Z

pkg/topology/full/full.go

+			// do nothing
+		case -1:
+			// current peer is closer
+			closest = peer


@janos do i need to shadow peer on L156 as a result?

I do not think that it is needed.

janos · 2020-04-16T17:19:10Z

pkg/topology/full/full.go

+			// do nothing
+		case -1:
+			// current peer is closer
+			closest = peer


I do not think that it is needed.

nolash · 2020-04-17T11:04:23Z

pkg/swarm/distance_test.go

+	distanceCmpTests = []distanceCmpTest{
+		{
+			a:      MustParseHexAddress("9100000000000000000000000000000000000000000000000000000000000000").Bytes(), // 10010001
+			x:      MustParseHexAddress("8200000000000000000000000000000000000000000000000000000000000000").Bytes(), // 10000010 xor(91,82) =	9


nolash · 2020-04-17T11:05:27Z

pkg/swarm/distance_test.go

+		{
+			a:      MustParseHexAddress("9100000000000000000000000000000000000000000000000000000000000000").Bytes(), // 10010001
+			x:      MustParseHexAddress("8200000000000000000000000000000000000000000000000000000000000000").Bytes(), // 10000010 xor(91,82) =	9
+			y:      MustParseHexAddress("1200000000000000000000000000000000000000000000000000000000000000").Bytes(), // 00010010 xor(91,12) = 87


sorry 83 of course

nolash · 2020-04-17T11:19:11Z

pkg/swarm/distance.go

+		dy := y[i] ^ a[i]
+		if dx == dy {
+			continue
+		} else if dx < dy {


Good catch!

nolash · 2020-04-17T11:22:17Z

pkg/swarm/distance_test.go

+		{
+			x:      MustParseHexAddress("9100000000000000000000000000000000000000000000000000000000000000").Bytes(),
+			y:      MustParseHexAddress("8200000000000000000000000000000000000000000000000000000000000000").Bytes(),
+			result: "8593944123082061379093159043613555660984881674403010612303492563087302590464",


Oh, an easily verifiable number. Any reason why we have to run these testes with 32 byte ints?

you mean why we don't?

nolash · 2020-04-17T11:25:44Z

pkg/topology/full/full_test.go

@@ -186,33 +186,19 @@ func TestAddPeer(t *testing.T) {
 	})
 }

-// TestSyncPeer tests that SyncPeer method returns closest connected peer
+// TestSyncPeer tests that SyncPeer method returns closest connected peer to a given chunk


nolash · 2020-04-17T11:26:31Z

pkg/topology/full/full.go

+	return closestPeer(addr, d.base, overlays)
+}
+
+func closestPeer(addr, self swarm.Address, peers []swarm.Address) (swarm.Address, error) {


let's add a comment just to be nice.

acud assigned jmozah and acud Apr 15, 2020

acud requested a review from janos April 15, 2020 11:05

acud added the in progress ongoing development , hold out with review label Apr 15, 2020

acud requested a review from nolash April 16, 2020 10:18

acud added 10 commits April 16, 2020 12:19

wip skeleton

ad9de9c

wip add tests for full topology SyncPeer functionality

0f57e2c

trivial test cases pass

f5d9400

add test case for peer wants self

a60821f

fix build

3df2cd3

add distance metrics

95f9f17

copy

b939358

copy

5f49d02

wip

61facbd

fix tests to use distance expectancies

70357d6

acud force-pushed the upload-to-closest branch from c793514 to 70357d6 Compare April 16, 2020 10:19

cleanup

f3e8881

acud added ready for review The PR is ready to be reviewed and removed in progress ongoing development , hold out with review labels Apr 16, 2020

cleanup

bd5cf88

nolash reviewed Apr 16, 2020

View reviewed changes

janos reviewed Apr 16, 2020

View reviewed changes

acud added 5 commits April 16, 2020 17:01

fix lash code

0488871

fix more lash code

6aee7a4

fix my code

ef76e29

fix nolash nitpick

1e0a590

more nitpicks

067aca5

acud requested review from janos and nolash April 16, 2020 15:45

extracted closestPeer to separate function

2438f90

acud commented Apr 16, 2020

View reviewed changes

janos approved these changes Apr 16, 2020

View reviewed changes

pkg/topology/full/full.go

// do nothing

case -1:

// current peer is closer

closest = peer

Copy link

Member

janos Apr 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think that it is needed.

nolash reviewed Apr 17, 2020

View reviewed changes

more nitpicking

58d427d

acud requested a review from nolash April 17, 2020 11:50

nolash approved these changes Apr 17, 2020

View reviewed changes

fix xor value comment

9a391f5

acud merged commit 7e40e57 into master Apr 17, 2020

acud mentioned this pull request Apr 17, 2020

[bee #49]: get closest peer #109

Merged

acud deleted the upload-to-closest branch September 26, 2020 10:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[bee #48] upload syncs to closest - add SyncPeer method #97

[bee #48] upload syncs to closest - add SyncPeer method #97

acud commented Apr 15, 2020 •

edited

nolash Apr 16, 2020

acud Apr 16, 2020

nolash Apr 16, 2020

nolash Apr 16, 2020

acud Apr 16, 2020

acud Apr 16, 2020

nolash Apr 16, 2020

acud Apr 16, 2020

nolash Apr 16, 2020

nolash Apr 16, 2020

nolash Apr 16, 2020

acud Apr 16, 2020

nolash Apr 16, 2020

acud Apr 16, 2020

nolash Apr 16, 2020

nolash Apr 16, 2020

acud Apr 16, 2020

janos Apr 16, 2020

acud commented Apr 16, 2020

acud Apr 16, 2020

janos Apr 16, 2020

janos Apr 16, 2020

nolash Apr 17, 2020

nolash Apr 17, 2020

nolash Apr 17, 2020

nolash Apr 17, 2020

nolash Apr 17, 2020

acud Apr 17, 2020

nolash Apr 17, 2020

nolash Apr 17, 2020

		// Distance returns the distance between address x and address y as a (comparable) big integer using the distance metric defined in the swarm specification
		// Fails if not all addresses are of equal length

[bee #48] upload syncs to closest - add SyncPeer method #97

[bee #48] upload syncs to closest - add SyncPeer method #97

Conversation

acud commented Apr 15, 2020 • edited

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

acud commented Apr 16, 2020

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

acud commented Apr 15, 2020 •

edited