Skip to content
This repository has been archived by the owner on Aug 2, 2021. It is now read-only.

pot: Add Distance methods with tests #1621

Merged
merged 7 commits into from
Aug 14, 2019
Merged

pot: Add Distance methods with tests #1621

merged 7 commits into from
Aug 14, 2019

Conversation

nolash
Copy link
Contributor

@nolash nolash commented Jul 30, 2019

Adds methods for calculating distance metric value, as well as comparison.

Comparison of XOR distance requires the addresses to be compared to be the same length.

The change to proxCmp is merely a slight bumming.

@nolash nolash requested review from janos and zelig July 30, 2019 12:13
@nolash nolash self-assigned this Jul 30, 2019
@nolash nolash requested a review from nonsense July 31, 2019 16:24
zelig
zelig previously approved these changes Aug 4, 2019
View reviewed changes
Copy link
Member

@zelig zelig left a comment

Choose a reason for hiding this comment

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

where exactly will these be used?
I dont mind it here at all, but have you considered the chunk package where Proximity is?

pot/address.go Outdated Show resolved Hide resolved
pot/address.go Outdated Show resolved Hide resolved
@nolash
Copy link
Contributor Author

nolash commented Aug 5, 2019

@zelig

where exactly will these be used?

First thought was the prox test for pss. Since chunk and pss do not explicitly share ancestry. In my view this has more to do with network and routing than the chunk per se. Thus I considered network or pot package, and for consistency I chose pot because the other "closeness" metric ProxCmp is found there.

janos
janos reviewed Aug 6, 2019
View reviewed changes
pot/address_test.go Outdated Show resolved Hide resolved
pot/address.go Outdated Show resolved Hide resolved
@acud
Copy link
Member

acud commented Aug 7, 2019

@nolash I had a look at this and would like to share my 2 cents as I was thinking about implementing this at the time.

I think it would be nice to have distance as a type, returned alongside with the po from chunk.Proximity. in the sense that the chunk.Proximity signature would be changed to the following:

type XORDistance []byte

func Proximity(one, other []byte) (ret int, d XORDistance) {
...
}

On top of XORDistance (or whatever you choose to call it), you could have the comparison methods in the form of receiver methods:

func (x XORDistance) Compare(...)

and so forth...

In regards to representing distance with bin.Int - not sure if this is the correct way to go.
The distance metric is in fact a XOR between two uint256 (link), so I'm not sure if big.Int would actually suffice for correct comparison without overflows (but then again I have not intimate knowledge of the type).

Also, looking at the code shows that you're not using any of the native comparators of the actual underlying type but you just iterate over the byte slice and do manual comparison, so maybe just stick to the byte slice representation?

WDYT?

@nolash
Copy link
Contributor Author

nolash commented Aug 7, 2019 •
edited
Loading

@acud

so I'm not sure if big.Int would actually suffice for correct comparison without overflows

Well with an XOR of two byte vectors of same length you can't really have overflows?

big.Int should be up for the challenge. It handles any arbitrary size. How useful the value is in the end I am not so sure, though. Generally we probably are just interested in the comparison?

I think it would be nice to have distance as a type

Would that be a concrete type, sir? ;)

returned alongside with the po from chunk.Proximity

Are you sure we'd want to perform the calculation every time?

@acud
Copy link
Member

acud commented Aug 8, 2019

Are you sure we'd want to perform the calculation every time?

if by "calculation" you mean "XORing" the two addresses, then yes, and that should not be a burden because we are doing so anyway in the proximity function. The only difference would be that the Proximity function won't stop XORing when the first different bit is occured, but will continue till the end of the address length in order to get the full XOR distance to return.

Would that be a concrete type, sir? ;)

Not sure if I get it...

janos
janos previously approved these changes Aug 9, 2019
View reviewed changes
@nolash
Copy link
Contributor Author

nolash commented Aug 9, 2019 •
edited
Loading

Had a call with @acud today and we agreed a good approach is to keep distance in the pot package, and in followup PR re-use (and if necessary adapt) proximity code in pot instead of proximity function in chunk.

janos
janos previously approved these changes Aug 9, 2019
View reviewed changes
Copy link
Member

@janos janos left a comment

Choose a reason for hiding this comment

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

LGTM.

acud
acud previously approved these changes Aug 12, 2019
View reviewed changes
@nolash nolash dismissed stale reviews from acud and janos via d54341e August 12, 2019 09:18
zelig
zelig approved these changes Aug 14, 2019
View reviewed changes
Copy link
Member

@zelig zelig left a comment

Choose a reason for hiding this comment

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

remove on commented line

network/discovery.go Outdated
@@ -89,7 +89,8 @@ func NotifyPeer(p *BzzAddr, k *Kademlia) {
// unless already notified during the connection session
func (d *Peer) NotifyPeer(a *BzzAddr, po uint8) {
// immediately return
if (po < d.getDepth() && pot.ProxCmp(d.kad.BaseAddr(), d, a) != 1) || d.seen(a) {
//if (po < d.getDepth() && pot.ProxCmp(d.kad.BaseAddr(), d, a) != 1) || d.seen(a) {
Copy link
Member

Choose a reason for hiding this comment

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

remove line?

@zelig zelig merged commit b246437 into ethersphere:master Aug 14, 2019
@nolash nolash deleted the xor-distance branch August 15, 2019 07:37
@skylenet skylenet added this to the 0.5.0 milestone Sep 17, 2019
chadsr added a commit to chadsr/swarm that referenced this pull request Sep 23, 2019
@chadsr
Merge branch 'master' of github.com:ethersphere/swarm
a914e5b 
* 'master' of github.com:ethersphere/swarm:
  chunk, storage: chunk.Store multiple chunk Put (ethersphere#1681)
  storage: fix pyramid chunker and hasherstore possible deadlocks (ethersphere#1679)
  pss: Use distance to determine single guaranteed recipient (ethersphere#1672)
  storage: fix possible hasherstore deadlock on waitC channel (ethersphere#1674)
  network: Add adaptive capabilities message (ethersphere#1619)
  p2p/protocols, p2p/testing; conditional propagation of context (ethersphere#1648)
  api/http: remove unnecessary conversion (ethersphere#1673)
  storage: fix LazyChunkReader.join potential deadlock (ethersphere#1670)
  HTTP API support for pinning contents (ethersphere#1658)
  pot: Add Distance methods with tests (ethersphere#1621)
  README.md: Update Vendored Dependencies section (ethersphere#1667)
  network, p2p, vendor: move vendored p2p/testing under swarm (ethersphere#1647)
  build, vendor: use go modules for vendoring (ethersphere#1532)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@acud acud acud approved these changes

@zelig zelig zelig approved these changes

@janos janos janos left review comments

@nonsense nonsense Awaiting requested review from nonsense

Assignees

@nolash nolash

Labels
enhancement pot
Projects
None yet
Milestone
0.5.0
Development

Successfully merging this pull request may close these issues.
5 participants
@nolash @acud @janos @zelig @skylenet