Merge PIVX-Project#2908: [Core] Fix Exchange address activation

d7d30dc Change exchange address functional tests (Duddino)
6c01283 Fix Exchange address activation (Duddino)

Pull request description:

  Change exchange address activation with the following:
  - Before 5.6 activation, accept TxOuts with OP_EXCHANGEADDR, even if they have sapling data in it
  - Before 5.6 activation, treat OP_EXCHANGEADDR as OP_UNKNOWN, thus rejecting any attempt at spending them
  - After 5.6 activation, keep things as is (Don't accept exchange address UTXOs if the tx has sapling data)

ACKs for top commit: d7d30dc
  Liquid369:
    tACK d7d30dc
  panleone:
    utACK d7d30dc

Tree-SHA512: 34f2cb7e8c2ce71c3e4fcc6fd977853e4828381e23a65b85cc8f86b95da7fe640b1d28c337d9f32df4d8d833225794a8cd1120474c1ab2c58062afc55612f3b5

src/consensus/tx_verify.cpp

@@ -116,9 +116,6 @@ bool CheckTransaction(const CTransaction& tx, CValidationState& state, bool fCol
     }
 
     bool hasExchangeUTXOs = tx.HasExchangeAddr();
-    int nTxHeight = chainActive.Height();
-    if (hasExchangeUTXOs && !Params().GetConsensus().NetworkUpgradeActive(nTxHeight, Consensus::UPGRADE_V5_6))
-        return state.DoS(100, false, REJECT_INVALID, "bad-exchange-address-not-started");
 
     if (tx.IsCoinBase()) {
         if (tx.vin[0].scriptSig.size() < 2 || tx.vin[0].scriptSig.size() > 150)

src/policy/policy.h

@@ -45,7 +45,8 @@ static constexpr unsigned int STANDARD_SCRIPT_VERIFY_FLAGS = MANDATORY_SCRIPT_VE
                                                          SCRIPT_VERIFY_NULLDUMMY |
                                                          SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS |
                                                          SCRIPT_VERIFY_CLEANSTACK |
-                                                         SCRIPT_VERIFY_LOW_S;
+                                                         SCRIPT_VERIFY_LOW_S |
+                                                         SCRIPT_VERIFY_EXCHANGEADDR;
 
 /** For convenience, standard but not mandatory verify flags. */
 static constexpr unsigned int STANDARD_NOT_MANDATORY_VERIFY_FLAGS = STANDARD_SCRIPT_VERIFY_FLAGS & ~MANDATORY_SCRIPT_VERIFY_FLAGS;

src/sapling/sapling_validation.cpp

@@ -33,8 +33,8 @@ bool CheckTransaction(const CTransaction& tx, CValidationState& state, CAmount&
 
     // From here, all of the checks are done in v3+ transactions.
 
-    // if the tx has shielded data, cannot be a coinstake, coinbase, zcspend and zcmint or exchange address
-    if (tx.IsCoinStake() || tx.IsCoinBase() || tx.HasZerocoinSpendInputs() || tx.HasZerocoinMintOutputs() || tx.HasExchangeAddr())
+    // if the tx has shielded data, cannot be a coinstake, coinbase, zcspend and zcmint
+    if (tx.IsCoinStake() || tx.IsCoinBase() || tx.HasZerocoinSpendInputs() || tx.HasZerocoinMintOutputs())
         return state.DoS(100, error("%s: Sapling version with invalid data", __func__),
                          REJECT_INVALID, "bad-txns-invalid-sapling");
 
@@ -160,6 +160,12 @@ bool ContextualCheckTransaction(
 
     if (hasShieldedData) {
         uint256 dataToBeSigned;
+
+        if (tx.HasExchangeAddr() && Params().GetConsensus().NetworkUpgradeActive(nHeight, Consensus::UPGRADE_V5_6)) {
+            return state.DoS(100, error("%s: Sapling version with invalid data", __func__),
+                REJECT_INVALID, "bad-txns-exchange-addr-has-sapling");
+        }
+
         // Empty output script.
         CScript scriptCode;
         try {

src/script/interpreter.cpp

@@ -334,6 +334,10 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
                     break;
 
                 case OP_EXCHANGEADDR:
+                    // Not enabled, treat as OP_UNKNOWN
+                    if (!(flags & SCRIPT_VERIFY_EXCHANGEADDR)) {
+                        return set_error(serror, SCRIPT_ERR_BAD_OPCODE);
+                    }
                     if (!script.IsPayToExchangeAddress())
                         return set_error(serror, SCRIPT_ERR_EXCHANGEADDRVERIFY);
                     break;

src/script/interpreter.h

@@ -80,7 +80,11 @@ enum
     // Verify CHECKLOCKTIMEVERIFY
     //
     // See BIP65 for details.
-    SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY = (1U << 9)
+    SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY = (1U << 9),
+
+    // Verify OP_EXCHANGEADDR.
+    // Treat as UNKNOWN before 5.6 activation
+    SCRIPT_VERIFY_EXCHANGEADDR = (1U << 10),
 };
 
 bool CheckSignatureEncoding(const std::vector<unsigned char> &vchSig, unsigned int flags, ScriptError* serror);

src/test/transaction_tests.cpp

@@ -47,6 +47,7 @@ static std::map<std::string, unsigned int> mapFlagNames = {
     {std::string("DISCOURAGE_UPGRADABLE_NOPS"), (unsigned int)SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS},
     {std::string("CLEANSTACK"), (unsigned int)SCRIPT_VERIFY_CLEANSTACK},
     {std::string("CHECKLOCKTIMEVERIFY"), (unsigned int)SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY},
+    {std::string("EXCHANGEADDRVERIFY"), (unsigned int)SCRIPT_VERIFY_EXCHANGEADDR},
 };
 
 unsigned int ParseScriptFlags(std::string strFlags)

src/validation.cpp

@@ -569,12 +569,15 @@ static bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState &state,
         }
 
         bool fCLTVIsActivated = consensus.NetworkUpgradeActive(chainHeight, Consensus::UPGRADE_BIP65);
-
+        bool exchangeAddrActivated = consensus.NetworkUpgradeActive(chainHeight, Consensus::UPGRADE_V5_6);
         // Check against previous transactions
         // This is done last to help prevent CPU exhaustion denial-of-service attacks.
         int flags = STANDARD_SCRIPT_VERIFY_FLAGS;
         if (fCLTVIsActivated)
             flags |= SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY;
+        if (exchangeAddrActivated)
+            flags |= SCRIPT_VERIFY_EXCHANGEADDR;
+
 
         PrecomputedTransactionData precomTxData(tx);
         if (!CheckInputs(tx, state, view, true, flags, true, precomTxData)) {
@@ -593,6 +596,8 @@ static bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState &state,
         flags = MANDATORY_SCRIPT_VERIFY_FLAGS;
         if (fCLTVIsActivated)
             flags |= SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY;
+        if (exchangeAddrActivated)
+            flags |= SCRIPT_VERIFY_EXCHANGEADDR;
         if (!CheckInputs(tx, state, view, true, flags, true, precomTxData)) {
             return error("%s: BUG! PLEASE REPORT THIS! ConnectInputs failed against MANDATORY but not STANDARD flags %s, %s",
                     __func__, hash.ToString(), FormatStateMessage(state));
@@ -1496,8 +1501,10 @@ static bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockInd
 
     // If scripts won't be checked anyways, don't bother seeing if CLTV is activated
     bool fCLTVIsActivated = false;
+    bool exchangeAddrActivated = false;
     if (fScriptChecks && pindex->pprev) {
         fCLTVIsActivated = consensus.NetworkUpgradeActive(pindex->pprev->nHeight, Consensus::UPGRADE_BIP65);
+        exchangeAddrActivated = consensus.NetworkUpgradeActive(pindex->pprev->nHeight, Consensus::UPGRADE_V5_6);
     }
 
     CCheckQueueControl<CScriptCheck> control(fScriptChecks && nScriptCheckThreads ? &scriptcheckqueue : nullptr);
@@ -1575,6 +1582,8 @@ static bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockInd
             unsigned int flags = SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_DERSIG;
             if (fCLTVIsActivated)
                 flags |= SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY;
+            if (exchangeAddrActivated)
+                flags |= SCRIPT_VERIFY_EXCHANGEADDR;
 
             bool fCacheResults = fJustCheck; /* Don't cache results if we're actually connecting blocks (still consult the cache, though) */
             if (!CheckInputs(tx, state, view, fScriptChecks, flags, fCacheResults, precomTxData[i], nScriptCheckThreads ? &vChecks : nullptr))

test/functional/feature_exchangeaddr.py

@@ -44,7 +44,6 @@ def run_test(self):
         # Sign the transaction
         signed_tx = self.nodes[0].signrawtransaction(ToHex(tx))
 
-        # Send the raw transaction
         # Before the upgrade, this should fail if OP_EXCHANGEADDR is disallowed
         error_code = -26
         error_message = "scriptpubkey"
@@ -56,18 +55,22 @@ def run_test(self):
         # Attempt to send funds from transparent address to exchange address
         ex_addr_validation_result = self.nodes[0].validateaddress(ex_addr)
         assert_equal(ex_addr_validation_result['isvalid'], True)
-        # This should fail to be sent
-        error_code = -4
-        error_message = "bad-exchange-address-not-started"
-        assert_raises_rpc_error(
-            error_code,
-            error_message,
-            self.nodes[0].sendtoaddress,
-            ex_addr, 1.0
-        )
+        # This should succeed even before the upgrade
+        self.nodes[0].sendtoaddress(ex_addr, 1.0)
 
+        # Check wallet version
+        wallet_info = self.nodes[0].getwalletinfo()
+        if wallet_info['walletversion'] >= FEATURE_PRE_SPLIT_KEYPOOL:
+            sapling_addr = self.nodes[0].getnewshieldaddress()
+            self.nodes[0].sendtoaddress(sapling_addr, 2.0)
+            self.nodes[0].generate(1)
+            sap_to_ex = [{"address": ex_addr, "amount": Decimal('1')}]
+            # Shield data should be allowed before activation
+            self.nodes[0].shieldsendmany(sapling_addr, sap_to_ex)
+        else:
+            self.nodes[0].generate(1)
         # Mine and activate exchange addresses
-        self.nodes[0].generate(194)
+        self.nodes[0].generate(193)
         assert_equal(self.nodes[0].getblockcount(), 1000)
         self.nodes[0].generate(1)
 
@@ -83,8 +86,10 @@ def run_test(self):
 
         # Verify balance
         node_bal = self.nodes[1].getbalance()
-        assert_equal(node_bal, 2)
-
+        if wallet_info['walletversion'] >= FEATURE_PRE_SPLIT_KEYPOOL:
+            assert_equal(node_bal, 4)
+        else:
+            assert_equal(node_bal, 3)
         # Attempt to send funds from exchange address back to transparent address
         tx2 = self.nodes[0].sendtoaddress(t_addr_2, 1.0)
         self.nodes[0].generate(6)
@@ -96,7 +101,6 @@ def run_test(self):
 
         # Transparent to Shield to Exchange should fail
         # Check wallet version
-        wallet_info = self.nodes[0].getwalletinfo()
         if wallet_info['walletversion'] < FEATURE_PRE_SPLIT_KEYPOOL:
             self.log.info("Pre-HD wallet version detected. Skipping Shield tests.")
             return
@@ -107,7 +111,7 @@ def run_test(self):
 
         # Expect shieldsendmany to fail with bad-txns-invalid-sapling
         expected_error_code = -4
-        expected_error_message = "Failed to accept tx in the memory pool (reason: bad-txns-invalid-sapling)"
+        expected_error_message = "Failed to accept tx in the memory pool (reason: bad-txns-exchange-addr-has-sapling)"
         assert_raises_rpc_error(
             expected_error_code,
             expected_error_message,