swarm/pss: forwarding function refactoring by gluk256 · Pull Request #1043 · ethersphere/swarm

gluk256 · 2018-12-04T13:45:31Z

fixes #995

zelig · 2018-12-04T21:32:00Z

prerequisite for ethereum/go-ethereum#18239

acud

@gluk256 I just reviewed this PR but unfortunately there's no background or description about what it solves, what was the bug and so. Please let's have more context so that we can review more effectively.

nolash · 2018-12-05T07:40:24Z

 	to := make([]byte, addressLength)
 	copy(to[:len(msg.To)], msg.To)
+	neighbourhoodDepth := p.Kademlia.NeighbourhoodDepth()
+	luminousRadius := len(msg.To) * 8


In that case, I think the correct term would be luminosityRadius

Please change this name as noted @gluk256 because it's not grammatically correct.

nolash · 2018-12-05T07:52:54Z

+		// address, if there are any; otherwise only to one peer, closest to the recipient address.
+		// in any case, msg must be sent to at least one peer.
+		p.Kademlia.EachConn(to, addressLength*8, func(sp *network.Peer, po int, _ bool) bool {
+			isAddrMatch := (po == luminousRadius)


I think the po can be bigger here, no?

yes, changed

nolash · 2018-12-05T07:54:10Z

+					sent++
+				}
+			}
+			mustContinue := (isAddrMatch || sent == 0)


Put this before the if line 959 and replace in if-condition please, please.

this is not the same, because sent++ might be incremented

You're right. So maybe:

if !p.trySend(sp, msg) { return true } [...] return isAddrMatch

instead?

nolash · 2018-12-05T07:56:27Z

