removed the reputation calls through rsIdentity. Improved the logic of updating static identity flags in p3gxsreputations

libretroshare/src/chat/distributedchat.cc

@@ -138,7 +138,7 @@ bool DistributedChatService::handleRecvChatLobbyMsgItem(RsChatMsgItem *ci)
         return false ;
     }
     
-    if(rsIdentity->overallReputationLevel(cli->signature.keyId) == RsReputations::REPUTATION_LOCALLY_NEGATIVE)
+    if(rsReputations->overallReputationLevel(cli->signature.keyId) == RsReputations::REPUTATION_LOCALLY_NEGATIVE)
     {
         std::cerr << "(WW) Received lobby msg/item from banned identity " << cli->signature.keyId << ". Dropping it." << std::endl;
         return false ;
@@ -648,7 +648,7 @@ void DistributedChatService::handleRecvChatLobbyEventItem(RsChatLobbyEventItem *
 #endif
 	time_t now = time(NULL) ;
 
-    if(rsIdentity->overallReputationLevel(item->signature.keyId) == RsReputations::REPUTATION_LOCALLY_NEGATIVE)
+    if(rsReputations->overallReputationLevel(item->signature.keyId) == RsReputations::REPUTATION_LOCALLY_NEGATIVE)
     {
         std::cerr << "(WW) Received lobby msg/item from banned identity " << item->signature.keyId << ". Dropping it." << std::endl;
         return ;

libretroshare/src/grouter/p3grouter.cc

@@ -1984,7 +1984,7 @@ bool p3GRouter::verifySignedDataItem(RsGRouterAbstractMsgItem *item,const RsIden
 {
     try
     {
-        if(rsIdentity->overallReputationLevel(item->signature.keyId) == RsReputations::REPUTATION_LOCALLY_NEGATIVE)
+        if(rsReputations->overallReputationLevel(item->signature.keyId) == RsReputations::REPUTATION_LOCALLY_NEGATIVE)
         {
             std::cerr << "(WW) received global router message from banned identity " << item->signature.keyId << ". Rejecting the message." << std::endl;
             return false ;

libretroshare/src/gxs/rsgixs.h

@@ -179,6 +179,7 @@ public:
     virtual bool haveReputation(const RsGxsId &id) = 0;
     virtual bool loadReputation(const RsGxsId &id, const std::list<RsPeerId>& peers) = 0;
     virtual bool getReputation(const RsGxsId &id, GixsReputation &rep) = 0;
+    virtual RsReputations::ReputationLevel overallReputationLevel(const RsGxsId& id) = 0;
 };
 
 /*** This Class pulls all the GXS Interfaces together ****/

libretroshare/src/gxs/rsgxsnetservice.cc

@@ -2738,9 +2738,11 @@ void RsGxsNetService::locked_genReqMsgTransaction(NxsTransaction* tr)
         return ;
     }
 
+#ifdef TO_REMOVE
     int cutoff = 0;
     if(grpMeta != NULL)
         cutoff = grpMeta->mReputationCutOff;
+#endif
 
     GxsMsgReq reqIds;
     reqIds[grpId] = std::vector<RsGxsMessageId>();
@@ -2827,7 +2829,7 @@ void RsGxsNetService::locked_genReqMsgTransaction(NxsTransaction* tr)
             //  - if author is locally banned, do not download.
             //  - if author is not locally banned, download, whatever friends' opinion might be.
             
-        	if(rsIdentity && rsIdentity->overallReputationLevel(syncItem->authorId) == RsReputations::REPUTATION_LOCALLY_NEGATIVE)
+        	if(mReputations->overallReputationLevel(syncItem->authorId) == RsReputations::REPUTATION_LOCALLY_NEGATIVE)
             {
 #ifdef NXS_NET_DEBUG_1
                 GXSNETDEBUG_PG(item->PeerId(),grpId) << ", Identity " << syncItem->authorId << " is banned. Not requesting message!" << std::endl;
@@ -3017,7 +3019,7 @@ void RsGxsNetService::locked_genReqGrpTransaction(NxsTransaction* tr)
         // FIXTESTS global variable rsReputations not available in unittests!
     
 #warning Update the code below to correctly send/recv dependign on reputation
-		if(!grpSyncItem->authorId.isNull() && rsIdentity && rsIdentity->overallReputationLevel(grpSyncItem->authorId) == RsReputations::REPUTATION_LOCALLY_NEGATIVE)
+		if(!grpSyncItem->authorId.isNull() && mReputations->overallReputationLevel(grpSyncItem->authorId) == RsReputations::REPUTATION_LOCALLY_NEGATIVE)
 		{
 #ifdef NXS_NET_DEBUG_0
 			GXSNETDEBUG_PG(tr->mTransaction->PeerId(),grpId) << "  Identity " << grpSyncItem->authorId << " is banned. Not syncing group." << std::endl;

libretroshare/src/retroshare/rsidentity.h

@@ -303,7 +303,6 @@ public:
      * \param id
      * \return
      */
-    virtual RsReputations::ReputationLevel overallReputationLevel(const RsGxsId& id)=0;
     virtual time_t getLastUsageTS(const RsGxsId &id) =0;
 
     // Specific RsIdentity Functions....

libretroshare/src/retroshare/rsreputations.h

@@ -62,6 +62,7 @@ public:
 
 	virtual bool setOwnOpinion(const RsGxsId& key_id, const Opinion& op) =0;
     virtual bool getReputationInfo(const RsGxsId& id, const RsPgpId &ownerNode, ReputationInfo& info) =0;
+    virtual ReputationLevel overallReputationLevel(const RsGxsId& id)=0;
 
     // parameters
 

libretroshare/src/services/p3gxsreputation.cc

@@ -144,6 +144,7 @@ static const uint32_t BANNED_NODES_INACTIVITY_KEEP        = 86400*60; // remove
 
 static const uint32_t REPUTATION_DEFAULT_MIN_VOTES_FOR_REMOTELY_POSITIVE = 1;	// min difference in votes that makes friends opinion globally positive
 static const uint32_t REPUTATION_DEFAULT_MIN_VOTES_FOR_REMOTELY_NEGATIVE = 1;	// min difference in votes that makes friends opinion globally negative
+static const uint32_t MIN_DELAY_BETWEEN_REPUTATION_CONFIG_SAVE = 61 ; // never save more often than once a minute.
 
 p3GxsReputation::p3GxsReputation(p3LinkMgr *lm)
 	:p3Service(), p3Config(),
@@ -162,6 +163,9 @@ p3GxsReputation::p3GxsReputation(p3LinkMgr *lm)
     mAutoSetPositiveOptionToContacts = true;	// default
     mMinVotesForRemotelyPositive = REPUTATION_DEFAULT_MIN_VOTES_FOR_REMOTELY_POSITIVE;
     mMinVotesForRemotelyNegative = REPUTATION_DEFAULT_MIN_VOTES_FOR_REMOTELY_NEGATIVE;
+
+    mLastReputationConfigSaved = 0;
+    mChanged = false ;
 }
 
 const std::string GXS_REPUTATION_APP_NAME = "gxsreputation";
@@ -198,12 +202,12 @@ int	p3GxsReputation::tick()
 
 	if(now > IDENTITY_FLAGS_UPDATE_DELAY+mLastIdentityFlagsUpdate)
 	{
-		updateIdentityFlags() ;
+		updateStaticIdentityFlags() ;
 		mLastIdentityFlagsUpdate = now ;
 	}
 	if(now > BANNED_NODES_UPDATE_DELAY+mLastBannedNodesUpdate)	// 613 is not a multiple of 100, to avoid piling up work
 	{
-		updateIdentityFlags() ;	// needed before updateBannedNodesList!
+		updateStaticIdentityFlags() ;	// needed before updateBannedNodesList!
 		updateBannedNodesProxy();
 		mLastBannedNodesUpdate = now ;
 	}
@@ -223,6 +227,14 @@ int	p3GxsReputation::tick()
 		debug_print() ;
 	}
 #endif
+
+    if(mChanged && now > mLastReputationConfigSaved + MIN_DELAY_BETWEEN_REPUTATION_CONFIG_SAVE)
+    {
+        IndicateConfigChanged() ;
+        mLastReputationConfigSaved = now ;
+        mChanged = false ;
+    }
+
 	return 0;
 }
 
@@ -233,7 +245,9 @@ void p3GxsReputation::setNodeAutoPositiveOpinionForContacts(bool b)
     if(b != mAutoSetPositiveOptionToContacts)
     {
         mLastIdentityFlagsUpdate = 0 ;
+        mLastCleanUp = 0 ;
         mAutoSetPositiveOptionToContacts = b ;
+
         IndicateConfigChanged() ;
     }
 }
@@ -271,12 +285,11 @@ void p3GxsReputation::updateBannedNodesProxy()
             mPerNodeBannedIdsProxy.insert(*it) ;
 }
 
-void p3GxsReputation::updateIdentityFlags()
+void p3GxsReputation::updateStaticIdentityFlags()
 {
     // This function is the *only* place where rsIdentity is called. Normally the cross calls between p3IdService and p3GxsReputations should only
-    // happen one way: from rsIdentity to rsReputations. Still, reputations need to keep track of some identity flags. It's very important to make sure that:
+    // happen one way: from rsIdentity to rsReputations. Still, reputations need to keep track of some identity flags. It's very important to make sure that
-    // - rsIdentity is not called inside a mutex-protected zone, because normally calls happen in the other way.
+    // rsIdentity is not called inside a mutex-protected zone, because normally calls happen in the other way.
-    // -
 
     std::list<RsGxsId> to_update ;
 
@@ -289,11 +302,11 @@ void p3GxsReputation::updateIdentityFlags()
 #endif
 
 	    for( std::map<RsGxsId, Reputation>::iterator rit = mReputations.begin();rit!=mReputations.end();++rit)
+        {
             if( (!(rit->second.mIdentityFlags & REPUTATION_IDENTITY_FLAG_UP_TO_DATE)) && (mPerNodeBannedIdsProxy.find(rit->first) == mPerNodeBannedIdsProxy.end()))
 			    to_update.push_back(rit->first) ;
         }
-
+    }
-    std::list<RsGxsId> should_set_to_positive ;
 
     for(std::list<RsGxsId>::const_iterator rit(to_update.begin());rit!=to_update.end();++rit)
     {
@@ -306,8 +319,6 @@ void p3GxsReputation::updateIdentityFlags()
 #endif
 		    continue ;
 	    }
-        bool is_a_contact = rsIdentity->isARegularContact(*rit) ;
-
         {
             RsStackMutex stack(mReputationMtx); /****** LOCKED MUTEX *******/
             std::map<RsGxsId,Reputation>::iterator it = mReputations.find(*rit) ;
@@ -317,7 +328,7 @@ void p3GxsReputation::updateIdentityFlags()
                 std::cerr << "  Weird situation: item " << *rit << " has been deleted from the list??" << std::endl;
                 continue ;
             }
-            it->second.mIdentityFlags = 0 ;		// resets the NEEDS_UPDATE flag. All other flags set later on.
+            it->second.mIdentityFlags = REPUTATION_IDENTITY_FLAG_UP_TO_DATE ;		// resets the NEEDS_UPDATE flag. All other flags set later on.
 
             if(details.mFlags & RS_IDENTITY_FLAGS_PGP_LINKED)
             {
@@ -326,20 +337,14 @@ void p3GxsReputation::updateIdentityFlags()
             }
             if(details.mFlags & RS_IDENTITY_FLAGS_PGP_KNOWN ) it->second.mIdentityFlags |= REPUTATION_IDENTITY_FLAG_PGP_KNOWN ;
 
-            if(mAutoSetPositiveOptionToContacts && is_a_contact && it->second.mOwnOpinion == RsReputations::OPINION_NEUTRAL)
-                should_set_to_positive.push_back(*rit) ;
-
 #ifdef DEBUG_REPUTATION
             std::cerr << "  updated flags for " << *rit << " to " << std::hex << it->second.mIdentityFlags << std::dec << std::endl;
 #endif
 
             it->second.updateReputation() ;
-            IndicateConfigChanged();
+            mChanged = true ;
         }
     }
