Merge #2275: [Consensus] New cold-staking opcode

4a5baf3 [Doc] Add cold-staking changes to release notes (random-zebra)
c19416f [GUI] Use new opcode for P2CS after v6 enforcement (random-zebra)
45ebfee [RPC] Use new opcode for P2CS after v6 enforcement (random-zebra)
8eefab5 [Tests] Add script test for new opcode (random-zebra)
df11631 [Script] Introduce new OP_CHECKCOLDSTAKEVERIFY opcode (random-zebra)
b194386 [Refactor] Rename CCSV opcode to OP_CHECKCOLDSTAKEVERIFY_LOF (random-zebra)

Pull request description:

  Extracted from #2267, and rebased on top of #2258.
  Given the consensus change, introduced in #2274, we can define a more secure `OP_CHECKCOLDSTAKEVERIFY` opcode, which doesn't leave the last output of the coinstake "free" (as we no longer pay masternode/budgets in the coinstake tx).

  Built on top of:
  - [x] #2258
  - [x] #2274

ACKs for top commit:
  Fuzzbawls:
    re-Code ACK 4a5baf3 after rebase, no code changes.
  furszy:
    ACK 4a5baf3.

Tree-SHA512: ee78b76137e40df05b854f8959755f1b99e7c7328fb13708ba7e3e6dae6357450210ce19ed7a99dc54aa0d6051d0dbd745af70f4b6e9927de5d3cbb2e04fffb6

doc/release-notes.md

@@ -44,6 +44,19 @@ Notable Changes
 
 (Developers: add your notes here as part of your pull requests whenever possible)
 
