Fix IS: (#1161)

- TXLOCKREQUEST should be processed as normal tx plus some custom logic, should not "fake" inventory
- should not create "fake" local lock, should instead keep track of orphan votes and reprocess them when corresponding TXLOCKREQUEST arrives
- orphan vote time map should be indexed by full outpoint, not by txid of mn collateral

bump MIN_INSTANTSEND_PROTO_VERSION

src/instantx.cpp

@@ -29,10 +29,11 @@ int nCompleteTXLocks;
 std::map<uint256, CTransaction> mapLockRequestAccepted;
 std::map<uint256, CTransaction> mapLockRequestRejected;
 std::map<uint256, CTxLockVote> mapTxLockVotes;
+std::map<uint256, CTxLockVote> mapTxLockVotesOrphan;
 std::map<COutPoint, uint256> mapLockedInputs;
 
 std::map<uint256, CTxLockCandidate> mapTxLockCandidates;
-std::map<uint256, int64_t> mapUnknownVotes; //track votes with no tx for DOS
+std::map<COutPoint, int64_t> mapMasternodeOrphanVotes; //track masternodes who voted with no txreq (for DOS protection)
 
 CCriticalSection cs_instantsend;
 
@@ -51,117 +52,17 @@ void ProcessMessageInstantSend(CNode* pfrom, std::string& strCommand, CDataStrea
     // Ignore any InstantSend messages until masternode list is synced
     if(!masternodeSync.IsMasternodeListSynced()) return;
 
-    if (strCommand == NetMsgType::TXLOCKREQUEST) // InstantSend Transaction Lock Request
+    // NOTE: NetMsgType::TXLOCKREQUEST is handled via ProcessMessage() in main.cpp
-    {
-        //LogPrintf("ProcessMessageInstantSend\n");
-        CDataStream vMsg(vRecv);
-        CTransaction tx;
-        vRecv >> tx;
 
-        // FIXME: this part of simulating inv is not good actually, leaving it only for 12.1 backwards compatibility
+    if (strCommand == NetMsgType::TXLOCKVOTE) // InstantSend Transaction Lock Consensus Votes
-        // and since we are using invs for relaying even initial ix request, this can (and should) be safely removed in 12.2
-        CInv inv(MSG_TXLOCK_REQUEST, tx.GetHash());
-        pfrom->AddInventoryKnown(inv);
-        GetMainSignals().Inventory(inv.hash);
-
-        // have we seen it already?
-        if(mapLockRequestAccepted.count(inv.hash) || mapLockRequestRejected.count(inv.hash)) return;
-        // is it a valid one?
-        if(!IsInstantSendTxValid(tx)) return;
-
-        BOOST_FOREACH(const CTxOut o, tx.vout) {
-            // InstandSend supports normal scripts and unspendable scripts (used in PrivateSend collateral and Governance collateral).
-            // TODO: Look into other script types that are normal and can be included
-            if(!o.scriptPubKey.IsNormalPaymentScript() && !o.scriptPubKey.IsUnspendable()) {
-                LogPrintf("ProcessMessageInstantSend -- Invalid Script %s", tx.ToString());
-                return;
-            }
-        }
-
-        int nBlockHeight = CreateTxLockCandidate(tx);
-
-        bool fMissingInputs = false;
-        CValidationState state;
-
-        bool fAccepted = false;
-        {
-            LOCK(cs_main);
-            fAccepted = AcceptToMemoryPool(mempool, state, tx, true, &fMissingInputs);
-        }
-        if(fAccepted) {
-            RelayInv(inv);
-
-            CreateTxLockVote(tx, nBlockHeight);
-
-            mapLockRequestAccepted.insert(std::make_pair(tx.GetHash(), tx));
-
-            LogPrintf("ProcessMessageInstantSend -- Transaction Lock Request: %s %s : accepted %s\n",
-                pfrom->addr.ToString(), pfrom->cleanSubVer,
-                tx.GetHash().ToString()
-            );
-
-            // Masternodes will sometimes propagate votes before the transaction is known to the client.
-            // If this just happened - update transaction status, try forcing external script notification,
-            // lock inputs and resolve conflicting locks
-            if(IsLockedInstandSendTransaction(tx.GetHash())) {
-                UpdateLockedTransaction(tx, true);
-                LockTransactionInputs(tx);
-                ResolveConflicts(tx);
-            }
-
-            return;
-
-        } else {
-            mapLockRequestRejected.insert(std::make_pair(tx.GetHash(), tx));
-
-            // can we get the conflicting transaction as proof?
-
-            LogPrintf("ProcessMessageInstantSend -- Transaction Lock Request: %s %s : rejected %s\n",
-                pfrom->addr.ToString(), pfrom->cleanSubVer,
-                tx.GetHash().ToString()
-            );
-
-            LockTransactionInputs(tx);
-            ResolveConflicts(tx);
-
-            return;
-        }
-    }
-    else if (strCommand == NetMsgType::TXLOCKVOTE) // InstantSend Transaction Lock Consensus Votes
     {
         CTxLockVote vote;
         vRecv >> vote;
 
-        CInv inv(MSG_TXLOCK_VOTE, vote.GetHash());
-        pfrom->AddInventoryKnown(inv);
-
         if(mapTxLockVotes.count(vote.GetHash())) return;
         mapTxLockVotes.insert(std::make_pair(vote.GetHash(), vote));
 
-        if(ProcessTxLockVote(pfrom, vote)) {
+        ProcessTxLockVote(pfrom, vote);
-            //Spam/Dos protection
-            /*
-                Masternodes will sometimes propagate votes before the transaction is known to the client.
-                This tracks those messages and allows it at the same rate of the rest of the network, if
-                a peer violates it, it will simply be ignored
-            */
-            if(!mapLockRequestAccepted.count(vote.txHash) && !mapLockRequestRejected.count(vote.txHash)) {
-                if(!mapUnknownVotes.count(vote.vinMasternode.prevout.hash))
-                    mapUnknownVotes[vote.vinMasternode.prevout.hash] = GetTime()+(60*10);
-
-                if(mapUnknownVotes[vote.vinMasternode.prevout.hash] > GetTime() &&
-                    mapUnknownVotes[vote.vinMasternode.prevout.hash] - GetAverageUnknownVoteTime() > 60*10) {
-                        LogPrintf("ProcessMessageInstantSend -- masternode is spamming transaction votes: %s %s\n",
-                            vote.vinMasternode.ToString(),
-                            vote.txHash.ToString()
-                        );
-                        return;
-                } else {
-                    mapUnknownVotes[vote.vinMasternode.prevout.hash] = GetTime()+(60*10);
-                }
-            }
-            RelayInv(inv);
-        }
 
         return;
     }
@@ -218,6 +119,46 @@ bool IsInstantSendTxValid(const CTransaction& txCandidate)
     return true;
 }
 
+bool ProcessTxLockRequest(CNode* pfrom, const CTransaction &tx)
+{
+    if(!IsInstantSendTxValid(tx)) return false;
+
+    BOOST_FOREACH(const CTxOut o, tx.vout) {
+        // InstandSend supports normal scripts and unspendable scripts (used in PrivateSend collateral and Governance collateral).
+        // TODO: Look into other script types that are normal and can be included
+        if(!o.scriptPubKey.IsNormalPaymentScript() && !o.scriptPubKey.IsUnspendable()) {
+            LogPrintf("TXLOCKREQUEST -- Invalid Script %s", tx.ToString());
+            return false;
+        }
+    }
+
+    int nBlockHeight = CreateTxLockCandidate(tx);
+    if(!nBlockHeight) {
+        // smth is not right
+        return false;
+    }
+
+    uint256 txHash = tx.GetHash();
+    mapLockRequestAccepted.insert(std::make_pair(txHash, tx));
+
+    LogPrintf("TXLOCKREQUEST -- Transaction Lock Request: %s %s : accepted %s\n",
+            pfrom ? pfrom->addr.ToString() : "", pfrom ? pfrom->cleanSubVer : "", txHash.ToString());
+
+    CreateTxLockVote(tx, nBlockHeight);
+    ProcessOrphanTxLockVotes();
+
+    // Masternodes will sometimes propagate votes before the transaction is known to the client.
+    // If this just happened - update transaction status, try forcing external script notification,
+    // lock inputs and resolve conflicting locks
+    if(IsLockedInstandSendTransaction(txHash)) {
+        UpdateLockedTransaction(tx, true);
+        LockTransactionInputs(tx);
+        ResolveConflicts(tx);
+    }
+
+    return true;
+}
+
 int64_t CreateTxLockCandidate(const CTransaction& tx)
 {
     // Find the age of the first input but all inputs must be old enough too
@@ -311,6 +252,36 @@ void CreateTxLockVote(const CTransaction& tx, int nBlockHeight)
 //received a consensus vote
 bool ProcessTxLockVote(CNode* pnode, CTxLockVote& vote)
 {
+    // Masternodes will sometimes propagate votes before the transaction is known to the client,
+    // will actually process only after the lock request itself has arrived
+    if(!mapLockRequestAccepted.count(vote.txHash)) {
+        if(!mapTxLockVotesOrphan.count(vote.GetHash())) {
+            LogPrint("instantsend", "ProcessTxLockVote -- Orphan vote: txid=%s  masternode=%s new\n", vote.txHash.ToString(), vote.vinMasternode.prevout.ToStringShort());
+            vote.nOrphanExpireTime = GetTime() + 60; // keep orphan votes for 1 minute
+            mapTxLockVotesOrphan[vote.GetHash()] = vote;
+        } else {
+            LogPrint("instantsend", "ProcessTxLockVote -- Orphan vote: txid=%s  masternode=%s seen\n", vote.txHash.ToString(), vote.vinMasternode.prevout.ToStringShort());
+        }
+
+        // This tracks those messages and allows only the same rate as of the rest of the network
+
+        int nMasternodeOrphanExpireTime = GetTime() + 60*10; // keep time data for 10 minutes
+        if(!mapMasternodeOrphanVotes.count(vote.vinMasternode.prevout)) {
+            mapMasternodeOrphanVotes[vote.vinMasternode.prevout] = nMasternodeOrphanExpireTime;
+        } else {
+            int64_t nPrevOrphanVote = mapMasternodeOrphanVotes[vote.vinMasternode.prevout];
+            if(nPrevOrphanVote > GetTime() && nPrevOrphanVote > GetAverageMasternodeOrphanVoteTime()) {
+                LogPrint("instantsend", "ProcessTxLockVote -- masternode is spamming orphan Transaction Lock Votes: txid=%s  masternode=%s\n",
+                        vote.vinMasternode.prevout.ToStringShort(), vote.txHash.ToString());
+                // Misbehaving(pfrom->id, 1);
+                return false;
+            }
+            // not spamming, refresh
+            mapMasternodeOrphanVotes[vote.vinMasternode.prevout] = nMasternodeOrphanExpireTime;
+        }
+
+        return true;
+    }
 
     LogPrint("instantsend", "ProcessTxLockVote -- Transaction Lock Vote, txid=%s\n", vote.txHash.ToString());
 
@@ -324,7 +295,9 @@ bool ProcessTxLockVote(CNode* pnode, CTxLockVote& vote)
     if(n == -1) {
         //can be caused by past versions trying to vote with an invalid protocol
         LogPrint("instantsend", "ProcessTxLockVote -- Outdated masternode %s\n", vote.vinMasternode.prevout.ToStringShort());
-        mnodeman.AskForMN(pnode, vote.vinMasternode);
+        if(pnode) {
+            mnodeman.AskForMN(pnode, vote.vinMasternode);
+        }
         return false;
     }
     LogPrint("instantsend", "ProcessTxLockVote -- Masternode %s, rank=%d\n", vote.vinMasternode.prevout.ToStringShort(), n);
@@ -338,56 +311,54 @@ bool ProcessTxLockVote(CNode* pnode, CTxLockVote& vote)
     if(!vote.CheckSignature()) {
         LogPrintf("ProcessTxLockVote -- Signature invalid\n");
         // don't ban, it could just be a non-synced masternode
-        mnodeman.AskForMN(pnode, vote.vinMasternode);
+        if(pnode) {
+            mnodeman.AskForMN(pnode, vote.vinMasternode);
+        }
         return false;
     }
 
-    if (!mapTxLockCandidates.count(vote.txHash)) {
+    if(!mapTxLockCandidates.count(vote.txHash)) {
-        LogPrintf("ProcessTxLockVote -- New Transaction Lock Candidate! txid=%s\n", vote.txHash.ToString());
+        // this should never happen
-
+        return false;
-        CTxLockCandidate txLockCandidate;
-        txLockCandidate.nBlockHeight = 0;
-        //locks expire after nInstantSendKeepLock confirmations
-        txLockCandidate.nExpirationBlock = chainActive.Height() + Params().GetConsensus().nInstantSendKeepLock;
-        txLockCandidate.nTimeout = GetTime()+(60*5);
-        txLockCandidate.txHash = vote.txHash;
-        mapTxLockCandidates.insert(std::make_pair(vote.txHash, txLockCandidate));
-    } else {
-        LogPrint("instantsend", "ProcessTxLockVote -- Transaction Lock Exists! txid=%s\n", vote.txHash.ToString());
     }
 
     //compile consessus vote
-    std::map<uint256, CTxLockCandidate>::iterator i = mapTxLockCandidates.find(vote.txHash);
+    mapTxLockCandidates[vote.txHash].AddVote(vote);
-    if (i != mapTxLockCandidates.end()) {
-        (*i).second.AddVote(vote);
 
-        int nSignatures = (*i).second.CountVotes();
+    int nSignatures = mapTxLockCandidates[vote.txHash].CountVotes();
-        LogPrint("instantsend", "ProcessTxLockVote -- Transaction Lock signatures count: %d, vote hash=%s\n", nSignatures, vote.GetHash().ToString());
+    LogPrint("instantsend", "ProcessTxLockVote -- Transaction Lock signatures count: %d, vote hash=%s\n", nSignatures, vote.GetHash().ToString());
 
-        if(nSignatures >= INSTANTSEND_SIGNATURES_REQUIRED) {
+    if(nSignatures >= INSTANTSEND_SIGNATURES_REQUIRED) {
-            LogPrint("instantsend", "ProcessTxLockVote -- Transaction Lock Is Complete! txid=%s\n", vote.txHash.ToString());
+        LogPrint("instantsend", "ProcessTxLockVote -- Transaction Lock Is Complete! txid=%s\n", vote.txHash.ToString());
 
-            // Masternodes will sometimes propagate votes before the transaction is known to the client,
+        if(!FindConflictingLocks(mapLockRequestAccepted[vote.txHash])) { //?????
-            // will check for conflicting locks and update transaction status on a new vote message
+            if(mapLockRequestAccepted.count(vote.txHash)) {
-            // only after the lock itself has arrived
+                UpdateLockedTransaction(mapLockRequestAccepted[vote.txHash]);
-            if(!mapLockRequestAccepted.count(vote.txHash) && !mapLockRequestRejected.count(vote.txHash)) return true;
+                LockTransactionInputs(mapLockRequestAccepted[vote.txHash]);
-
+            } else if(mapLockRequestRejected.count(vote.txHash)) {
-            if(!FindConflictingLocks(mapLockRequestAccepted[vote.txHash])) { //?????
+                ResolveConflicts(mapLockRequestRejected[vote.txHash]); ///?????
-                if(mapLockRequestAccepted.count(vote.txHash)) {
+            } else {
-                    UpdateLockedTransaction(mapLockRequestAccepted[vote.txHash]);
+                LogPrint("instantsend", "ProcessTxLockVote -- Transaction Lock is missing! nSignatures=%d, vote hash=%s\n", nSignatures, vote.GetHash().ToString());
-                    LockTransactionInputs(mapLockRequestAccepted[vote.txHash]);
-                } else if(mapLockRequestRejected.count(vote.txHash)) {
-                    ResolveConflicts(mapLockRequestRejected[vote.txHash]); ///?????
-                } else {
-                    LogPrint("instantsend", "ProcessTxLockVote -- Transaction Lock Request is missing! nSignatures=%d, vote hash %s\n", nSignatures, vote.GetHash().ToString());
-                }
             }
         }
-        return true;
     }
 
+    CInv inv(MSG_TXLOCK_VOTE, vote.GetHash());
+    RelayInv(inv);
 
-    return false;
+    return true;
+}
+
+void ProcessOrphanTxLockVotes()
+{
+    std::map<uint256, CTxLockVote>::iterator it = mapTxLockVotesOrphan.begin();
+    while(it != mapTxLockVotesOrphan.end()) {
+        if(ProcessTxLockVote(NULL, it->second)) {
+            mapTxLockVotesOrphan.erase(it++);
+        } else {
+            ++it;
+        }
+    }
 }
 
 void UpdateLockedTransaction(const CTransaction& tx, bool fForceNotification)
@@ -471,20 +442,20 @@ void ResolveConflicts(const CTransaction& tx)
     }
 }
 
-int64_t GetAverageUnknownVoteTime()
+int64_t GetAverageMasternodeOrphanVoteTime()
 {
-    // should never actually call this function when mapUnknownVotes is empty
+    // NOTE: should never actually call this function when mapMasternodeOrphanVotes is empty
-    if(mapUnknownVotes.empty()) return 0;
+    if(mapMasternodeOrphanVotes.empty()) return 0;
 
-    std::map<uint256, int64_t>::iterator it = mapUnknownVotes.begin();
+    std::map<COutPoint, int64_t>::iterator it = mapMasternodeOrphanVotes.begin();
     int64_t total = 0;
 
-    while(it != mapUnknownVotes.end()) {
+    while(it != mapMasternodeOrphanVotes.end()) {
         total+= it->second;
         ++it;
     }
 
-    return total / mapUnknownVotes.size();
+    return total / mapMasternodeOrphanVotes.size();
 }
 
 void CleanTxLockCandidates()
@@ -523,6 +494,28 @@ void CleanTxLockCandidates()
             ++it;
         }
     }
+
+    // clean expired orphan votes
+    std::map<uint256, CTxLockVote>::iterator it1 = mapTxLockVotesOrphan.begin();
+    while(it1 != mapTxLockVotesOrphan.end()) {
+        if(it1->second.nOrphanExpireTime < GetTime()) {
+            LogPrint("instantsend", "CleanTxLockCandidates -- Removing expired orphan vote: txid=%s  masternode=%s\n", it1->second.txHash.ToString(), it1->second.vinMasternode.prevout.ToStringShort());
+            mapTxLockVotesOrphan.erase(it1++);
+        } else {
+            ++it1;
+        }
+    }
+
+    // clean expired masternode orphan vote times
+    std::map<COutPoint, int64_t>::iterator it2 = mapMasternodeOrphanVotes.begin();
+    while(it2 != mapMasternodeOrphanVotes.end()) {
+        if(it2->second < GetTime()) {
+            LogPrint("instantsend", "CleanTxLockCandidates -- Removing expired orphan masternode vote time: masternode=%s\n", it2->first.ToStringShort());
+            mapMasternodeOrphanVotes.erase(it2++);
+        } else {
+            ++it2;
+        }
+    }
 }
 
 bool IsLockedInstandSendTransaction(const uint256 &txHash)

src/instantx.h

@@ -28,7 +28,7 @@ static const int INSTANTSEND_SIGNATURES_REQUIRED    = 6;
 static const int INSTANTSEND_SIGNATURES_TOTAL       = 10;
 static const int DEFAULT_INSTANTSEND_DEPTH          = 5;
 
-static const int MIN_INSTANTSEND_PROTO_VERSION      = 70202;
+static const int MIN_INSTANTSEND_PROTO_VERSION      = 70203;
 static const CAmount INSTANTSEND_MIN_FEE            = 0.001 * COIN;
 
 extern bool fEnableInstantSend;
@@ -45,6 +45,8 @@ void ProcessMessageInstantSend(CNode* pfrom, std::string& strCommand, CDataStrea
 
 bool IsInstantSendTxValid(const CTransaction& txCandidate);
 
+bool ProcessTxLockRequest(CNode* pfrom, const CTransaction &tx);
+
 int64_t CreateTxLockCandidate(const CTransaction &tx);
 
 //check if we need to vote on this transaction
@@ -53,6 +55,8 @@ void CreateTxLockVote(const CTransaction& tx, int nBlockHeight);
 //process consensus vote message
 bool ProcessTxLockVote(CNode *pnode, CTxLockVote& vote);
 
+void ProcessOrphanTxLockVotes();
+
 //update UI and notify external script if any
 void UpdateLockedTransaction(const CTransaction& tx, bool fForceNotification = false);
 
@@ -76,7 +80,7 @@ int GetTransactionLockSignatures(const uint256 &txHash);
 // verify if transaction lock timed out
 bool IsTransactionLockTimedOut(const uint256 &txHash);
 
-int64_t GetAverageUnknownVoteTime();
+int64_t GetAverageMasternodeOrphanVoteTime();
 
 class CTxLockVote
 {
@@ -86,6 +90,9 @@ public:
     int nBlockHeight;
     std::vector<unsigned char> vchMasterNodeSignature;
 
+    // local memory only
+    int64_t nOrphanExpireTime;
+
     ADD_SERIALIZE_METHODS;
 
     template <typename Stream, typename Operation>

src/main.cpp

@@ -5685,7 +5685,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
     }
 
 
-    else if (strCommand == NetMsgType::TX || strCommand == NetMsgType::DSTX)
+    else if (strCommand == NetMsgType::TX || strCommand == NetMsgType::DSTX || strCommand == NetMsgType::TXLOCKREQUEST)
     {
         // Stop processing the transaction early if
         // We are in blocks only mode and peer is either not whitelisted or whitelistrelay is off
@@ -5735,6 +5735,9 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
             LogPrintf("DSTX -- Got Masternode transaction %s\n", hashTx.ToString());
             mempool.PrioritiseTransaction(hashTx, hashTx.ToString(), 1000, 0.1*COIN);
             pmn->fAllowMixingTx = false;
+        } else if (strCommand == NetMsgType::TXLOCKREQUEST) {
+            vRecv >> tx;
+            nInvType = MSG_TXLOCK_REQUEST;
         }
 
         CInv inv(nInvType, tx.GetHash());
@@ -5750,8 +5753,11 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
 
         if (!AlreadyHave(inv) && AcceptToMemoryPool(mempool, state, tx, true, &fMissingInputs))
         {
+            // Process custom txes
             if (strCommand == NetMsgType::DSTX) {
                 mapDarksendBroadcastTxes.insert(make_pair(tx.GetHash(), dstx));
+            } else if (strCommand == NetMsgType::TXLOCKREQUEST) {
+                if(!ProcessTxLockRequest(pfrom, tx)) return false;
             }
 
             mempool.check(pcoinsTip);
@@ -5830,6 +5836,20 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
             assert(recentRejects);
             recentRejects->insert(tx.GetHash());
 
+            if (strCommand == NetMsgType::TXLOCKREQUEST && !AlreadyHave(inv)) { // i.e. AcceptToMemoryPool failed
+                mapLockRequestRejected.insert(std::make_pair(tx.GetHash(), tx));
+
+                // can we get the conflicting transaction as proof?
+
+                LogPrintf("TXLOCKREQUEST -- Transaction Lock Request: %s %s : rejected %s\n",
+                    pfrom->addr.ToString(), pfrom->cleanSubVer,
+                    tx.GetHash().ToString()
+                );
+
+                LockTransactionInputs(tx);
+                ResolveConflicts(tx);
+            }
+
             if (pfrom->fWhitelisted && GetBoolArg("-whitelistforcerelay", DEFAULT_WHITELISTFORCERELAY)) {
                 // Always relay transactions received from whitelisted peers, even
                 // if they were already in the mempool or rejected from it due