Clean up some YAGNI issues with implementation of GitHub issue #180, and make best path choice aware of path rank.

9 files changed, 114 insertions, 96 deletions

diff --git a/node/IncomingPacket.cpp b/node/IncomingPacket.cpp
index afb07b9d..ae99352e 100644
--- a/node/IncomingPacket.cpp
+++ b/node/IncomingPacket.cpp
@@ -909,7 +909,6 @@ bool IncomingPacket::_doPUSH_DIRECT_PATHS(const RuntimeEnvironment *RR,const Sha
 			// TODO: properly handle blacklisting, support other features... see Packet.hpp.
 
 			unsigned int flags = (*this)[ptr++];
-			/*int metric = (*this)[ptr++];*/ ++ptr;
 			unsigned int extLen = at<uint16_t>(ptr); ptr += 2;
 			ptr += extLen; // unused right now
 			unsigned int addrType = (*this)[ptr++];
diff --git a/node/InetAddress.hpp b/node/InetAddress.hpp
index 35fd4314..e3537ce0 100644
--- a/node/InetAddress.hpp
+++ b/node/InetAddress.hpp
@@ -63,17 +63,20 @@ struct InetAddress : public sockaddr_storage
 
 	/**
 	 * IP address scope
+	 *
+	 * Note that these values are in ascending order of path preference and
+	 * MUST remain that way or Path must be changed to reflect.
 	 */
 	enum IpScope
 	{
-		IP_SCOPE_NONE = 0,          // not an IP address -- also the number of classes, must be last entry
-		IP_SCOPE_LINK_LOCAL = 1,    // 169.254.x.x, IPv6 LL
-		IP_SCOPE_PRIVATE = 2,       // 10.x.x.x, etc.
-		IP_SCOPE_PSEUDOPRIVATE = 3, // 28.x.x.x, etc. -- unofficially unrouted IP blocks often "bogarted"
-		IP_SCOPE_SHARED = 4,        // 100.64.0.0/10, shared space for e.g. carrier-grade NAT
-		IP_SCOPE_GLOBAL = 5,        // globally routable IP address (all others)
-		IP_SCOPE_LOOPBACK = 6,      // 127.0.0.1
-		IP_SCOPE_MULTICAST = 7      // 224.0.0.0 and other multicast IPs
+		IP_SCOPE_NONE = 0,          // NULL or not an IP address
+		IP_SCOPE_MULTICAST = 1,     // 224.0.0.0 and other V4/V6 multicast IPs
+		IP_SCOPE_LOOPBACK = 2,      // 127.0.0.1, ::1, etc.
+		IP_SCOPE_PSEUDOPRIVATE = 3, // 28.x.x.x, etc. -- unofficially unrouted IPv4 blocks often "bogarted"
+		IP_SCOPE_GLOBAL = 4,        // globally routable IP address (all others)
+		IP_SCOPE_LINK_LOCAL = 5,    // 169.254.x.x, IPv6 LL
+		IP_SCOPE_SHARED = 6,        // 100.64.0.0/10, shared space for e.g. carrier-grade NAT
+		IP_SCOPE_PRIVATE = 7        // 10.x.x.x, 192.168.x.x, etc.
 	};
 
 	InetAddress() throw() { memset(this,0,sizeof(InetAddress)); }
diff --git a/node/Node.cpp b/node/Node.cpp
index 3df34aec..ebe0527e 100644
--- a/node/Node.cpp
+++ b/node/Node.cpp
@@ -432,11 +432,11 @@ void Node::freeQueryResult(void *qr)
 		::free(qr);
 }
 