-
-    for(std::list<RsGxsId>::const_iterator it(should_set_to_positive.begin());it!=should_set_to_positive.end();++it)
-        setOwnOpinion(*it,RsReputations::OPINION_POSITIVE) ;
 }
 
 void p3GxsReputation::cleanup()
@@ -355,8 +360,6 @@ void p3GxsReputation::cleanup()
     // identities, which would cause an excess of hits to the database. We do it in two steps to avoid a deadlock when calling rsIdentity from here.
     // Also, neutral opinions for banned PGP linked nodes are kept, so as to be able to not request them again.
 
-	bool updated = false ;
-
 	{
 		RsStackMutex stack(mReputationMtx); /****** LOCKED MUTEX *******/
 
@@ -374,15 +377,19 @@ void p3GxsReputation::cleanup()
                 should_delete = true ;
             }
 
-            // Delete slots that havn't been used for a while
+            // Delete slots that havn't been used for a while. The else below is here for debug display purposes, and not harmful since both conditions lead the same effect.
 
-            if(it->second.mLastUsedTS + REPUTATION_INFO_KEEP_DELAY < now)
+            else if(it->second.mLastUsedTS + REPUTATION_INFO_KEEP_DELAY < now)
             {
 #ifdef DEBUG_REPUTATION
                 std::cerr << "  ID " << it->first << ": no request for reputation for more than " << REPUTATION_INFO_KEEP_DELAY/86400 << " days => deleting." << std::endl;
 #endif
                 should_delete = true ;
             }
