From c4a64c86495b74a0c69c1ae760e60d43a6aeaeae Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Sun, 30 Nov 2014 16:04:55 +0000 Subject: [PATCH 1/2] refactor and explain route calculation This was rather impenetrable and had some significant code duplication. So add some words of explanation, and refactor to remove the duplication. Some fly-by fixing of minor bug too: buildUnicastRoutes failed to exclude asymmetric connections from the first hop. --- router/peer.go | 57 +++++++++++++++++++++-------- router/peer_cache.go | 2 +- router/topology.go | 85 ++++++++++++++++---------------------------- 3 files changed, 74 insertions(+), 70 deletions(-) diff --git a/router/peer.go b/router/peer.go index e7facb1b..5739a8b8 100644 --- a/router/peer.go +++ b/router/peer.go @@ -65,42 +65,71 @@ func (peer *Peer) ForEachConnection(fun func(PeerName, Connection)) { } } -func (peer *Peer) HasPathTo(target *Peer, symmetric bool) (bool, map[PeerName]bool) { +// Calculate the routing table from this peer to all peers reachable +// from it, returning a "next hop" map of PeerNameX -> PeerNameY, +// which says "in order to send a message to X, the peer should send +// the message to its neighbour Y". +// +// Because currently we do not have weightings on the connections +// between peers, there is no need to use a minimum spanning tree +// algorithm. Instead we employ the simpler and cheaper breadth-first +// widening. The computation is deterministic, which ensures that when +// it is performed on the same data by different peers, they get the +// same result. This is important since otherwise we risk message loss +// or routing cycles. +// +// When the 'symmetric' flag is set, only symmetric connections are +// considered, i.e. where both sides indicate they have a connection +// to the other. +// +// When a non-nil stopAt peer is supplied, the widening stops when it +// reaches that peer. The boolean return indicates whether that has +// happened. +// +// We acquire read locks on peers as we encounter them during the +// traversal. This prevents the connectivity graph from changing +// underneath us in ways that would invalidate the result. Thus the +// answer returned may be out of date, but never inconsistent. +func (peer *Peer) Routes(stopAt *Peer, symmetric bool) (bool, map[PeerName]PeerName) { peer.RLock() defer peer.RUnlock() - reached := make(map[PeerName]bool) + routes := make(map[PeerName]PeerName) + routes[peer.Name] = UnknownPeerName nextWorklist := []*Peer{peer} - reached[peer.Name] = true for len(nextWorklist) > 0 { worklist := nextWorklist sort.Sort(ListOfPeers(worklist)) nextWorklist = []*Peer{} for _, curPeer := range worklist { - if curPeer == target { - return true, reached + if curPeer == stopAt { + return true, routes } + curName := curPeer.Name for remoteName, conn := range curPeer.connections { - if _, found := reached[remoteName]; found { + if _, found := routes[remoteName]; found { continue } - // By taking this lock, we ensure no changes can occur - // to any peer we have reached. As a result, if/when - // we reach the target, we know the entire path from - // peer to target is valid - no changes can have been - // made to that path during our calculation. remote := conn.Remote() remote.RLock() - if _, found := remote.connections[curPeer.Name]; !symmetric || found { + if _, found := remote.connections[curName]; !symmetric || found { defer remote.RUnlock() nextWorklist = append(nextWorklist, remote) - reached[remoteName] = true + // We now know how to get to remoteName: the same + // way we get to curPeer. Except, if curPeer the + // starting peer in which case we know we can + // reach remoteName directly. + if curPeer == peer { + routes[remoteName] = remoteName + } else { + routes[remoteName] = routes[curName] + } } else { remote.RUnlock() } } } } - return false, reached + return false, routes } func (peer *Peer) SetVersionAndConnections(version uint64, connections map[PeerName]Connection) { diff --git a/router/peer_cache.go b/router/peer_cache.go index c296d40b..321bc130 100644 --- a/router/peer_cache.go +++ b/router/peer_cache.go @@ -131,7 +131,7 @@ func (cache *PeerCache) fetchAlias(peer *Peer) (*Peer, bool) { func (cache *PeerCache) garbageCollect(ourself *Peer) []*Peer { removed := []*Peer{} for name, peer := range cache.table { - found, _ := ourself.HasPathTo(peer, false) + found, _ := ourself.Routes(peer, false) if !found && !peer.IsLocallyReferenced() { cache.onGC(peer) delete(cache.table, name) diff --git a/router/topology.go b/router/topology.go index 833481c1..b8578f6b 100644 --- a/router/topology.go +++ b/router/topology.go @@ -4,7 +4,6 @@ import ( "bytes" "fmt" "log" - "sort" ) func (topo *Topology) Unicast(name PeerName) (PeerName, bool) { @@ -97,69 +96,45 @@ func (topo *Topology) queryLoop(queryChan <-chan *Interaction) { } } +// Calculate all the routes for the question: if *we* want to send a +// packet to Peer X, what is the next hop? +// +// When we sniff a packet, we determine the destination peer +// ourself. Consequently, we can relay the packet via any +// arbitrary peers - the intermediate peers do not have to have +// any knowledge of the MAC address at all. Thus there's no need +// to exchange knowledge of MAC addresses, nor any constraints on +// the routes that we construct. func (topo *Topology) buildUnicastRoutes() map[PeerName]PeerName { - // He we are calculating all the routes for the question: if *we* - // want to send a packet to Peer X, what is the next hop? - // - // When we sniff a packet, we determine the destination peer - // ourself. Consequently, we can relay the packet via any - // arbitrary peers - the intermediate peers do not have to have - // any knowledge of the MAC address at all. Thus there's no need - // to exchange knowledge of MAC addresses, nor any constraints on - // the routes that we construct. - // - // Because currently we do not have weightings on the connections - // between peers, there is no need to use Dijkstra's - // algorithm. Instead, we can use the simpler and cheaper - // breadth-first widening. - // - // Currently we make no attempt to perform any balancing of any - // sort. That could happen in the future. - - unicast := make(map[PeerName]PeerName) - ourself := topo.router.Ourself - unicast[ourself.Name] = UnknownPeerName - nextWorklist := []*Peer{} - ourself.ForEachConnection(func(remoteName PeerName, conn Connection) { - unicast[remoteName] = remoteName - nextWorklist = append(nextWorklist, conn.Remote()) - }) - for len(nextWorklist) > 0 { - worklist := nextWorklist - sort.Sort(ListOfPeers(worklist)) - nextWorklist = []*Peer{} - for _, peer := range worklist { - localName := peer.Name - peer.ForEachConnection(func(remoteName PeerName, conn Connection) { - if _, found := unicast[remoteName]; found { - return - } - // We now know how to get to remoteName: the same - // way we get to peer. However, we only record this as - // a valid path if remoteName has peer in its - // connections. I.e. it must be a symmetric - // connection. - if _, found := conn.Remote().ConnectionTo(localName); found { - unicast[remoteName] = unicast[localName] - nextWorklist = append(nextWorklist, conn.Remote()) - } - }) - } - } - return unicast + _, routes := topo.router.Ourself.Routes(nil, true) + return routes } +// Calculate all the routes for the question: if we receive a +// broadcast originally from Peer X, which peers should we pass the +// frames on to? +// +// When the topology is stable, and thus all peers perform route +// calculations based on the same data, the algorithm ensures that +// broadcasts reach every peer exactly once. +// +// This is largely due to properties of the Peer.Routes algorithm. In +// particular: +// +// ForAll X,Y,Z in Peers. +// X.Routes(Y) <= X.Routes(Z) \/ +// X.Routes(Z) <= X.Routes(Y) +// ForAll X,Y,Z in Peers. +// Y =/= Z /\ X.Routes(Y) <= X.Routes(Z) => +// X.Routes(Y) u [P | Y.HasSymmetricConnectionTo(P)] <= X.Routes(Z) +// where <= is the subset relationship on keys of the returned map. func (topo *Topology) buildBroadcastRoutes() map[PeerName][]PeerName { - // For broadcast, we need to construct which of our connected - // peers we should pass frames on to should we receive any - // broadcast originally from peer X broadcast := make(map[PeerName][]PeerName) ourself := topo.router.Ourself topo.router.Peers.ForEach(func(name PeerName, peer *Peer) { hops := []PeerName{} - found, reached := peer.HasPathTo(ourself, true) - if found { + if found, reached := peer.Routes(ourself, true); found { ourself.ForEachConnection(func(remoteName PeerName, conn Connection) { if _, found := reached[remoteName]; found { return From 37eb2a4598dfa1a85fe7305f15f1316a59701141 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Mon, 1 Dec 2014 11:04:51 +0000 Subject: [PATCH 2/2] fix typo --- router/peer.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/router/peer.go b/router/peer.go index 5739a8b8..684a4eb2 100644 --- a/router/peer.go +++ b/router/peer.go @@ -115,8 +115,8 @@ func (peer *Peer) Routes(stopAt *Peer, symmetric bool) (bool, map[PeerName]PeerN defer remote.RUnlock() nextWorklist = append(nextWorklist, remote) // We now know how to get to remoteName: the same - // way we get to curPeer. Except, if curPeer the - // starting peer in which case we know we can + // way we get to curPeer. Except, if curPeer is + // the starting peer in which case we know we can // reach remoteName directly. if curPeer == peer { routes[remoteName] = remoteName