-int Node::addLocalInterfaceAddress(const struct sockaddr_storage *addr,int metric,ZT1_LocalInterfaceAddressTrust trust,int reliable)
+int Node::addLocalInterfaceAddress(const struct sockaddr_storage *addr,int metric,ZT1_LocalInterfaceAddressTrust trust)
 {
 	if (Path::isAddressValidForPath(*(reinterpret_cast<const InetAddress *>(addr)))) {
 		Mutex::Lock _l(_directPaths_m);
-		_directPaths.push_back(Path(*(reinterpret_cast<const InetAddress *>(addr)),metric,(Path::Trust)trust,reliable != 0));
+		_directPaths.push_back(Path(*(reinterpret_cast<const InetAddress *>(addr)),metric,(Path::Trust)trust));
 		std::sort(_directPaths.begin(),_directPaths.end());
 		_directPaths.erase(std::unique(_directPaths.begin(),_directPaths.end()),_directPaths.end());
 		return 1;
@@ -711,10 +711,10 @@ void ZT1_Node_setNetconfMaster(ZT1_Node *node,void *networkControllerInstance)
 	} catch ( ... ) {}
 }
 
-int ZT1_Node_addLocalInterfaceAddress(ZT1_Node *node,const struct sockaddr_storage *addr,int metric,ZT1_LocalInterfaceAddressTrust trust,int reliable)
+int ZT1_Node_addLocalInterfaceAddress(ZT1_Node *node,const struct sockaddr_storage *addr,int metric,ZT1_LocalInterfaceAddressTrust trust)
 {
 	try {
-		return reinterpret_cast<ZeroTier::Node *>(node)->addLocalInterfaceAddress(addr,metric,trust,reliable);
+		return reinterpret_cast<ZeroTier::Node *>(node)->addLocalInterfaceAddress(addr,metric,trust);
 	} catch ( ... ) {
 		return 0;
 	}
diff --git a/node/Node.hpp b/node/Node.hpp
index 579d3a57..0e966aa6 100644
--- a/node/Node.hpp
+++ b/node/Node.hpp
@@ -105,7 +105,7 @@ public:
 	ZT1_VirtualNetworkConfig *networkConfig(uint64_t nwid) const;
 	ZT1_VirtualNetworkList *networks() const;
 	void freeQueryResult(void *qr);
-	int addLocalInterfaceAddress(const struct sockaddr_storage *addr,int metric,ZT1_LocalInterfaceAddressTrust trust,int reliable);
+	int addLocalInterfaceAddress(const struct sockaddr_storage *addr,int metric,ZT1_LocalInterfaceAddressTrust trust);
 	void clearLocalInterfaceAddresses();
 	void setNetconfMaster(void *networkControllerInstance);
 
diff --git a/node/Packet.hpp b/node/Packet.hpp
index e84306c2..fa377964 100644
--- a/node/Packet.hpp
+++ b/node/Packet.hpp
@@ -70,7 +70,7 @@
 
 /**
  * Maximum hop count allowed by packet structure (3 bits, 0-7)
- * 
+ *
  * This is a protocol constant. It's the maximum allowed by the length
  * of the hop counter -- three bits. See node/Constants.hpp for the
  * pragmatic forwarding limit, which is typically lower.
@@ -352,7 +352,7 @@ namespace ZeroTier {
 
 /**
  * ZeroTier packet
- * 
+ *
  * Packet format:
  *   <[8] random initialization vector (doubles as 64-bit packet ID)>
  *   <[5] destination ZT address>
@@ -362,7 +362,7 @@ namespace ZeroTier {
  *   [... -- begin encryption envelope -- ...]
  *   <[1] encrypted flags (top 3 bits) and verb (last 5 bits)>
  *   [... verb-specific payload ...]
- * 
+ *
  * Packets smaller than 28 bytes are invalid and silently discarded.
  *
  * The flags/cipher/hops bit field is: FFCCCHHH where C is a 3-bit cipher
@@ -384,15 +384,15 @@ class Packet : public Buffer<ZT_PROTO_MAX_PACKET_LENGTH>
 public:
 	/**
 	 * A packet fragment
-	 * 
+	 *
 	 * Fragments are sent if a packet is larger than UDP MTU. The first fragment
 	 * is sent with its normal header with the fragmented flag set. Remaining
 	 * fragments are sent this way.
-	 * 
+	 *
 	 * The fragmented bit indicates that there is at least one fragment. Fragments
 	 * themselves contain the total, so the receiver must "learn" this from the
 	 * first fragment it receives.
-	 * 
+	 *
 	 * Fragments are sent with the following format:
 	 *   <[8] packet ID of packet whose fragment this belongs to>
 	 *   <[5] destination ZT address>
@@ -430,7 +430,7 @@ public:
 
 		/**
 		 * Initialize from a packet
-		 * 
+		 *
 		 * @param p Original assembled packet
 		 * @param fragStart Start of fragment (raw index in packet data)
 		 * @param fragLen Length of fragment in bytes
@@ -446,7 +446,7 @@ public:
 
 		/**
 		 * Initialize from a packet
-		 * 
+		 *
 		 * @param p Original assembled packet
 		 * @param fragStart Start of fragment (raw index in packet data)
 		 * @param fragLen Length of fragment in bytes
@@ -473,7 +473,7 @@ public:
 
 		/**
 		 * Get this fragment's destination
-		 * 
+		 *
 		 * @return Destination ZT address
 		 */
 		inline Address destination() const { return Address(field(ZT_PACKET_FRAGMENT_IDX_DEST,ZT_ADDRESS_LENGTH),ZT_ADDRESS_LENGTH); }
@@ -872,7 +872,6 @@ public:
 		 *
 		 * Path record format:
 		 *   <[1] flags>
-		 *   <[1] metric from 0 (highest priority) to 255 (lowest priority)>
 		 *   <[2] length of extended path characteristics or 0 for none>
 		 *   <[...] extended path characteristics>
 		 *   <[1] address type>
@@ -882,9 +881,8 @@ public:
 		 * Path record flags:
 		 *   0x01 - Forget this path if it is currently known
 		 *   0x02 - Blacklist this path, do not use
-		 *   0x04 - Reliable path (no NAT keepalives, etc. are necessary)
-		 *   0x08 - Disable encryption (trust: privacy)
-		 *   0x10 - Disable encryption and authentication (trust: ultimate)
+		 *   0x04 - Disable encryption (trust: privacy)
+		 *   0x08 - Disable encryption and authentication (trust: ultimate)
 		 *
 		 * Address types and addresses are of the same format as the destination
 		 * address type and address in HELLO.
@@ -901,15 +899,10 @@ public:
 		 * is set.
 		 *
 		 * Only a subset of this functionality is currently implemented: basic
-		 * path pushing and learning. Metrics, most flags, and OK responses are
-		 * not yet implemented as of 1.0.4.
-		 *
-		 * OK response payload:
-		 *   <[2] 16-bit number of active direct paths we already have>
-		 *   <[2] 16-bit number of paths in push that we don't already have>
-		 *   <[2] 16-bit number of new paths we are trying (or will try)>
+		 * path pushing and learning. Blacklisting and trust are not fully
+		 * implemented yet (encryption is still always used).
 		 *
-		 * ERROR is presently not sent.
+		 * OK and ERROR are not generated.
 		 */
 		VERB_PUSH_DIRECT_PATHS = 16
 	};