+#ifdef DEBUG_REPUTATION
+            else
+				std::cerr << "  ID " << it->first << ": flags=" << std::hex << it->second.mIdentityFlags << std::dec << ". Last used: " << (now - it->second.mLastUsedTS)/86400 << " days ago: kept." << std::endl;
+#endif
 
 			if(should_delete)
 			{
@@ -390,7 +397,7 @@ void p3GxsReputation::cleanup()
 				++tmp ;
 				mReputations.erase(it) ;
 				it = tmp ;
-				updated = true ;
+                mChanged = true ;
 			}
 			else
 				++it;
@@ -413,14 +420,31 @@ void p3GxsReputation::cleanup()
                 mBannedPgpIds.erase(it) ;
                 it = tmp ;
 
-                updated = true ;
+                mChanged = true ;
             }
             else
                 ++it ;
     }
 
-	if(updated)
+    // update opinions based on flags and contact information
-		IndicateConfigChanged() ;
+	{
+        std::list<RsGxsId> should_set_to_positive ;
+
+		{
+			RsStackMutex stack(mReputationMtx); /****** LOCKED MUTEX *******/
+
+			for(std::map<RsGxsId,Reputation>::iterator it(mReputations.begin());it!=mReputations.end();++it)
+			{
+				bool is_a_contact = rsIdentity->isARegularContact(*it) ;
+
+				if(mAutoSetPositiveOptionToContacts && is_a_contact && it->second.mOwnOpinion == RsReputations::OPINION_NEUTRAL)
+					should_set_to_positive.push_back(*rit) ;
+			}
+		}
+
+		for(std::list<RsGxsId>::const_iterator it(should_set_to_positive.begin());it!=should_set_to_positive.end();++it)
+			setOwnOpinion(*it,RsReputations::OPINION_POSITIVE) ;
+	}
 }
 
 const float RsReputations::REPUTATION_THRESHOLD_ANTI_SPAM = 1.4f ;