+	if isDstInProxBin {
+		// forward to all the nearest neighbours of the forwarding node
+		p.Kademlia.EachConn(p.BaseAddr(), addressLength*8, func(sp *network.Peer, _ int, isproxbin bool) bool {
+			if isproxbin {


We agreed to remove isproxbin as it's wrong and it's only used here. Please calculate this inside the iterator.

Edit after talking about this: We need to refactor EachConn to eliminate possibility of wrong use with wrong param.

nolash · 2018-12-05T07:58:45Z

-		p.fwdPoolMu.RLock()
-		pp := p.fwdPool[sp.Info().ID]
-		p.fwdPoolMu.RUnlock()
+			mustContinue := isproxbin


Put this assignment as first thing in the iterator and use that var instead?

i think this is better, because otherwise i will need to write a comment in plain language

gluk256

Here is description of what this function is supposed to do:
If the recipient address (or partial address) is within the neighbourhood depth of the forwarding node, then it will be forwarded to all the nearest neighbours of the forwarding node. In case of partial address, it should be forwarded to all the peers matching the partial address, if there are any; otherwise only to one peer, closest to the recipient address. In any case, if the message forwarding fails, the node should try to forward it to the next best peer, until the message is successfully forwarded to at least one peer.

acud · 2018-12-06T06:00:46Z

@gluk256 and could you please say what it was doing before the fix?
also, if we could PR this into the documentation/write this into the wiki that would be great.

nolash · 2018-12-06T08:56:14Z

+					sent++
+				}
+			}
+			mustContinue := (isAddrMatch || sent == 0)


You're right. So maybe:

if !p.trySend(sp, msg) { return true } [...] return isAddrMatch

instead?

nolash · 2018-12-06T08:56:50Z

 	to := make([]byte, addressLength)
 	copy(to[:len(msg.To)], msg.To)
+	neighbourhoodDepth := p.Kademlia.NeighbourhoodDepth()
+	luminousRadius := len(msg.To) * 8


Please change this name as noted @gluk256 because it's not grammatically correct.

nolash · 2018-12-06T09:01:58Z

-		pp := p.fwdPool[sp.Info().ID]
-		p.fwdPoolMu.RUnlock()
+			mustContinue := isPeerInProxBin
+			return mustContinue


I understand your point about the comment.

But "explaining through varnames" is rather unusual in our code.

We tend to add a comment instead (that is, when we remember to). I think it makes sense to try to maintain a minimum of style coordination through our code so that the reader doesn't have to interpret that as well as the logic.

So I think:

// if we are in proxbin continue forwarding return isPeerInProxBin

Is better.

see my algo, WDYT?

zelig · 2018-12-06T07:24:16Z

-		p.fwdPoolMu.RLock()
-		pp := p.fwdPool[sp.Info().ID]
-		p.fwdPoolMu.RUnlock()
+			mustContinue := isPeerInProxBin


zelig · 2018-12-07T07:58:13Z

+	// luminosity is the opposite of darkness. the more bytes are removed from the address, the higher is darkness,
+	// but the luminosity is less. here luminosity equals the number of bits present in the destination address.
+	luminousRadius := len(msg.To) * 8
+	if luminousRadius >= neighbourhoodDepth {


this is quite convoluted esp without comments.

i think this is so much simpler:

pof := pot.DefaultPof(neighbourhoodDepth) // pof function matching upto neighbourhoodDepth bits depth, full := pof(to, p.BaseAddr(), 0) // if full match, then depth == neighbourhoodDepth, we broadcast to all neighbours. if !full { // if not full match, we broadcast upto luminousityRadius // if to is not partial address, luminosityRadius is 256 so po<depth will be true after the first depth = luminosityRadius } p.Kademlia.EachConn(to, addressLength*8, func(sp *network.Peer, po int, _ bool) bool { if po < depth && sent > 0 { return false } if p.trySend(sp, msg) { sent++ } }

zelig · 2018-12-07T07:59:01Z

-		pp := p.fwdPool[sp.Info().ID]
-		p.fwdPoolMu.RUnlock()
+			mustContinue := isPeerInProxBin
+			return mustContinue


see my algo, WDYT?

zelig

LGTM

nonsense · 2018-12-13T14:41:29Z

+	testForwardMsg(900, t, ps, peerAddresses[19][:1], peerAddresses, all[16:])
+}
+
+func testForwardMsg(num int, t *testing.T, ps *Pss, addr []byte, addresses []pot.Address, expected []int) {


A comment on this function to explain what it is testing and what is expected would be nice.

nonsense · 2018-12-13T14:42:39Z

-func (p *Pss) forward(msg *PssMsg) error {
-	metrics.GetOrRegisterCounter("pss.forward", nil).Inc(1)
+// tries to send a message, returns true if successful
+func trySendMsg(p *Pss, sp *network.Peer, msg *PssMsg) bool {


Why is this function called trySendMsg and not just sendMsg? Pretty much everything in Go can return an error, so no need to include try in front, as it's clear the action might error?

nonsense · 2018-12-13T14:46:25Z

+// are any; otherwise only to one peer, closest to the recipient address. In any case, if the message
+// forwarding fails, the node should try to forward it to the next best peer, until the message is
+// successfully forwarded to at least one peer.
+func (p *Pss) forward(msg *PssMsg, trySend func(p *Pss, sp *network.Peer, msg *PssMsg) bool) error {


It looks like we are defining trySend function only because we want to override it in tests. If this is the case, wouldn't it be cleaner if trySend is just defined as a var in this package, and the test package overrides it, instead of exposing this behaviour in the interface/signature of the function?

brilliant pattern also used by @janos in new localstore now function

nolash · 2018-12-13T17:43:51Z

+	ps := createPss(t, kad)
+	addPeers(kad, peerAddresses)
+
+	const firstNearest = depth * 2 // first peer in the nearest neighbours' bin


Maybe it would be easier to understand if we just use literal numbers?

then it would be very difficult to change the test. remembering all the magic numbers is impossible. now you only need to change one constant (depth).

Ok. Should we maybe add a comment saying "add some peers far away from the others". depth * 2 is a bit obscure, no? :)

nolash · 2018-12-13T17:44:21Z

+
+	const firstNearest = depth * 2 // first peer in the nearest neighbours' bin
+	nearestNeighbours := []int{firstNearest, firstNearest + 1, firstNearest + 2}
+	//fmt.Println(kad.String()) // print kademlia map for debugging, before any test starts


Please remove unused comment

nolash · 2018-12-13T17:48:35Z

+	}
+}
+
+func createPss(t *testing.T, kad *network.Kademlia) *Pss {


Why not use pss_test.go:newTestPss()?

because i wanted to use specific sttings (e.g. base address)

I understand, but now we have at least three different functions for setting up pss :'(

How about expanding the existing one with a chained method? This is a convention we've agreed to use in Swarm.

nolash · 2018-12-13T17:54:21Z

+	return network.NewPeer(bp, kad)
+}
+
+func newTestMsg(addr []byte) *PssMsg {


I see code already existing in pss_test.go etc already creates messages inline in the test. Having a testmessage "factory" is fine, but then we should endeavour to use it the same way everywhere.

Also, I suggest it should generate random data for the payload data, and take a topic param.

Refactoring all that is of course not within scope of this PR.

Perhaps as a first step towards consolidation is that we put this method (and similar generic methods) in a file called common_test.go.

@gluk256 You didn't want to do anything with this?

nolash · 2018-12-13T17:58:40Z

+		}
+	}
+	if !isPssEnabled {
+		log.Trace("peer doesn't have matching pss capabilities, skipping", "peer", info.Name, "caps", info.Caps)


Just occurred to me this should likely be a log.Error. Running swarm with Pss used to be optional, but it is no longer.

nolash · 2018-12-13T18:22:04Z

+)
+
+func TestForwardBasic(t *testing.T) {
+	base := newBaseAddress() // 0xFFFFFF.......


Wouldn't just using zeros be easier?

since we compare distances (xor), it's easier for me to identify 1s then 0s.

Heh, yeah I sympathize. But for people reading the code later, though, I think the more browsing back and forth in the code we force them to do, the harder it is to get the overview.

nolash · 2018-12-13T18:26:57Z

+	testForwardMsg(900, t, ps, peerAddresses[19][:1], peerAddresses, all[16:])
+}
+
+func testForwardMsg(num int, t *testing.T, ps *Pss, addr []byte, addresses []pot.Address, expected []int) {


What does the num parameter here do?

And please consider more informative varnames than addr and addresses :)

nolash · 2018-12-13T18:38:20Z

+	var fail bool
+	s := fmt.Sprintf("test id: %d, msg address: %x..., radius: %d", num, addr[:len(addr)%4], 8*len(addr))
+
+	// false negatives


Please describe what false negatives and false positives mean in this context. I assume:

false negative is expected message didn't reach peer

false positive is unexpected message reached peer

zelig · 2018-12-14T01:34:49Z

+// are any; otherwise only to one peer, closest to the recipient address. In any case, if the message
+// forwarding fails, the node should try to forward it to the next best peer, until the message is
+// successfully forwarded to at least one peer.
+func (p *Pss) forward(msg *PssMsg, trySend func(p *Pss, sp *network.Peer, msg *PssMsg) bool) error {


brilliant pattern also used by @janos in new localstore now function

zelig · 2018-12-14T01:35:50Z

+	neighbourhoodDepth := p.Kademlia.NeighbourhoodDepth()
+
+	// luminosity is the opposite of darkness. the more bytes are removed from the address, the higher is darkness,
+	// but the luminosity is less. here luminosity equals the number of bits present in the destination address.


i think given is best

nolash

I am mortified to realize that I actually didn't read through and evaluate that the test cases themselves are ok. Expect this in a separate review following shortly.

nolash · 2018-12-14T14:36:11Z

+	ps := createPss(t, kad)
+	addPeers(kad, peerAddresses)
+
+	const firstNearest = depth * 2 // first peer in the nearest neighbours' bin


Ok. Should we maybe add a comment saying "add some peers far away from the others". depth * 2 is a bit obscure, no? :)

nolash · 2018-12-14T14:37:17Z

+	}
+
+	for i := 0; i < firstNearest; i++ {
+		// send random messages with different proximity orders


not just different, but every bin in order even.

i added two peers for no other reason than to simulate the system as close to production as possible. concerning the firstNearest = depth * 2, this formula helps to change the depth var, without need to go through the code and fix all other variables as well.

nolash · 2018-12-14T14:38:01Z

+		testForwardMsg(300+i, t, ps, dst[:], peerAddresses, nearestNeighbours)
+	}
+
+	// send msg with proximity order higher than the last nearest neighbour


Ambiguous. Is "last" deeper or shallower?

nolash · 2018-12-14T14:39:22Z

+}
+
+// this function tests the forwarding of a single message. the recipient address is passed as param,
+// along with addreses of all peers, and indexes of those peers which are expected to receive the message.