@@ -974,7 +967,7 @@ public:
 
 	/**
 	 * Construct a new empty packet with a unique random packet ID
-	 * 
+	 *
 	 * Flags and hops will be zero. Other fields and data region are undefined.
 	 * Use the header access methods (setDestination() and friends) to fill out
 	 * the header. Payload should be appended; initial size is header size.
@@ -1004,7 +997,7 @@ public:
 
 	/**
 	 * Construct a new empty packet with a unique random packet ID
-	 * 
+	 *
 	 * @param dest Destination ZT address
 	 * @param source Source ZT address
 	 * @param v Verb
@@ -1021,7 +1014,7 @@ public:
 
 	/**
 	 * Reset this packet structure for reuse in place
-	 * 
+	 *
 	 * @param dest Destination ZT address
 	 * @param source Source ZT address
 	 * @param v Verb
@@ -1047,28 +1040,28 @@ public:
 
 	/**
 	 * Set this packet's destination
-	 * 
+	 *
 	 * @param dest ZeroTier address of destination
 	 */
 	inline void setDestination(const Address &dest) { dest.copyTo(field(ZT_PACKET_IDX_DEST,ZT_ADDRESS_LENGTH),ZT_ADDRESS_LENGTH); }
 
 	/**
 	 * Set this packet's source
-	 * 
+	 *
 	 * @param source ZeroTier address of source
 	 */
 	inline void setSource(const Address &source) { source.copyTo(field(ZT_PACKET_IDX_SOURCE,ZT_ADDRESS_LENGTH),ZT_ADDRESS_LENGTH); }
 
 	/**
 	 * Get this packet's destination
-	 * 
+	 *
 	 * @return Destination ZT address
 	 */
 	inline Address destination() const { return Address(field(ZT_PACKET_IDX_DEST,ZT_ADDRESS_LENGTH),ZT_ADDRESS_LENGTH); }
 
 	/**
 	 * Get this packet's source
-	 * 
+	 *
 	 * @return Source ZT address
 	 */
 	inline Address source() const { return Address(field(ZT_PACKET_IDX_SOURCE,ZT_ADDRESS_LENGTH),ZT_ADDRESS_LENGTH); }
