Merge #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,