addresses, indexes -> indices

nolash · 2018-12-14T14:42:57Z

+	}
+}
+
+func createPss(t *testing.T, kad *network.Kademlia) *Pss {


I understand, but now we have at least three different functions for setting up pss :'(

How about expanding the existing one with a chained method? This is a convention we've agreed to use in Swarm.

nolash · 2018-12-14T14:43:22Z

+	return ps
+}
+
+func newBaseAddress() pot.Address {


So you want to keep this, then?

nolash · 2018-12-14T14:43:56Z

+	return network.NewPeer(bp, kad)
+}
+
+func newTestMsg(addr []byte) *PssMsg {


@gluk256 You didn't want to do anything with this?

nolash · 2018-12-14T14:45:26Z

-			metrics.GetOrRegisterCounter("pss.pp.send.error", nil).Inc(1)
-			log.Error(err.Error())
-			return true
+	// soft threshold for msg broadcast


I promise to think of a more elaborate and enlightening comment here, but let's not let that stop the merging.

nolash

Otherwise LGTM.

nolash · 2018-12-17T09:15:46Z

+	ps := createPss(t, kad)
+	addPeers(kad, peerAddresses)
+
+	const firstNearest = depth // first peer in the nearest neighbours' bin


please let's stick to the terms shallower/deeper .. first is ambiguous

nolash · 2018-12-17T09:18:09Z

+		testForwardMsg(600+i, t, ps, peerAddresses[i][:part], peerAddresses, nearestNeighbours)
+	}
+
+	// partial address with proximity order higher than the last nearest neighbour


