Improved reliability of the determination of external address: removed DHT stunner from the pool, added mPeerMgr which vote is based on ext address most often reported by peer discovery

libretroshare/src/pqi/p3netmgr.cc

@@ -718,14 +718,16 @@ void p3NetMgrIMPL::netExtCheck()
 					{
 						// must be stable???
 						isStable = true;
-                        mNetFlags.mExtAddr = tmpip;
+						//mNetFlags.mExtAddr = tmpip;
 						mNetFlags.mExtAddrOk = true;
 						mNetFlags.mExtAddrStableOk = isStable;
 
 						address_votes[tmpip].n++ ;
+                        
+						std::cerr << "NetAssistAddress reported external address " << sockaddr_storage_iptostring(tmpip) << std::endl;
 					}
 					else
-                        std::cerr << "(SS) netAssisExternalAddress returned wrong own IP " << sockaddr_storage_iptostring(tmpip) << " (banned). Rejecting." << std::endl;
+						std::cerr << "(SS) netAssisExternalAddress returned banned own IP " << sockaddr_storage_iptostring(tmpip) << " (banned). Rejecting." << std::endl;
 				}
 #if defined(NETMGR_DEBUG_TICK) || defined(NETMGR_DEBUG_RESET)
 				else
@@ -737,6 +739,10 @@ void p3NetMgrIMPL::netExtCheck()
 
 		}
 
+#ifdef ALLOW_DHT_STUNNER
+        // (cyril) I disabled this because it's pretty dangerous. The DHT can report a wrong address quite easily
+        // if the other DHT peers are not collaborating.
+        
 		/* Next ask the DhtStunner */
 		{
 #if defined(NETMGR_DEBUG_TICK) || defined(NETMGR_DEBUG_RESET)
@@ -755,7 +761,7 @@ void p3NetMgrIMPL::netExtCheck()
 					{
 						// must be stable???
 						isStable = (isstable == 1);
-                        mNetFlags.mExtAddr = tmpaddr;
+						//mNetFlags.mExtAddr = tmpaddr;
 						mNetFlags.mExtAddrOk = true;
 						mNetFlags.mExtAddrStableOk = isStable;
 
@@ -772,6 +778,7 @@ void p3NetMgrIMPL::netExtCheck()
 				}
 			}
 		}
+#endif
 
 		/* otherwise ask ExtAddrFinder */
 		{
@@ -796,7 +803,7 @@ void p3NetMgrIMPL::netExtCheck()
 					std::cerr << std::endl;
 #endif
 
-					mNetFlags.mExtAddr = tmpip;
+					//mNetFlags.mExtAddr = tmpip;
 					mNetFlags.mExtAddrOk = true;
 
 					address_votes[tmpip].n++ ;
@@ -805,10 +812,50 @@ void p3NetMgrIMPL::netExtCheck()
 #warning "ALLOWING ExtAddrFinder -> ExtAddrStableOk = true (which it is not normally)"
 					mNetFlags.mExtAddrStableOk = true;
                     
+		    std::cerr << "ExtAddrFinder reported external address " << sockaddr_storage_iptostring(tmpip) << std::endl;
+
 				}
 			}
 		}
         
+        	/* also ask peer mgr. */
+		
+		if (mPeerMgr)
+		{
+#if defined(NETMGR_DEBUG_TICK) || defined(NETMGR_DEBUG_RESET)
+			std::cerr << "p3NetMgrIMPL::netExtCheck() checking mPeerMgr" << std::endl;
+#endif
+            		uint8_t isstable ;	// unused
+                    	sockaddr_storage tmpaddr ;
+
+			if (mPeerMgr->getExtAddressReportedByFriends(tmpaddr, isstable))
+			{
+#if defined(NETMGR_DEBUG_TICK) || defined(NETMGR_DEBUG_RESET)
+				std::cerr << "p3NetMgrIMPL::netExtCheck() Ext supplied by ExtAddrFinder" << std::endl;
+#endif
+				/* best guess at port */
+				sockaddr_storage_setport(tmpaddr, sockaddr_storage_port(mLocalAddr));
+
+#if defined(NETMGR_DEBUG_TICK) || defined(NETMGR_DEBUG_RESET)
+				std::cerr << "p3NetMgrIMPL::netExtCheck() ";
+				std::cerr << "ExtAddr: " << sockaddr_storage_tostring(tmpip);
+				std::cerr << std::endl;
+#endif
+
+				//mNetFlags.mExtAddr = tmpaddr;
+				mNetFlags.mExtAddrOk = true;
+				mNetFlags.mExtAddrStableOk = isstable;
+
+				address_votes[tmpaddr].n++ ;
+                
+		    std::cerr << "PeerMgr reported external address " << sockaddr_storage_iptostring(tmpaddr) << std::endl;
+			}
+#if defined(NETMGR_DEBUG_TICK) || defined(NETMGR_DEBUG_RESET)
+	    else
+                std::cerr << "  No reliable address returned." << std::endl;
+#endif
+		}
+        
 		/* any other sources ??? */
 
 		/* finalise address */