@@ -718,6 +742,14 @@ bool p3GxsReputation::updateLatestUpdate(RsPeerId peerid,time_t latest_update)
  * Opinion
  ****/
 
+RsReputations::ReputationLevel p3GxsReputation::overallReputationLevel(const RsGxsId& id)
+{
+    ReputationInfo info ;
+    getReputationInfo(id,RsPgpId(),info) ;
+
+    return info.mOverallReputationLevel ;
+}
+
 bool p3GxsReputation::getReputationInfo(const RsGxsId& gxsid, const RsPgpId& ownerNode, RsReputations::ReputationInfo& info)
 {
     if(gxsid.isNull())
@@ -751,12 +783,13 @@ bool p3GxsReputation::getReputationInfo(const RsGxsId& gxsid, const RsPgpId& own
         info.mFriendsNegativeVotes = rep.mFriendsNegative ;
         info.mFriendsPositiveVotes = rep.mFriendsPositive ;
 
-        if(rep.mOwnerNode.isNull())
+        if(rep.mOwnerNode.isNull() && !ownerNode.isNull())
             rep.mOwnerNode = ownerNode ;
 
         owner_id = rep.mOwnerNode ;
-
         rep.mLastUsedTS = now ;
+
+		mChanged = true ;
     }
 
     // now compute overall score and reputation

libretroshare/src/services/p3gxsreputation.h

@@ -32,9 +32,9 @@
 #include <map>
 #include <set>
 
-#define REPUTATION_IDENTITY_FLAG_UP_TO_DATE    0x0100
+static const uint32_t  REPUTATION_IDENTITY_FLAG_UP_TO_DATE    = 0x0100;	// This flag means that the static info has been initialised from p3IdService. Normally such a call should happen once.
-#define REPUTATION_IDENTITY_FLAG_PGP_LINKED    0x0001
+static const uint32_t  REPUTATION_IDENTITY_FLAG_PGP_LINKED    = 0x0001;
-#define REPUTATION_IDENTITY_FLAG_PGP_KNOWN     0x0002
+static const uint32_t  REPUTATION_IDENTITY_FLAG_PGP_KNOWN     = 0x0002;
 
 #include "serialiser/rsgxsreputationitems.h"
 
@@ -113,6 +113,7 @@ public:
 
     virtual bool isNodeBanned(const RsPgpId& id);
     virtual void banNode(const RsPgpId& id,bool b) ;
+    virtual ReputationLevel overallReputationLevel(const RsGxsId& id);
 
     virtual void setNodeAutoPositiveOpinionForContacts(bool b) ;
     virtual bool nodeAutoPositiveOpinionForContacts() ;
@@ -155,7 +156,7 @@ private:
     void sendReputationRequests();
     int  sendReputationRequest(RsPeerId peerid);
     void debug_print() ;
-    void updateIdentityFlags();
+    void updateStaticIdentityFlags();
 
 private:
     RsMutex mReputationMtx;
@@ -184,6 +185,9 @@ private:
 
     uint32_t mMinVotesForRemotelyPositive ;
     uint32_t mMinVotesForRemotelyNegative ;
+
+    bool mChanged ; // slow version of IndicateConfigChanged();
+    time_t mLastReputationConfigSaved ;
 };
 
 #endif //SERVICE_RSGXSREPUTATION_HEADER

libretroshare/src/services/p3idservice.cc

@@ -611,15 +611,6 @@ bool p3IdService::getIdDetails(const RsGxsId &id, RsIdentityDetails &details)
     return false;
 }
 
-RsReputations::ReputationLevel p3IdService::overallReputationLevel(const RsGxsId &id)
-{
-#warning some IDs might be deleted but the reputation should still say they are banned.
-    RsIdentityDetails det ;
-    getIdDetails(id,det) ;
-
-    return det.mReputation.mOverallReputationLevel ;
-}
-
 bool p3IdService::isOwnId(const RsGxsId& id)
 {
     RsStackMutex stack(mIdMtx); /********** STACK LOCKED MTX ******/

libretroshare/src/services/p3idservice.h

@@ -273,7 +273,6 @@ public:
 
 	virtual bool setAsRegularContact(const RsGxsId& id,bool is_a_contact) ;
 	virtual bool isARegularContact(const RsGxsId& id) ;
-    virtual RsReputations::ReputationLevel overallReputationLevel(const RsGxsId& id);
 	virtual time_t getLastUsageTS(const RsGxsId &id) ;
 
 	/**************** RsGixs Implementation ***************/