Here too; higher

nolash · 2018-12-17T09:24:02Z

+	all := []int{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11}
+	testForwardMsg(800, t, ps, []byte{}, peerAddresses, all)
+
+	// luminous radius of one byte (8 bits)


should we instead have a case where the luminosity radius covers more than just one shallower than depth.

It's maybe over-cautios, but the reason would be to fully guarantee that the iterator doesn't do either of:

Include the first shallower bin after depth and terminate.

Continue "spamming" only if you hit depth on the second iteration.

But feel free to ignore this if you think it's not needed.

zelig · 2018-12-17T10:37:29Z

+// this function substitutes the real send function, since
+// we only want to test the peer selection functionality
+func dummySendMsg(_ *Pss, sp *network.Peer, _ *PssMsg) bool {
+	a := pot.NewAddressFromBytes(sp.Address())


just Hex the Address

i don't understand: what should be done here? and what's wrong with existing implementation?

zelig · 2018-12-17T10:38:45Z

+	return true
+}
+
+// setDummySendMsg replaces sendMessage function for testing purposes


you dont need these functions, just use this pattern:

in the code:

// default value for production var sendFunc = send // the send function used as arg to forward in production via assignment to `sendFunc` func send(...) ...

in the test:

testSendFunc := func(..... ... } // this defer does closure with the current value of sendFunc so it will reset to its orig value when test returns defer func(t) { sendFunc = t }(sendFunc) sendFunc = testSendFunc

zelig · 2018-12-17T10:53:45Z

+// and kademlia constellation.
 func TestForwardBasic(t *testing.T) {
-	base := newBaseAddress() // 0xFFFFFF.......
+	setDummySendMsg()


see my comment above for a simpler pattern to use here

zelig · 2018-12-17T11:12:49Z

 	return nil
 }

+// sendMessage is a helper function that tries to send a message and returns true on success


call it sendFunc and see the pattern proposed

zelig · 2018-12-17T12:10:15Z

-	testForwardMsg(900, t, ps, peerAddresses[19][:1], peerAddresses, all[16:])
+
+	// luminous radius of one byte (8 bits)
+	testForwardMsg(900, t, ps, baseAddrBytes[:1], peerAddresses, all[8:])


so we are missing a lot of cases:

test which shows that within the bin, the forward function selects the closest peer to the address first

test which shows that if luminosity is < depth but not 0 then it sends to all peers deeper than luminosity

test which shows what happens if send returns false (send error)

Therefore I really suggest using a pattern.