libretroshare/src/pqi/p3peermgr.cc

@@ -1247,6 +1247,7 @@ bool p3PeerMgrIMPL::addCandidateForOwnExternalAddress(const RsPeerId &from, cons
     // 		* if multiple peers report the same address => notify the LinkMgr that the external address had changed.
 
 	sockaddr_storage addr_filtered ;
+	sockaddr_storage_clear(addr_filtered) ;
 	sockaddr_storage_copyip(addr_filtered,addr) ;
 
 #ifdef PEER_DEBUG
@@ -1261,6 +1262,36 @@ bool p3PeerMgrIMPL::addCandidateForOwnExternalAddress(const RsPeerId &from, cons
 		return false ;
 	}
 
+    // Update a list of own IPs:
+    //	- remove old values for that same peer
+    //	- remove values for non connected peers
+    
+    {
+	    RsStackMutex stack(mPeerMtx); /****** STACK LOCK MUTEX *******/
+            
+	    mReportedOwnAddresses[from] = addr_filtered ;
+
+	    for(std::map<RsPeerId,sockaddr_storage>::iterator it(mReportedOwnAddresses.begin());it!=mReportedOwnAddresses.end();)
+		    if(!mLinkMgr->isOnline(it->first))
+		    {
+			    std::map<RsPeerId,sockaddr_storage>::iterator tmp(it) ;
+			    ++tmp ;
+			    mReportedOwnAddresses.erase(it) ;
+			    it=tmp ;
+		    }
+	    else
+		    ++it ;
+
+	    sockaddr_storage current_best_ext_address_guess ;
+	    uint32_t count ;
+
+	    locked_computeCurrentBestOwnExtAddressCandidate(current_best_ext_address_guess,count) ;
+
+	    std::cerr << "p3PeerMgr::  Current external address is calculated to be: " << sockaddr_storage_iptostring(current_best_ext_address_guess) << " (simultaneously reported by " << count << " peers)." << std::endl;
+    }
+       
+    // now current 
+
     sockaddr_storage own_addr ;
 
     if(!mNetMgr->getExtAddress(own_addr))
@@ -1283,9 +1314,55 @@ bool p3PeerMgrIMPL::addCandidateForOwnExternalAddress(const RsPeerId &from, cons
         RsServer::notify()->AddFeedItem(RS_FEED_ITEM_SEC_IP_WRONG_EXTERNAL_IP_REPORTED, from.toStdString(), sockaddr_storage_iptostring(own_addr), sockaddr_storage_iptostring(addr));
     }
     
+    // we could also sweep over all connected friends and see if some report a different address.
+
     return true ;
 }
 
