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

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

Closed
holisticode wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fd364b1
swarm/network/stream: use bzzAddr for proximity calculation
holisticode Apr 27, 2019
f35f644
swarm/network/stream: use bzzPeer instead of protocols.Peer for strea…
holisticode Apr 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions swarm/network/stream/peer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ var ErrMaxPeerServers = errors.New("max peer servers")
// Peer is the Peer extension for the streaming protocol
type Peer struct {
*protocols.Peer
bzzPeer *network.BzzPeer

@nonsense nonsense Apr 27, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The bzzPeer has access to the protocols.Peer, so maybe we don't need the embedding on line 60 ?

@holisticode holisticode Apr 27, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I applied a change to address this which resulted in quite a lot of cascading changes...I hope this does not introduce some new level of "ugliness"?

@nonsense nonsense Apr 28, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@holisticode all the peer structs are confusing to me. I suggested the change in case it didn't result in many cascading changes. If this is not the case, feel free to just address this particular problem, and once this is correct, we can review and discuss how to refactor :)

@nonsense nonsense Apr 28, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@holisticode I am sorry if you have understood my comment as a request to do that change - it was not intended as such.

I think the new cascading change is much worse to what we have right now, and will also result in many conflicts with other branches we have in progress right now.

Furthermore this cascading change is not addressing the underlying problem - which is the mess of all the different peer structs and their inheritance.

I suggest we just fix the proximity calculations if possible, and not do such cascading change throughout the codebase, unless we actually address the underlying problem of the design.

streamer *Registry
pq *pq.PriorityQueue
serverMu sync.RWMutex
Expand All @@ -77,9 +78,10 @@ type WrappedPriorityMsg struct {
}

// NewPeer is the constructor for Peer
func NewPeer(peer *protocols.Peer, streamer *Registry) *Peer {
func NewPeer(peer *network.BzzPeer, streamer *Registry) *Peer {
p := &Peer{
Peer: peer,
Peer: peer.Peer,

@nonsense nonsense Apr 27, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Explicitly keeping a pointer to peer.Peer seems redundant.

@holisticode holisticode Apr 27, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

See above

bzzPeer: peer,
pq: pq.New(int(PriorityQueue), PriorityQueueCap),
streamer: streamer,
servers: make(map[Stream]*server),
Expand Down Expand Up @@ -440,7 +442,7 @@ func (p *Peer) runUpdateSyncing() {
}

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

depth := kad.NeighbourhoodDepth()

Expand Down
9 changes: 8 additions & 1 deletion swarm/network/stream/peer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ func TestUpdateSyncingSubscriptions(t *testing.T) {
r.Close()
clean()
}
bucket.Store("addr", addr)
return r, cleanup, nil
},
})
Expand All @@ -166,7 +167,13 @@ func TestUpdateSyncingSubscriptions(t *testing.T) {
// 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())
item, ok := sim.NodeItem(id, "addr")
if !ok {
t.Fatal("No addr")
}

bzzAddr := item.(*network.BzzAddr)
nodeProximities[id.String()] = chunk.Proximity(pivotKademlia.BaseAddr(), bzzAddr.Address())
}
// wait until sync subscriptions are done for all nodes
waitForSubscriptions(t, pivotRegistry, ids[1:]...)
Expand Down
2 changes: 1 addition & 1 deletion swarm/network/stream/stream.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,7 @@ func (r *Registry) peersCount() (c int) {

// Run protocol run function
func (r *Registry) Run(p *network.BzzPeer) error {
sp := NewPeer(p.Peer, r)
sp := NewPeer(p, r)
r.setPeer(sp)
defer r.deletePeer(sp)
defer close(sp.quit)
Expand Down