type testCase struct{ name string recipient []byte errors: []int expected: []int peers: [][]byte } testCases := []{ { expected: all, peers: peers, }, ... { recipient: RandomAddrAt(pivot, 2), errors: []bool{false, true}, expected: []int{3}, peers: peers, } ... for _, tc := range testCases { t.Run(tc.Name, func(t *testing.T) { if err := testForward(tc); err != nil { t.Fatal(err) } }

func testForwardMsg(tc testCase) error { testSendFunc := func(... // use tc fields } // set sendFunc, defer reset // call forward on pss }

test which shows that if luminosity is < depth but not 0 then it sends to all peers deeper than luminosity

this is covered already no?

zelig · 2018-12-19T22:25:12Z

+var sendFunc func(p *Pss, sp *network.Peer, msg *PssMsg) bool = sendMessageProd
+
+// tries to send a message, returns true if successful
+func sendMessageProd(p *Pss, sp *network.Peer, msg *PssMsg) bool {


send or sendMsg pls

nolash and others added 2 commits November 29, 2018 11:12

swarm/pss: Rebase on master after kad depth change + handler refactor

e52c5ec

swarm/pss forwarding function refactored

8805f56

gluk256 requested review from nolash, nonsense and zelig as code owners December 4, 2018 13:45

zelig assigned gluk256 Dec 4, 2018

zelig added in progress pss push-sync network bug labels Dec 4, 2018

zelig added this to the 2018 Christmas Edition milestone Dec 4, 2018

zelig mentioned this pull request Dec 4, 2018

swarm/pss: Remove spurious forwarding #1008

Closed

2 tasks

acud reviewed Dec 5, 2018

View reviewed changes

Comment thread swarm/pss/pss.go Outdated

acud reviewed Dec 5, 2018

View reviewed changes

nolash suggested changes Dec 5, 2018

View reviewed changes

gluk256 commented Dec 5, 2018

View reviewed changes

swarm/pss: variables renamed, comment added

f48a3f3

gluk256 added the ready for review label Dec 5, 2018

nolash suggested changes Dec 6, 2018

View reviewed changes

zelig suggested changes Dec 7, 2018

View reviewed changes

swarm/pss: new version of pss.forward proposed by Viktor

af44980

zelig approved these changes Dec 11, 2018

View reviewed changes

acud removed the in progress label Dec 12, 2018

swarm/pss: forwarding test added, bugs fixed

f1eff7b

nonsense reviewed Dec 13, 2018

View reviewed changes

Comment thread swarm/pss/forwarding_test.go

nonsense reviewed Dec 13, 2018

View reviewed changes

Comment thread swarm/pss/forwarding_test.go

nonsense reviewed Dec 13, 2018

View reviewed changes

Comment thread swarm/pss/pss.go

nonsense reviewed Dec 13, 2018

View reviewed changes

nolash suggested changes Dec 13, 2018

View reviewed changes

zelig suggested changes Dec 14, 2018

View reviewed changes

gluk256 added 4 commits December 14, 2018 13:31

swarm/pss: comments added

bcf32d2

swarm/pss: changed comments, refactored helper func sendMessage

dd027ed

swarm/pss: variable renamed, comment added

7cd1483

swarm/pss: variable renamed

f28c0ca

nolash suggested changes Dec 14, 2018

View reviewed changes

swarm/pss: test function refactored

ad9e734

nolash reviewed Dec 17, 2018

View reviewed changes

Comment thread swarm/pss/forwarding_test.go Outdated

swarm/pss: test updated (one peer added)

c8d6458

nolash approved these changes Dec 17, 2018

View reviewed changes

zelig suggested changes Dec 17, 2018

View reviewed changes

zelig mentioned this pull request Dec 18, 2018

kademlia, connectivity, stability -- meta #1068

Closed

53 tasks

swarm/pss: bugfix (msg should only be sent to one nearest peer)

a7350bd

zelig suggested changes Dec 19, 2018

View reviewed changes

swarm/pss: func name changed

3e7aa78

zelig approved these changes Dec 20, 2018

View reviewed changes

zelig added ready to submit to ethereum/go-ethereum and removed ready for review labels Dec 20, 2018

nonsense mentioned this pull request Dec 21, 2018

swarm/pss: forwarding function refactoring ethereum/go-ethereum#18353

Merged

nonsense closed this Dec 21, 2018

Conversation

gluk256 commented Dec 4, 2018 • edited by zelig Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zelig commented Dec 4, 2018

Uh oh!

Uh oh!

acud left a comment

Choose a reason for hiding this comment

Uh oh!

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!

gluk256 left a comment

Choose a reason for hiding this comment

Uh oh!

acud commented Dec 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nolash Dec 6, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zelig left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

gluk256 commented Dec 4, 2018 •

edited by zelig

Loading

nolash Dec 6, 2018 •

edited

Loading