+bool p3PeerMgrIMPL::locked_computeCurrentBestOwnExtAddressCandidate(sockaddr_storage& addr, uint32_t& count)
+{
+    std::map<sockaddr_storage,ZeroedInt> addr_counts ;
+    
+    for(std::map<RsPeerId,sockaddr_storage>::iterator it(mReportedOwnAddresses.begin());it!=mReportedOwnAddresses.end();++it)
+	    ++addr_counts[it->second].n ;
+
+#ifdef PEER_DEBUG
+    std::cerr << "Current ext addr statistics:" << std::endl;
+#endif
+    
+    count = 0 ;
+    
+    for(std::map<sockaddr_storage,ZeroedInt>::const_iterator it(addr_counts.begin());it!=addr_counts.end();++it)
+    {
+        if(it->second.n > count)
+        {
+            addr = it->first ;
+            count = it->second.n ;
+        }
+        
+#ifdef PEER_DEBUG
+        std::cerr << sockaddr_storage_iptostring(it->first) << " : " << it->second.n << std::endl;
+#endif
+    }
+    
+    return true ;
+}
+ 
+bool p3PeerMgrIMPL::getExtAddressReportedByFriends(sockaddr_storage &addr, uint8_t& isstable)
+{
+        RsStackMutex stack(mPeerMtx); /****** STACK LOCK MUTEX *******/
+        
+        uint32_t count ;
+        
+        locked_computeCurrentBestOwnExtAddressCandidate(addr,count) ;
+        
+#ifdef PEER_DEBUG
+        std::cerr << "Estimation count = " << count << ". Trusted? = " << (count>=2) << std::endl;
+#endif
+        
+        return count >= 2 ;// 2 is not conservative enough. 3 should be probably better.	
+}
+
 static bool cleanIpList(std::list<pqiIpAddress>& lst,const RsPeerId& pid,p3LinkMgr *link_mgr)
 {
     bool changed = false ;

libretroshare/src/pqi/p3peermgr.h

@@ -153,6 +153,7 @@ virtual bool 	setLocalAddress(const RsPeerId &id, const struct sockaddr_storage
 virtual bool 	setExtAddress(const RsPeerId &id, const struct sockaddr_storage &addr) = 0;
 virtual bool    setDynDNS(const RsPeerId &id, const std::string &dyndns) = 0;
 virtual bool 	addCandidateForOwnExternalAddress(const RsPeerId& from, const struct sockaddr_storage &addr) = 0;
+virtual bool 	getExtAddressReportedByFriends(struct sockaddr_storage& addr,uint8_t& isstable) = 0;
 
 virtual bool 	setNetworkMode(const RsPeerId &id, uint32_t netMode) = 0;
 virtual bool 	setVisState(const RsPeerId &id, uint16_t vs_disc, uint16_t vs_dht) = 0;
@@ -200,6 +201,7 @@ virtual int 	getFriendCount(bool ssl, bool online) = 0;
 		// Single Use Function... shouldn't be here. used by p3serverconfig.cc
 virtual bool 	haveOnceConnected() = 0;
 
+virtual bool   locked_computeCurrentBestOwnExtAddressCandidate(sockaddr_storage &addr, uint32_t &count)=0;
 
 /*************************************************************************************************/
 /*************************************************************************************************/
@@ -256,6 +258,7 @@ virtual bool 	setLocalAddress(const RsPeerId &id, const struct sockaddr_storage
 virtual bool 	setExtAddress(const RsPeerId &id, const struct sockaddr_storage &addr);
 virtual bool    setDynDNS(const RsPeerId &id, const std::string &dyndns);
 virtual bool 	addCandidateForOwnExternalAddress(const RsPeerId& from, const struct sockaddr_storage &addr) ;
+virtual bool 	getExtAddressReportedByFriends(struct sockaddr_storage& addr, uint8_t &isstable) ;
 
 virtual bool 	setNetworkMode(const RsPeerId &id, uint32_t netMode);
 virtual bool 	setVisState(const RsPeerId &id, uint16_t vs_disc, uint16_t vs_dht);
@@ -327,6 +330,7 @@ int 	getConnectAddresses(const RsPeerId &id,
 				struct sockaddr_storage &lAddr, struct sockaddr_storage &eAddr, 
 				pqiIpAddrSet &histAddrs, std::string &dyndns);
 
+
 protected:
 	/* Internal Functions */
 
@@ -335,6 +339,8 @@ bool 	removeBannedIps();
 
 void    printPeerLists(std::ostream &out);
 
+virtual bool   locked_computeCurrentBestOwnExtAddressCandidate(sockaddr_storage &addr, uint32_t &count);
+
 	protected:
 /*****************************************************************/
 /***********************  p3config  ******************************/
@@ -362,6 +368,8 @@ private:
 	std::map<RsPeerId, peerState> mFriendList;	// <SSLid , peerState>
 	std::map<RsPeerId, peerState> mOthersList;
 
+    	std::map<RsPeerId,sockaddr_storage> mReportedOwnAddresses ;
+        
 	std::list<RsPeerGroupItem *> groupList;
 	uint32_t lastGroupId;