Streamline, refactor and unify PS checks for mixing entries and final txes (#3246)

* Move PS mixing entry verification on masternodes into `AddEntry()`

Also streamline logic a bit and drop unused/excessive parts.

* Unify PS checks among masternodes and clients

* No need to re-check outputs over and over again

* No need to count, fail early if any output is missing

* No need to look any further once we found the input we expected

A tx with duplicate inputs would be considered invalid anyway and we also know there are no duplicates because we just verified the final tx above.

Also drop an unused variable.

* Unify LogPrint-s

* Drop human-readable strings for unused PoolMessage-s

* Apply suggestions from code review

Co-Authored-By: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>

* Re-introduce zero-fee checks

* fix log

* Move all txin/txout verification logic shared by CPrivateSendClientSession::SignFinalTransaction() and CPrivateSendServer::AddEntry() into CPrivateSendBaseSession::IsValidInOuts()

* fix nit

* Add missing return

* Use CCoinsViewMemPool instead of doing it manually

Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>
Co-authored-by: Alexander Block <ablock84@gmail.com>

Author: 3 people

src/privatesend/privatesend-client.cpp

@@ -541,85 +541,97 @@ bool CPrivateSendClientSession::SignFinalTransaction(const CTransaction& finalTr
     if (!mixingMasternode) return false;
 
     finalMutableTransaction = finalTransactionNew;
-    LogPrint(BCLog::PRIVATESEND, "CPrivateSendClientSession::SignFinalTransaction -- finalMutableTransaction=%s", finalMutableTransaction.ToString());
+    LogPrint(BCLog::PRIVATESEND, "CPrivateSendClientSession::%s -- finalMutableTransaction=%s", __func__, finalMutableTransaction.ToString());
+
+    // STEP 1: check final transaction general rules
 
     // Make sure it's BIP69 compliant
     sort(finalMutableTransaction.vin.begin(), finalMutableTransaction.vin.end(), CompareInputBIP69());
     sort(finalMutableTransaction.vout.begin(), finalMutableTransaction.vout.end(), CompareOutputBIP69());
 
     if (finalMutableTransaction.GetHash() != finalTransactionNew.GetHash()) {
-        LogPrint(BCLog::PRIVATESEND, "CPrivateSendClientSession::SignFinalTransaction -- WARNING! Masternode %s is not BIP69 compliant!\n", mixingMasternode->proTxHash.ToString());
+        LogPrint(BCLog::PRIVATESEND, "CPrivateSendClientSession::%s -- ERROR! Masternode %s is not BIP69 compliant!\n", __func__, mixingMasternode->proTxHash.ToString());
+        UnlockCoins();
+        keyHolderStorage.ReturnAll();
+        SetNull();
+        return false;
+    }
+
+    // Make sure all inputs/outputs are valid
+    PoolMessage nMessageID{MSG_NOERR};
+    if (!IsValidInOuts(finalMutableTransaction.vin, finalMutableTransaction.vout, nMessageID, nullptr)) {
+        LogPrint(BCLog::PRIVATESEND, "CPrivateSendClientSession::%s -- ERROR! IsValidInOuts() failed: %s\n", __func__, CPrivateSend::GetMessageByID(nMessageID));
         UnlockCoins();
         keyHolderStorage.ReturnAll();
         SetNull();
         return false;
     }
 
+    // STEP 2: make sure our own inputs/outputs are present, otherwise refuse to sign
+
     std::vector<CTxIn> sigs;
 
-    //make sure my inputs/outputs are present, otherwise refuse to sign
     for (const auto& entry : vecEntries) {
+        // Check that the final transaction has all our outputs
+        for (const auto& txout : entry.vecTxOut) {
+            bool fFound = false;
+            for (const auto& txoutFinal : finalMutableTransaction.vout) {
+                if (txoutFinal == txout) {
+                    fFound = true;
+                    break;
+                }
+            }
+            if (!fFound) {
+                // Something went wrong and we'll refuse to sign. It's possible we'll be charged collateral. But that's
+                // better than signing if the transaction doesn't look like what we wanted.
+                LogPrint(BCLog::PRIVATESEND, "CPrivateSendClientSession::%s -- an output is missing, refusing to sign! txout=%s\n", txout.ToString());
+                UnlockCoins();
+                keyHolderStorage.ReturnAll();
+                SetNull();
+                return false;
+            }
+        }
+
         for (const auto& txdsin : entry.vecTxDSIn) {
             /* Sign my transaction and all outputs */
             int nMyInputIndex = -1;
             CScript prevPubKey = CScript();
-            CTxIn txin = CTxIn();
 
             for (unsigned int i = 0; i < finalMutableTransaction.vin.size(); i++) {
                 if (finalMutableTransaction.vin[i] == txdsin) {
                     nMyInputIndex = i;
                     prevPubKey = txdsin.prevPubKey;
-                    txin = txdsin;
+                    break;
                 }
             }
 
-            if (nMyInputIndex >= 0) { //might have to do this one input at a time?
-                int nFoundOutputsCount = 0;
-                CAmount nValue1 = 0;
-                CAmount nValue2 = 0;
-
-                for (const auto& txoutFinal : finalMutableTransaction.vout) {
-                    for (const auto& txout : entry.vecTxOut) {
-                        if (txoutFinal == txout) {
-                            nFoundOutputsCount++;
-                            nValue1 += txoutFinal.nValue;
-                        }
-                    }
-                }
-
-                for (const auto& txout : entry.vecTxOut) {
-                    nValue2 += txout.nValue;
-                }
-
-                int nTargetOuputsCount = entry.vecTxOut.size();
-                if (nFoundOutputsCount < nTargetOuputsCount || nValue1 != nValue2) {
-                    // in this case, something went wrong and we'll refuse to sign. It's possible we'll be charged collateral. But that's
-                    // better then signing if the transaction doesn't look like what we wanted.
-                    LogPrint(BCLog::PRIVATESEND, "CPrivateSendClientSession::SignFinalTransaction -- My entries are not correct! Refusing to sign: nFoundOutputsCount: %d, nTargetOuputsCount: %d\n", nFoundOutputsCount, nTargetOuputsCount);
-                    UnlockCoins();
-                    keyHolderStorage.ReturnAll();
-                    SetNull();
-
-                    return false;
-                }
-
-                const CKeyStore& keystore = *vpwallets[0];
+            if (nMyInputIndex == -1) {
+                // Can't find one of my own inputs, refuse to sign. It's possible we'll be charged collateral. But that's
+                // better than signing if the transaction doesn't look like what we wanted.
+                LogPrint(BCLog::PRIVATESEND, "CPrivateSendClientSession::%s -- missing input! txdsin=%s\n", __func__, txdsin.ToString());
+                UnlockCoins();
+                keyHolderStorage.ReturnAll();
+                SetNull();
+                return false;
+            }
 
-                LogPrint(BCLog::PRIVATESEND, "CPrivateSendClientSession::SignFinalTransaction -- Signing my input %i\n", nMyInputIndex);
-                // TODO we're using amount=0 here but we should use the correct amount. This works because Dash ignores the amount while signing/verifying (only used in Bitcoin/Segwit)
-                if (!SignSignature(keystore, prevPubKey, finalMutableTransaction, nMyInputIndex, 0, int(SIGHASH_ALL | SIGHASH_ANYONECANPAY))) { // changes scriptSig
-                    LogPrint(BCLog::PRIVATESEND, "CPrivateSendClientSession::SignFinalTransaction -- Unable to sign my own transaction!\n");
-                    // not sure what to do here, it will timeout...?
-                }
+            const CKeyStore& keystore = *vpwallets[0];
 
-                sigs.push_back(finalMutableTransaction.vin[nMyInputIndex]);
-                LogPrint(BCLog::PRIVATESEND, "CPrivateSendClientSession::SignFinalTransaction -- nMyInputIndex: %d, sigs.size(): %d, scriptSig=%s\n", nMyInputIndex, (int)sigs.size(), ScriptToAsmStr(finalMutableTransaction.vin[nMyInputIndex].scriptSig));
+            LogPrint(BCLog::PRIVATESEND, "CPrivateSendClientSession::%s -- Signing my input %i\n", __func__, nMyInputIndex);
+            // TODO we're using amount=0 here but we should use the correct amount. This works because Dash ignores the amount while signing/verifying (only used in Bitcoin/Segwit)
+            if (!SignSignature(keystore, prevPubKey, finalMutableTransaction, nMyInputIndex, 0, int(SIGHASH_ALL | SIGHASH_ANYONECANPAY))) { // changes scriptSig
+                LogPrint(BCLog::PRIVATESEND, "CPrivateSendClientSession::%s -- Unable to sign my own transaction!\n", __func__);
+                // not sure what to do here, it will timeout...?
             }
+
+            sigs.push_back(finalMutableTransaction.vin[nMyInputIndex]);
+            LogPrint(BCLog::PRIVATESEND, "CPrivateSendClientSession::%s -- nMyInputIndex: %d, sigs.size(): %d, scriptSig=%s\n",
+                    __func__, nMyInputIndex, (int)sigs.size(), ScriptToAsmStr(finalMutableTransaction.vin[nMyInputIndex].scriptSig));
         }
     }
 
     if (sigs.empty()) {
-        LogPrint(BCLog::PRIVATESEND, "CPrivateSendClientSession::SignFinalTransaction -- can't sign anything!\n");
+        LogPrint(BCLog::PRIVATESEND, "CPrivateSendClientSession::%s -- can't sign anything!\n", __func__);
         UnlockCoins();
         keyHolderStorage.ReturnAll();
         SetNull();
@@ -628,7 +640,7 @@ bool CPrivateSendClientSession::SignFinalTransaction(const CTransaction& finalTr
     }
 
     // push all of our signatures to the Masternode
-    LogPrint(BCLog::PRIVATESEND, "CPrivateSendClientSession::SignFinalTransaction -- pushing sigs to the masternode, finalMutableTransaction=%s", finalMutableTransaction.ToString());
+    LogPrint(BCLog::PRIVATESEND, "CPrivateSendClientSession::%s -- pushing sigs to the masternode, finalMutableTransaction=%s", __func__, finalMutableTransaction.ToString());
     CNetMsgMaker msgMaker(pnode->GetSendVersion());
     connman.PushMessage(pnode, msgMaker.Make(NetMsgType::DSSIGNFINALTX, sigs));
     SetState(POOL_STATE_SIGNING);

src/privatesend/privatesend-server.cpp

@@ -178,100 +178,10 @@ void CPrivateSendServer::ProcessMessage(CNode* pfrom, const std::string& strComm
 
         LogPrint(BCLog::PRIVATESEND, "DSVIN -- txCollateral %s", entry.txCollateral->ToString());
 
-        if (entry.vecTxDSIn.size() != entry.vecTxOut.size()) {
-            LogPrint(BCLog::PRIVATESEND, "DSVIN -- ERROR: inputs vs outputs size mismatch! %d vs %d\n", entry.vecTxDSIn.size(), entry.vecTxOut.size());
-            PushStatus(pfrom, STATUS_REJECTED, ERR_SIZE_MISMATCH, connman);
-            ConsumeCollateral(connman, entry.txCollateral);
-            return;
-        }
-
-        if (entry.vecTxDSIn.size() > PRIVATESEND_ENTRY_MAX_SIZE) {
-            LogPrint(BCLog::PRIVATESEND, "DSVIN -- ERROR: too many inputs! %d/%d\n", entry.vecTxDSIn.size(), PRIVATESEND_ENTRY_MAX_SIZE);
-            PushStatus(pfrom, STATUS_REJECTED, ERR_MAXIMUM, connman);
-            ConsumeCollateral(connman, entry.txCollateral);
-            return;
-        }
-
-        // do we have the same denominations as the current session?
-        if (!IsOutputsCompatibleWithSessionDenom(entry.vecTxOut)) {
-            LogPrint(BCLog::PRIVATESEND, "DSVIN -- not compatible with existing transactions!\n");
-            PushStatus(pfrom, STATUS_REJECTED, ERR_EXISTING_TX, connman);
-            return;
-        }
-
-        // check it like a transaction
-        {
-            CAmount nValueIn = 0;
-            CAmount nValueOut = 0;
-
-            CMutableTransaction tx;
-
-            for (const auto& txout : entry.vecTxOut) {
-                nValueOut += txout.nValue;
-                tx.vout.push_back(txout);
-
-                if (txout.scriptPubKey.size() != 25) {
-                    LogPrint(BCLog::PRIVATESEND, "DSVIN -- non-standard pubkey detected! scriptPubKey=%s\n", ScriptToAsmStr(txout.scriptPubKey));
-                    PushStatus(pfrom, STATUS_REJECTED, ERR_NON_STANDARD_PUBKEY, connman);
-                    ConsumeCollateral(connman, entry.txCollateral);
-                    return;
-                }
-                if (!txout.scriptPubKey.IsPayToPublicKeyHash()) {
-                    LogPrint(BCLog::PRIVATESEND, "DSVIN -- invalid script! scriptPubKey=%s\n", ScriptToAsmStr(txout.scriptPubKey));
-                    PushStatus(pfrom, STATUS_REJECTED, ERR_INVALID_SCRIPT, connman);
-                    ConsumeCollateral(connman, entry.txCollateral);
-                    return;
-                }
-            }
-
-            for (const auto& txin : entry.vecTxDSIn) {
-                tx.vin.push_back(txin);
-
-                LogPrint(BCLog::PRIVATESEND, "DSVIN -- txin=%s\n", txin.ToString());
-
-                Coin coin;
-                auto mempoolTx = mempool.get(txin.prevout.hash);
-                if (mempoolTx != nullptr) {
-                    if (mempool.isSpent(txin.prevout) || !llmq::quorumInstantSendManager->IsLocked(txin.prevout.hash)) {
-                        LogPrint(BCLog::PRIVATESEND, "DSVIN -- spent or non-locked mempool input! txin=%s\n", txin.ToString());
-                        PushStatus(pfrom, STATUS_REJECTED, ERR_MISSING_TX, connman);
-                        return;
-                    }
-                    if (!CPrivateSend::IsDenominatedAmount(mempoolTx->vout[txin.prevout.n].nValue)) {
-                        LogPrintf("DSVIN -- non-denominated mempool input! txin=%s\n", txin.ToString());
-                        PushStatus(pfrom, STATUS_REJECTED, ERR_DENOM, connman);
-                        ConsumeCollateral(connman, entry.txCollateral);
-                        return;
-                    }
-                    nValueIn += mempoolTx->vout[txin.prevout.n].nValue;
-                } else if (GetUTXOCoin(txin.prevout, coin)) {
-                    if (!CPrivateSend::IsDenominatedAmount(coin.out.nValue)) {
-                        LogPrintf("DSVIN -- non-denominated input! txin=%s\n", txin.ToString());
-                        PushStatus(pfrom, STATUS_REJECTED, ERR_DENOM, connman);
-                        ConsumeCollateral(connman, entry.txCollateral);
-                        return;
-                    }
-                    nValueIn += coin.out.nValue;
-                } else {
-                    LogPrint(BCLog::PRIVATESEND, "DSVIN -- missing input! txin=%s\n", txin.ToString());
-                    PushStatus(pfrom, STATUS_REJECTED, ERR_MISSING_TX, connman);
-                    return;
-                }
-            }
-
-            // There should be no fee in mixing tx
-            CAmount nFee = nValueIn - nValueOut;
-            if (nFee != 0) {
-                LogPrint(BCLog::PRIVATESEND, "DSVIN -- there should be no fee in mixing tx! fees: %lld, tx=%s", nFee, tx.ToString());
-                PushStatus(pfrom, STATUS_REJECTED, ERR_FEES, connman);
-                return;
-            }
-        }
-
         PoolMessage nMessageID = MSG_NOERR;
 
         entry.addr = pfrom->addr;
-        if (AddEntry(entry, nMessageID)) {
+        if (AddEntry(connman, entry, nMessageID)) {
             PushStatus(pfrom, STATUS_ACCEPTED, nMessageID, connman);
             CheckPool(connman);
             RelayStatus(STATUS_ACCEPTED, connman);
@@ -628,48 +538,62 @@ bool CPrivateSendServer::IsInputScriptSigValid(const CTxIn& txin)
 }
 
 //
-// Add a clients transaction to the pool
+// Add a client's transaction inputs/outputs to the pool
 //
-bool CPrivateSendServer::AddEntry(const CPrivateSendEntry& entryNew, PoolMessage& nMessageIDRet)
+bool CPrivateSendServer::AddEntry(CConnman& connman, const CPrivateSendEntry& entry, PoolMessage& nMessageIDRet)
 {
     if (!fMasternodeMode) return false;
 
-    for (const auto& txin : entryNew.vecTxDSIn) {
-        if (txin.prevout.IsNull()) {
-            LogPrint(BCLog::PRIVATESEND, "CPrivateSendServer::AddEntry -- input not valid!\n");
-            nMessageIDRet = ERR_INVALID_INPUT;
-            return false;
-        }
+    if (GetEntriesCount() >= nSessionMaxParticipants) {
+        LogPrint(BCLog::PRIVATESEND, "CPrivateSendServer::%s -- ERROR: entries is full!\n", __func__);
+        nMessageIDRet = ERR_ENTRIES_FULL;
+        return false;
     }
 
-    if (!CPrivateSend::IsCollateralValid(*entryNew.txCollateral)) {
-        LogPrint(BCLog::PRIVATESEND, "CPrivateSendServer::AddEntry -- collateral not valid!\n");
+    if (!CPrivateSend::IsCollateralValid(*entry.txCollateral)) {
+        LogPrint(BCLog::PRIVATESEND, "CPrivateSendServer::%s -- ERROR: collateral not valid!\n", __func__);
         nMessageIDRet = ERR_INVALID_COLLATERAL;
         return false;
     }
 
-    if (GetEntriesCount() >= nSessionMaxParticipants) {
-        LogPrint(BCLog::PRIVATESEND, "CPrivateSendServer::AddEntry -- entries is full!\n");
-        nMessageIDRet = ERR_ENTRIES_FULL;
+    if (entry.vecTxDSIn.size() > PRIVATESEND_ENTRY_MAX_SIZE) {
+        LogPrint(BCLog::PRIVATESEND, "CPrivateSendServer::%s -- ERROR: too many inputs! %d/%d\n", __func__, entry.vecTxDSIn.size(), PRIVATESEND_ENTRY_MAX_SIZE);
+        nMessageIDRet = ERR_MAXIMUM;
+        ConsumeCollateral(connman, entry.txCollateral);
         return false;
     }
 
-    for (const auto& txin : entryNew.vecTxDSIn) {
-        LogPrint(BCLog::PRIVATESEND, "looking for txin -- %s\n", txin.ToString());
+    std::vector<CTxIn> vin;
+    for (const auto& txin : entry.vecTxDSIn) {
+        LogPrint(BCLog::PRIVATESEND, "CPrivateSendServer::%s -- txin=%s\n", __func__, txin.ToString());
+
         for (const auto& entry : vecEntries) {
             for (const auto& txdsin : entry.vecTxDSIn) {
                 if (txdsin.prevout == txin.prevout) {
-                    LogPrint(BCLog::PRIVATESEND, "CPrivateSendServer::AddEntry -- found in txin\n");
+                    LogPrint(BCLog::PRIVATESEND, "CPrivateSendServer::%s -- ERROR: already have this txin in entries\n", __func__);
                     nMessageIDRet = ERR_ALREADY_HAVE;
+                    // Two peers sent the same input? Can't really say who is the malicious one here,
+                    // could be that someone is picking someone else's inputs randomly trying to force
+                    // collateral consumption. Do not punish.
                     return false;
                 }
             }
         }
+        vin.emplace_back(txin);
     }
 
-    vecEntries.push_back(entryNew);
+    bool fConsumeCollateral{false};
+    if (!IsValidInOuts(vin, entry.vecTxOut, nMessageIDRet, &fConsumeCollateral)) {
+        LogPrint(BCLog::PRIVATESEND, "CPrivateSendServer::%s -- ERROR! IsValidInOuts() failed: %s\n", __func__, CPrivateSend::GetMessageByID(nMessageIDRet));
+        if (fConsumeCollateral) {
+            ConsumeCollateral(connman, entry.txCollateral);
+        }
+        return false;
+    }
+
+    vecEntries.push_back(entry);
 
-    LogPrint(BCLog::PRIVATESEND, "CPrivateSendServer::AddEntry -- adding entry %d of %d required\n", GetEntriesCount(), nSessionMaxParticipants);
+    LogPrint(BCLog::PRIVATESEND, "CPrivateSendServer::%s -- adding entry %d of %d required\n", __func__, GetEntriesCount(), nSessionMaxParticipants);
     nMessageIDRet = MSG_ENTRIES_ADDED;
 
     return true;
@@ -724,19 +648,6 @@ bool CPrivateSendServer::IsSignaturesComplete()
     return true;
 }
 
-bool CPrivateSendServer::IsOutputsCompatibleWithSessionDenom(const std::vector<CTxOut>& vecTxOut)
-{
-    if (CPrivateSend::GetDenominations(vecTxOut) == 0) return false;
-
-    for (const auto& entry : vecEntries) {
-        LogPrint(BCLog::PRIVATESEND, "CPrivateSendServer::IsOutputsCompatibleWithSessionDenom -- vecTxOut denom %d, entry.vecTxOut denom %d\n",
-            CPrivateSend::GetDenominations(vecTxOut), CPrivateSend::GetDenominations(entry.vecTxOut));
-        if (CPrivateSend::GetDenominations(vecTxOut) != CPrivateSend::GetDenominations(entry.vecTxOut)) return false;
-    }
-
-    return true;
-}
-
 bool CPrivateSendServer::IsAcceptableDSA(const CPrivateSendAccept& dsa, PoolMessage& nMessageIDRet)
 {
     if (!fMasternodeMode) return false;

src/privatesend/privatesend-server.h

@@ -29,7 +29,7 @@ class CPrivateSendServer : public CPrivateSendBaseSession, public CPrivateSendBa
     bool fUnitTest;
 
     /// Add a clients entry to the pool
-    bool AddEntry(const CPrivateSendEntry& entryNew, PoolMessage& nMessageIDRet);
+    bool AddEntry(CConnman& connman, const CPrivateSendEntry& entry, PoolMessage& nMessageIDRet);
     /// Add signature to a txin
     bool AddScriptSig(const CTxIn& txin);
 
@@ -57,8 +57,6 @@ class CPrivateSendServer : public CPrivateSendBaseSession, public CPrivateSendBa
     bool IsSignaturesComplete();
     /// Check to make sure a given input matches an input in the pool and its scriptSig is valid
     bool IsInputScriptSigValid(const CTxIn& txin);
-    /// Are these outputs compatible with other client in the pool?
-    bool IsOutputsCompatibleWithSessionDenom(const std::vector<CTxOut>& vecTxOut);
 
     // Set the 'state' value, with some logging and capturing when the state changed
     void SetState(PoolState nStateNew);