+Cold-Staking Re-Activation
+--------------------------
+PIVX Core v6.0.0 includes a fix for the vulnerability identified within the cold-staking protocol (see PR [#2258](https://github.com/PIVX-Project/PIVX/pull/2258)).
+Therefore the feature will be re-enabled on the network, via `SPORK_19`, shortly after the upgrade enforcement.
+
+#### Protocol changes
+
+A new opcode (`0xd2`) is introduced (see PR [#2275](https://github.com/PIVX-Project/PIVX/pull/2275)). It enforces the same rules as the legacy cold-staking opcode, but without allowing a "free" script for the last output of the transaction.
+This is in accord with the consensus change introduced with the "Deterministic Masternodes" update, as masternode/budget payments are now outputs of the *coinbase* transaction (rather than the *coinstake*), therefore a "free" output for the coinstake is no longer needed.
+The new opcode takes the name of `OP_CHECKCOLDSTAKEVERIFY`, and the legacy opcode (`0xd1`) is renamed to `OP_CHECKCOLDSTAKEVERIFY_LOF` (last-output-free).
+Scripts with the old opcode are still accepted on the network (the restriction on the last-output is enforced after the script validation in this case), but the client creates new delegations with the new opcode, by default, after the upgrade enforcement.
+
+
 GUI changes
 -----------
 

src/qt/walletmodel.cpp

@@ -90,6 +90,11 @@ bool WalletModel::isSaplingEnforced() const
     return Params().GetConsensus().NetworkUpgradeActive(cachedNumBlocks, Consensus::UPGRADE_V5_0);
 }
 
+bool WalletModel::isV6Enforced() const
+{
+    return Params().GetConsensus().NetworkUpgradeActive(cachedNumBlocks, Consensus::UPGRADE_V6_0);
+}
+
 bool WalletModel::isStakingStatusActive() const
 {
     return wallet && wallet->pStakerStatus && wallet->pStakerStatus->IsActive();
@@ -480,7 +485,8 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact
                     return InvalidAddress;
                 }
 
-                scriptPubKey = GetScriptForStakeDelegation(*stakerId, *ownerId);
+                scriptPubKey = isV6Enforced() ? GetScriptForStakeDelegation(*stakerId, *ownerId)
+                                              : GetScriptForStakeDelegationLOF(*stakerId, *ownerId);
             } else {
                 // Regular P2PK or P2PKH
                 scriptPubKey = GetScriptForDestination(out);

src/qt/walletmodel.h

@@ -156,6 +156,7 @@ class WalletModel : public QObject
     bool isColdStakingNetworkelyEnabled() const;
     bool isSaplingInMaintenance() const;
     bool isSaplingEnforced() const;
+    bool isV6Enforced() const;
     CAmount getMinColdStakingAmount() const;
     /* current staking status from the miner thread **/
     bool isStakingStatusActive() const;

src/sapling/sapling_operation.h

@@ -51,8 +51,9 @@ struct SendManyRecipient
     {}
 
     // Transparent recipient: P2CS
-    SendManyRecipient(const CKeyID& ownerKey, const CKeyID& stakerKey, const CAmount& amount):
-        transparentRecipient(CTxOut(amount, GetScriptForStakeDelegation(stakerKey, ownerKey)))
+    SendManyRecipient(const CKeyID& ownerKey, const CKeyID& stakerKey, const CAmount& amount, bool fV6Enforced):
+        transparentRecipient(CTxOut(amount, fV6Enforced ? GetScriptForStakeDelegation(stakerKey, ownerKey)
+                                                        : GetScriptForStakeDelegationLOF(stakerKey, ownerKey)))
     {}
 
     // Transparent recipient: multisig

src/script/interpreter.cpp

@@ -962,28 +962,14 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
 
                 case OP_CHECKCOLDSTAKEVERIFY:
                 {
-                    if (g_IsV6Active) {
-                        // the stack can contain only <sig> <pk> <pkh> at this point
-                        if ((int)stack.size() != 3) {
-                            return set_error(serror, SCRIPT_ERR_INVALID_STACK_OPERATION);
-                        }
-                        // check pubkey/signature encoding
-                        valtype& vchSig    = stacktop(-3);
-                        valtype& vchPubKey = stacktop(-2);
-                        if (!CheckSignatureEncoding(vchSig, flags, serror) ||
-                                !CheckPubKeyEncoding(vchPubKey, flags, serror)) {
-                            // serror is set
-                            return false;
-                        }
-                        // check hash size
-                        valtype& vchPubKeyHash = stacktop(-1);
-                        if ((int)vchPubKeyHash.size() != 20) {
-                            return set_error(serror, SCRIPT_ERR_SCRIPT_SIZE);
-                        }
-                    }
-                    if(!checker.CheckColdStake(script)) {
-                        return set_error(serror, SCRIPT_ERR_CHECKCOLDSTAKEVERIFY);
-                    }
+                    return checker.CheckColdStake(false, script, stack, flags, serror);
+                }
+                break;
+
+                case OP_CHECKCOLDSTAKEVERIFY_LOF:
+                {
+                    // Allow last output script "free"
+                    return checker.CheckColdStake(true, script, stack, flags, serror);
                 }
                 break;
 
@@ -1357,36 +1343,59 @@ bool TransactionSignatureChecker::CheckLockTime(const CScriptNum& nLockTime) con
     return true;
 }
 
-bool TransactionSignatureChecker::CheckColdStake(const CScript& prevoutScript) const
+bool TransactionSignatureChecker::CheckColdStake(bool fAllowLastOutputFree, const CScript& prevoutScript, std::vector<valtype>& stack, unsigned int flags, ScriptError* serror) const
 {
+    if (g_IsV6Active) {
+        // the stack can contain only <sig> <pk> <pkh> at this point
+        if ((int)stack.size() != 3) {
+            return set_error(serror, SCRIPT_ERR_INVALID_STACK_OPERATION);
+        }
+        // check pubkey/signature encoding
+        valtype& vchSig    = stacktop(-3);
+        valtype& vchPubKey = stacktop(-2);
+        if (!CheckSignatureEncoding(vchSig, flags, serror) ||
+                !CheckPubKeyEncoding(vchPubKey, flags, serror)) {
+            // serror is set
+            return false;
+        }
+        // check hash size
+        valtype& vchPubKeyHash = stacktop(-1);
+        if ((int)vchPubKeyHash.size() != 20) {
+            return set_error(serror, SCRIPT_ERR_SCRIPT_SIZE);
+        }
+    }
+
+    // check it is used in a valid cold stake transaction.
     // Transaction must be a coinstake tx
     if (!txTo->IsCoinStake()) {
-        return false;
+        return set_error(serror, SCRIPT_ERR_CHECKCOLDSTAKEVERIFY);
     }
     // There must be one single input
     if (txTo->vin.size() != 1) {
-        return false;
+        return set_error(serror, SCRIPT_ERR_CHECKCOLDSTAKEVERIFY);
     }
     // Since this is a coinstake, it has at least 2 outputs
     const unsigned int outs = txTo->vout.size();
     assert(outs >= 2);
-    // All outputs, except the first, and (for cold stakes with outs >=3) the last one,
-    // must have the same pubKeyScript, and it must match the script we are spending.
-    // If the coinstake has at least 3 outputs, the last one is left free, to be used for
-    // budget/masternode payments, and is checked in CheckColdstakeFreeOutput().
+    // All outputs must have the same pubKeyScript, and it must match the script we are spending.
+    // If the coinstake has at least 3 outputs, the last one can be left free, to be used for
+    // budget/masternode payments (before v6.0 enforcement), and is checked in CheckColdstakeFreeOutput().
     // Here we verify only that input amount goes to the non-free outputs.
     CAmount outValue{0};
     for (unsigned int i = 1; i < outs; i++) {
         if (txTo->vout[i].scriptPubKey != prevoutScript) {
-            // Only the last one can be different (and only when outs >=3)
-            if (i != outs-1 || outs < 3) {
-                return false;
+            // Only the last one can be different (and only when outs >=3 and fAllowLastOutputFree=true)
+            if (!fAllowLastOutputFree || i != outs-1 || outs < 3) {
+                return set_error(serror, SCRIPT_ERR_CHECKCOLDSTAKEVERIFY);
             }
         } else {
             outValue += txTo->vout[i].nValue;
         }
     }
-    return outValue > amount;
+    if (outValue < amount) {
+        return set_error(serror, SCRIPT_ERR_CHECKCOLDSTAKEVERIFY);
+    }
+    return true;
 }
 
 

src/script/interpreter.h

@@ -107,7 +107,7 @@ class BaseSignatureChecker
          return false;
     }
 
-    virtual bool CheckColdStake(const CScript& script) const
+    virtual bool CheckColdStake(bool fAllowLastOutputFree, const CScript& prevoutScript, std::vector<valtype>& stack, unsigned int flags, ScriptError* error) const
     {
          return false;
     }
@@ -132,7 +132,7 @@ class TransactionSignatureChecker : public BaseSignatureChecker
 
     bool CheckSig(const std::vector<unsigned char>& scriptSig, const std::vector<unsigned char>& vchPubKey, const CScript& scriptCode, SigVersion sigversion) const override ;
     bool CheckLockTime(const CScriptNum& nLockTime) const override;
-    bool CheckColdStake(const CScript& prevoutScript) const override;
+    bool CheckColdStake(bool fAllowLastOutputFree, const CScript& prevoutScript, std::vector<valtype>& stack, unsigned int flags, ScriptError* serror) const override;
 };
 
 class MutableTransactionSignatureChecker : public TransactionSignatureChecker

src/script/script.cpp

@@ -148,7 +148,8 @@ const char* GetOpName(opcodetype opcode)
     case OP_ZEROCOINPUBLICSPEND    : return "OP_ZEROCOINPUBLICSPEND";
 
     // cold staking
-    case OP_CHECKCOLDSTAKEVERIFY   : return "OP_CHECKCOLDSTAKEVERIFY";
+    case OP_CHECKCOLDSTAKEVERIFY_LOF   : return "OP_CHECKCOLDSTAKEVERIFY_LOF";
+    case OP_CHECKCOLDSTAKEVERIFY       : return "OP_CHECKCOLDSTAKEVERIFY";
 
     case OP_INVALIDOPCODE          : return "OP_INVALIDOPCODE";
 
@@ -229,14 +230,15 @@ bool CScript::IsPayToScriptHash() const
 // can be removed once v6 enforcement is activated.
 std::atomic<bool> g_IsV6Active{false};
 
+// P2CS script: either with or without last output free
 bool CScript::IsPayToColdStaking() const
 {
     return (this->size() == 51 &&
             (!g_IsV6Active || (*this)[0] == OP_DUP) &&
             (!g_IsV6Active || (*this)[1] == OP_HASH160) &&
             (*this)[2] == OP_ROT &&
             (!g_IsV6Active || (*this)[3] == OP_IF) &&
-            (*this)[4] == OP_CHECKCOLDSTAKEVERIFY &&
+            ((*this)[4] == OP_CHECKCOLDSTAKEVERIFY || (*this)[4] == OP_CHECKCOLDSTAKEVERIFY_LOF) &&
             (*this)[5] == 0x14 &&
             (!g_IsV6Active || (*this)[26] == OP_ELSE) &&
             (*this)[27] == 0x14 &&
@@ -245,6 +247,11 @@ bool CScript::IsPayToColdStaking() const
             (*this)[50] == OP_CHECKSIG);
 }
 
+bool CScript::IsPayToColdStakingLOF() const
+{
+    return IsPayToColdStaking() && (*this)[4] == OP_CHECKCOLDSTAKEVERIFY_LOF;
+}
+
 bool CScript::StartsWithOpcode(const opcodetype opcode) const
 {
     return (!this->empty() && (*this)[0] == opcode);

src/script/script.h

@@ -184,7 +184,8 @@ enum opcodetype
     OP_ZEROCOINPUBLICSPEND = 0xc3,
 
     // cold staking
-    OP_CHECKCOLDSTAKEVERIFY = 0xd1,
+    OP_CHECKCOLDSTAKEVERIFY_LOF = 0xd1,     // last output free for masternode/budget payments
+    OP_CHECKCOLDSTAKEVERIFY = 0xd2,
 
     OP_INVALIDOPCODE = 0xff,
 };
@@ -632,6 +633,7 @@ class CScript : public CScriptBase
     bool IsPayToPublicKeyHash() const;
     bool IsPayToScriptHash() const;
     bool IsPayToColdStaking() const;
+    bool IsPayToColdStakingLOF() const;
     bool StartsWithOpcode(const opcodetype opcode) const;
     bool IsZerocoinMint() const;
     bool IsZerocoinSpend() const;

src/script/standard.cpp

@@ -305,6 +305,16 @@ CScript GetScriptForStakeDelegation(const CKeyID& stakingKey, const CKeyID& spen
     return script;
 }
 
+CScript GetScriptForStakeDelegationLOF(const CKeyID& stakingKey, const CKeyID& spendingKey)
+{
+    CScript script;
+    script << OP_DUP << OP_HASH160 << OP_ROT <<
+            OP_IF << OP_CHECKCOLDSTAKEVERIFY_LOF << ToByteVector(stakingKey) <<
+            OP_ELSE << ToByteVector(spendingKey) << OP_ENDIF <<
+            OP_EQUALVERIFY << OP_CHECKSIG;
+    return script;
+}
+
 CScript GetScriptForMultisig(int nRequired, const std::vector<CPubKey>& keys)
 {
     CScript script;

src/script/standard.h

@@ -84,6 +84,7 @@ CScript GetScriptForDestination(const CTxDestination& dest);
 CScript GetScriptForRawPubKey(const CPubKey& pubKey);
 CScript GetScriptForMultisig(int nRequired, const std::vector<CPubKey>& keys);
 CScript GetScriptForStakeDelegation(const CKeyID& stakingKey, const CKeyID& spendingKey);
+CScript GetScriptForStakeDelegationLOF(const CKeyID& stakingKey, const CKeyID& spendingKey);
 CScript GetScriptForOpReturn(const uint256& message);
 
 #endif // BITCOIN_SCRIPT_STANDARD_H