@@ -1138,17 +1131,17 @@ public:
 
 	/**
 	 * Get this packet's unique ID (the IV field interpreted as uint64_t)
-	 * 
+	 *
 	 * @return Packet ID
 	 */
 	inline uint64_t packetId() const { return at<uint64_t>(ZT_PACKET_IDX_IV); }
 
 	/**
 	 * Set packet verb
-	 * 
+	 *
 	 * This also has the side-effect of clearing any verb flags, such as
 	 * compressed, and so must only be done during packet composition.
-	 * 
+	 *
 	 * @param v New packet verb
 	 */
 	inline void setVerb(Verb v) { (*this)[ZT_PACKET_IDX_VERB] = (char)v; }
@@ -1186,22 +1179,22 @@ public:
 
 	/**
 	 * Attempt to compress payload if not already (must be unencrypted)
-	 * 
+	 *
 	 * This requires that the payload at least contain the verb byte already
 	 * set. The compressed flag in the verb is set if compression successfully
 	 * results in a size reduction. If no size reduction occurs, compression
 	 * is not done and the flag is left cleared.
-	 * 
+	 *
 	 * @return True if compression occurred
 	 */
 	bool compress();
 
 	/**
 	 * Attempt to decompress payload if it is compressed (must be unencrypted)
-	 * 
+	 *
 	 * If payload is compressed, it is decompressed and the compressed verb
 	 * flag is cleared. Otherwise nothing is done and true is returned.
-	 * 
+	 *
 	 * @return True if data is now decompressed and valid, false on error
 	 */
 	bool uncompress();
diff --git a/node/Path.hpp b/node/Path.hpp
index cd21444b..0e53772d 100644
--- a/node/Path.hpp
+++ b/node/Path.hpp
@@ -34,10 +34,31 @@
 
 namespace ZeroTier {
 
+/**
+ * Base class for paths
+ *
+ * The base Path class is an immutable value.
+ */
 class Path
 {
 public:
-	// Must be the same values as ZT1_LocalInterfaceAddressTrust in ZeroTierOne.h
+	/**
+	 * Path trust category
+	 *
+	 * Note that this is NOT peer trust and has nothing to do with root server
+	 * designations or other trust metrics. This indicates how much we trust
+	 * this path to be secure and/or private. A trust level of normal means
+	 * encrypt and authenticate all traffic. Privacy trust means we can send
+	 * traffic in the clear. Ultimate trust means we don't even need
+	 * authentication. Generally a private path would be a hard-wired local
+	 * LAN, while an ultimate trust path would be a physically isolated private
+	 * server backplane.
+	 *
+	 * Nearly all paths will be normal trust. The other levels are for high
+	 * performance local SDN use only.
+	 *
+	 * These values MUST match ZT1_LocalInterfaceAddressTrust in ZeroTierOne.h
+	 */
 	enum Trust
 	{
 		TRUST_NORMAL = 0,
@@ -47,17 +68,15 @@ public:
 
 	Path() :
 		_addr(),
-		_metric(0),
-		_trust(TRUST_NORMAL),
-		_reliable(false)
+		_ipScope(InetAddress::IP_SCOPE_NONE),
+		_trust(TRUST_NORMAL)
 	{
 	}
 
-	Path(const InetAddress &addr,int metric,Trust trust,bool reliable) :
+	Path(const InetAddress &addr,int metric,Trust trust) :
 		_addr(addr),
-		_metric(metric),
-		_trust(trust),
-		_reliable(reliable)
+		_ipScope(addr.ipScope()),
+		_trust(trust)
 	{
 	}
 
@@ -67,9 +86,14 @@ public:
 	inline const InetAddress &address() const throw() { return _addr; }
 
 	/**
-	 * @return Metric (higher == worse) or negative if path is blacklisted
+	 * @return IP scope -- faster shortcut for address().ipScope()
+	 */
+	inline InetAddress::IpScope ipScope() const throw() { return _ipScope; }
+
+	/**
+	 * @return Preference rank, higher == better
 	 */
-	inline int metric() const throw() { return _metric; }
+	inline int preferenceRank() const throw() { return (int)_ipScope; } // IP scopes are in ascending rank order in InetAddress.hpp
 
 	/**
 	 * @return Path trust level
@@ -79,7 +103,10 @@ public:
 	/**
 	 * @return True if path is considered reliable (no NAT keepalives etc. are needed)
 	 */
-	inline bool reliable() const throw() { return _reliable; }
+	inline bool reliable() const throw()
+	{
+		return ((_ipScope != InetAddress::IP_SCOPE_GLOBAL)&&(_ipScope != InetAddress::IP_SCOPE_PSEUDOPRIVATE));
+	}
 
 	/**
 	 * @return True if address is non-NULL
@@ -127,11 +154,10 @@ public:
 		return false;
 	}
 
-protected:
+private:
 	InetAddress _addr;
-	int _metric; // negative == blacklisted
+	InetAddress::IpScope _ipScope; // memoize this since it's a computed value checked often
 	Trust _trust;
-	bool _reliable;
 };
 
 } // namespace ZeroTier
diff --git a/node/Peer.cpp b/node/Peer.cpp
index ba765c72..23616a6a 100644
--- a/node/Peer.cpp
+++ b/node/Peer.cpp
@@ -161,6 +161,21 @@ void Peer::received(
 		_lastMulticastFrame = now;
 }
 
+RemotePath *Peer::getBestPath(uint64_t now)
+{
+	RemotePath *bestPath = (RemotePath *)0;
+	uint64_t lrMax = 0;
+	int rank = 0;
+	for(unsigned int p=0,np=_numPaths;p<np;++p) {
+		if ( (_paths[p].active(now)) && ((_paths[p].lastReceived() >= lrMax)||(_paths[p].preferenceRank() >= rank)) ) {
+			lrMax = _paths[p].lastReceived();
+			rank = _paths[p].preferenceRank();
+			bestPath = &(_paths[p]);
+		}
+	}
+	return bestPath;
+}
+
 void Peer::attemptToContactAt(const RuntimeEnvironment *RR,const InetAddress &atAddress,uint64_t now)
 {
 	Packet outp(_id.address(),RR->identity.address(),Packet::VERB_HELLO);
@@ -200,7 +215,7 @@ void Peer::doPingAndKeepalive(const RuntimeEnvironment *RR,uint64_t now)
 			TRACE("PING %s(%s)",_id.address().toString().c_str(),bestPath->address().toString().c_str());
 			attemptToContactAt(RR,bestPath->address(),now);
 			bestPath->sent(now);
-		} else if ((now - bestPath->lastSend()) >= ZT_NAT_KEEPALIVE_DELAY) {
+		} else if (((now - bestPath->lastSend()) >= ZT_NAT_KEEPALIVE_DELAY)&&(!bestPath->reliable())) {
 			TRACE("NAT keepalive %s(%s)",_id.address().toString().c_str(),bestPath->address().toString().c_str());
 			RR->node->putPacket(bestPath->address(),"",0);
 			bestPath->sent(now);
@@ -230,31 +245,24 @@ void Peer::pushDirectPaths(const RuntimeEnvironment *RR,RemotePath *path,uint64_
 					case AF_INET6:
 						addressType = 6;
 						break;
-					default:
+					default: // we currently only push IP addresses
 						++p;
 						continue;
 				}
 
 				uint8_t flags = 0;
-				if (p->metric() < 0)
-					flags |= (0x01 | 0x02); // forget and blacklist
-				else {
-					if (p->reliable())
-						flags |= 0x04; // no NAT keepalives and such
-					switch(p->trust()) {
-						default:
-							break;
-						case Path::TRUST_PRIVACY:
-							flags |= 0x08; // no encryption
-							break;
-						case Path::TRUST_ULTIMATE:
-							flags |= (0x08 | 0x10); // no encryption, no authentication (redundant but go ahead and set both)
-							break;
-					}
+				switch(p->trust()) {
+					default:
+						break;
+					case Path::TRUST_PRIVACY:
+						flags |= 0x04; // no encryption
+						break;
+					case Path::TRUST_ULTIMATE:
+						flags |= (0x04 | 0x08); // no encryption, no authentication (redundant but go ahead and set both)
+						break;
 				}
 
 				outp.append(flags);
-				outp.append((uint8_t)((p->metric() >= 0) ? ((p->metric() <= 255) ? p->metric() : 255) : 0));
 				outp.append((uint16_t)0); // no extensions
 				outp.append(addressType);
 				outp.append((uint8_t)((addressType == 4) ? 6 : 18));
diff --git a/node/Peer.hpp b/node/Peer.hpp
index f5118794..283e3f33 100644
--- a/node/Peer.hpp
+++ b/node/Peer.hpp
@@ -102,7 +102,7 @@ public:
 	 *
 	 * This is called by the decode pipe when a packet is proven to be authentic
 	 * and appears to be valid.
-	 * 
+	 *
 	 * @param RR Runtime environment
 	 * @param remoteAddr Internet address of sender
 	 * @param hops ZeroTier (not IP) hops
@@ -126,18 +126,7 @@ public:
 	 * @param now Current time
 	 * @return Best path or NULL if there are no active (or fixed) direct paths
 	 */
-	inline RemotePath *getBestPath(uint64_t now)
-	{
-		RemotePath *bestPath = (RemotePath *)0;
-		uint64_t lrMax = 0;
-		for(unsigned int p=0,np=_numPaths;p<np;++p) {
-			if ((_paths[p].active(now))&&(_paths[p].lastReceived() >= lrMax)) {
-				lrMax = _paths[p].lastReceived();
-				bestPath = &(_paths[p]);
-			}
-		}
-		return bestPath;
-	}
+	RemotePath *getBestPath(uint64_t now);
 
 	/**
 	 * Send via best path
diff --git a/node/RemotePath.hpp b/node/RemotePath.hpp
index 5592c8e1..291943c9 100644
--- a/node/RemotePath.hpp
+++ b/node/RemotePath.hpp
@@ -56,7 +56,7 @@ public:
 		_fixed(false) {}
 
 	RemotePath(const InetAddress &addr,bool fixed) :
-		Path(addr,0,TRUST_NORMAL,false),
+		Path(addr,0,TRUST_NORMAL),
 		_lastSend(0),
 		_lastReceived(0),
 		_fixed(fixed) {}
@@ -123,7 +123,7 @@ public:
 	 */
 	inline bool send(const RuntimeEnvironment *RR,const void *data,unsigned int len,uint64_t now)
 	{
-		if (RR->node->putPacket(_addr,data,len)) {
+		if (RR->node->putPacket(address(),data,len)) {
 			sent(now);
 			RR->antiRec->logOutgoingZT(data,len);
 			return true;