Merge bitcoin#1970: [Validation][RPC] Delegate from shielded addresses

467b0d8 [Tests] Check delegations with sapling transactions (random-zebra)
d412525 [RPC] Add fUseShielded parameter to delegatestake/rawdelegatestake (random-zebra)
847d278 [Trivial] Missing newline in delegatestake (random-zebra)
7440709 [Validation] Don't reject sapling txes with P2CS outputs (random-zebra)

Pull request description:

  Enable pay-to-cold-staking outputs in shielded transactions.
  Add the option in `delegatestake`/`rawdelegatestake` to spend shielded funds directly to P2CS (without having to unshield them first).

  Add functional test.

  Based on top of
  - [x] bitcoin#1964
  - [x] bitcoin#1967
  - [x] bitcoin#1969

  Starts with `[Validation] Don't reject sapling txes with P2CS outputs` (461d4ea)

ACKs for top commit:
  furszy:
    Looking good, ACK 467b0d8
  Fuzzbawls:
    ACK 467b0d8

Tree-SHA512: 88752fe1550def605b90cc894d0666f5c37dd8f075566e976c8fbfaa6e088ee0c1bb028e1885411b739781fda6ebac12a5c99fc19a06f453e594441ee1e1b599

src/sapling/sapling_validation.cpp

@@ -34,9 +34,8 @@ bool CheckTransaction(const CTransaction& tx, CValidationState& state, CAmount&
 
     // From here, all of the checks are done in +v2 transactions.
 
-    // if the tx has shielded data, cannot be a coinstake, coinbase, zcspend, p2csOut and zcmint
-    if (tx.IsCoinStake() || tx.IsCoinBase() || tx.HasZerocoinSpendInputs() || tx.HasP2CSOutputs() ||
-        tx.HasZerocoinMintOutputs())
+    // 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");
 

src/wallet/rpcwallet.cpp

@@ -827,11 +827,7 @@ UniValue listshieldedaddresses(const JSONRPCRequest& request)
     libzcash::SaplingIncomingViewingKey ivk;
     libzcash::SaplingExtendedFullViewingKey extfvk;
     for (libzcash::SaplingPaymentAddress addr : addresses) {
-        if (fIncludeWatchonly || (
-                pwalletMain->GetSaplingIncomingViewingKey(addr, ivk) &&
-                pwalletMain->GetSaplingFullViewingKey(ivk, extfvk) &&
-                pwalletMain->HaveSaplingSpendingKey(extfvk)
-        )) {
+        if (fIncludeWatchonly || pwalletMain->HaveSpendingKeyForPaymentAddress(addr)) {
             ret.push_back(KeyIO::EncodePaymentAddress(addr));
         }
     }
@@ -989,8 +985,8 @@ UniValue CreateColdStakeDelegation(const UniValue& params, CWalletTx& wtxNew, CR
 
     // Check that Cold Staking has been enforced or fForceNotEnabled = true
     bool fForceNotEnabled = false;
-    if (params.size() > 5 && !params[5].isNull())
-        fForceNotEnabled = params[5].get_bool();
+    if (params.size() > 6 && !params[6].isNull())
+        fForceNotEnabled = params[6].get_bool();
 
     if (!sporkManager.IsSporkActive(SPORK_17_COLDSTAKING_ENFORCEMENT) && !fForceNotEnabled) {
         std::string errMsg = "Cold Staking disabled with SPORK 17.\n"
@@ -1059,16 +1055,36 @@ UniValue CreateColdStakeDelegation(const UniValue& params, CWalletTx& wtxNew, CR
         ownerAddressStr = EncodeDestination(ownerAddr);
     }
 
-    // Get P2CS script for addresses
-    CScript scriptPubKey = GetScriptForStakeDelegation(*stakeKey, ownerKey);
-
     // Create the transaction
-    CAmount nFeeRequired;
-    if (!pwalletMain->CreateTransaction(scriptPubKey, nValue, wtxNew, reservekey, nFeeRequired, strError, nullptr, ALL_COINS, (CAmount)0, fUseDelegated)) {
-        if (nValue + nFeeRequired > currBalance)
-            strError = strprintf("Error: This transaction requires a transaction fee of at least %s because of its amount, complexity, or use of recently received funds!", FormatMoney(nFeeRequired));
-        LogPrintf("%s : %s\n", __func__, strError);
-        throw JSONRPCError(RPC_WALLET_ERROR, strError);
+    const bool fUseShielded = (params.size() > 5) && params[5].get_bool();
+    if (!fUseShielded) {
+        // Delegate transparent coins
+        CAmount nFeeRequired;
+        CScript scriptPubKey = GetScriptForStakeDelegation(*stakeKey, ownerKey);
+        if (!pwalletMain->CreateTransaction(scriptPubKey, nValue, wtxNew, reservekey, nFeeRequired, strError, nullptr, ALL_COINS, (CAmount)0, fUseDelegated)) {
+            if (nValue + nFeeRequired > currBalance)
+                strError = strprintf("Error: This transaction requires a transaction fee of at least %s because of its amount, complexity, or use of recently received funds!", FormatMoney(nFeeRequired));
+            LogPrintf("%s : %s\n", __func__, strError);
+            throw JSONRPCError(RPC_WALLET_ERROR, strError);
+        }
+    } else {
+        // Delegate shielded coins
+        const Consensus::Params& consensus = Params().GetConsensus();
+        // Check network status
+        int nextBlockHeight = chainActive.Height() + 1;
+        if (!consensus.NetworkUpgradeActive(nextBlockHeight, Consensus::UPGRADE_V5_DUMMY)) {
+            throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, Sapling not active yet");
+        }
+        CAmount nFee = DEFAULT_SAPLING_FEE;
+        std::vector<SendManyRecipient> recipients = {SendManyRecipient(ownerKey, *stakeKey, nValue)};
+        TransactionBuilder txBuilder = TransactionBuilder(consensus, nextBlockHeight, pwalletMain);
+        SaplingOperation operation(txBuilder);
+        OperationResult res = operation.setSelectShieldedCoins(true)
+                                       ->setRecipients(recipients)
+                                       ->setFee(nFee)
+                                       ->build();
+        if (!res) throw JSONRPCError(RPC_WALLET_ERROR, res.getError());
+        wtxNew = CWalletTx(pwalletMain, operation.getFinalTx());
     }
 
     UniValue result(UniValue::VOBJ);
@@ -1079,7 +1095,7 @@ UniValue CreateColdStakeDelegation(const UniValue& params, CWalletTx& wtxNew, CR
 
 UniValue delegatestake(const JSONRPCRequest& request)
 {
-    if (request.fHelp || request.params.size() < 2 || request.params.size() > 6)
+    if (request.fHelp || request.params.size() < 2 || request.params.size() > 7)
         throw std::runtime_error(
             "delegatestake \"stakingaddress\" amount ( \"owneraddress\" fExternalOwner fUseDelegated fForceNotEnabled )\n"
             "\nDelegate an amount to a given address for cold staking. The amount is a real and is rounded to the nearest 0.00000001\n" +
@@ -1088,12 +1104,13 @@ UniValue delegatestake(const JSONRPCRequest& request)
             "\nArguments:\n"
             "1. \"stakingaddress\"      (string, required) The pivx staking address to delegate.\n"
             "2. \"amount\"              (numeric, required) The amount in PIV to delegate for staking. eg 100\n"
-            "3. \"owneraddress\"        (string, optional) The pivx address corresponding to the key that will be able to spend the stake. \n"
+            "3. \"owneraddress\"        (string, optional) The pivx address corresponding to the key that will be able to spend the stake.\n"
             "                               If not provided, or empty string, a new wallet address is generated.\n"
             "4. \"fExternalOwner\"      (boolean, optional, default = false) use the provided 'owneraddress' anyway, even if not present in this wallet.\n"
             "                               WARNING: The owner of the keys to 'owneraddress' will be the only one allowed to spend these coins.\n"
-            "5. \"fUseDelegated\"       (boolean, optional, default = false) include already delegated inputs if needed."
-            "6. \"fForceNotEnabled\"    (boolean, optional, default = false) force the creation even if SPORK 17 is disabled (for tests)."
+            "5. \"fUseDelegated\"       (boolean, optional, default = false) include already delegated inputs if needed.\n"
+            "6. \"fFromShielded\"       (boolean, optional, default = false) delegate shielded funds.\n"
+            "7. \"fForceNotEnabled\"    (boolean, optional, default = false) ONLY FOR TESTING: force the creation even if SPORK 17 is disabled.\n"
 
             "\nResult:\n"
             "{\n"
@@ -1123,7 +1140,7 @@ UniValue delegatestake(const JSONRPCRequest& request)
 
 UniValue rawdelegatestake(const JSONRPCRequest& request)
 {
-    if (request.fHelp || request.params.size() < 2 || request.params.size() > 5)
+    if (request.fHelp || request.params.size() < 2 || request.params.size() > 7)
         throw std::runtime_error(
             "rawdelegatestake \"stakingaddress\" amount ( \"owneraddress\" fExternalOwner fUseDelegated )\n"
             "\nDelegate an amount to a given address for cold staking. The amount is a real and is rounded to the nearest 0.00000001\n"
@@ -1133,11 +1150,13 @@ UniValue rawdelegatestake(const JSONRPCRequest& request)
             "\nArguments:\n"
             "1. \"stakingaddress\"      (string, required) The pivx staking address to delegate.\n"
             "2. \"amount\"              (numeric, required) The amount in PIV to delegate for staking. eg 100\n"
-            "3. \"owneraddress\"        (string, optional) The pivx address corresponding to the key that will be able to spend the stake. \n"
+            "3. \"owneraddress\"        (string, optional) The pivx address corresponding to the key that will be able to spend the stake.\n"
             "                               If not provided, or empty string, a new wallet address is generated.\n"
             "4. \"fExternalOwner\"      (boolean, optional, default = false) use the provided 'owneraddress' anyway, even if not present in this wallet.\n"
             "                               WARNING: The owner of the keys to 'owneraddress' will be the only one allowed to spend these coins.\n"
-            "5. \"fUseDelegated         (boolean, optional, default = false) include already delegated inputs if needed."
+            "5. \"fUseDelegated\"       (boolean, optional, default = false) include already delegated inputs if needed.\n"
+            "6. \"fFromShielded\"       (boolean, optional, default = false) delegate shielded funds.\n"
+            "7. \"fForceNotEnabled\"    (boolean, optional, default = false) ONLY FOR TESTING: force the creation even if SPORK 17 is disabled (for tests).\n"
 
             "\nResult:\n"
             "{\n"
@@ -1527,7 +1546,7 @@ static SaplingOperation CreateShieldedTransaction(const JSONRPCRequest& request)
     if (!Params().GetConsensus().NetworkUpgradeActive(nextBlockHeight, Consensus::UPGRADE_V5_DUMMY)) {
         // If Sapling is not active, do not allow sending from or sending to Sapling addresses.
         if (fromSapling || containsSaplingOutput) {
-            throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, Sapling has not activated");
+            throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, Sapling not active yet");
         }
     }
 

test/functional/mining_pos_coldStaking.py

@@ -23,6 +23,8 @@
     sync_mempools,
 )
 
+from decimal import Decimal
+
 # filter utxos based on first 5 bytes of scriptPubKey
 def getDelegatedUtxos(utxos):
     return [x for x in utxos if x["scriptPubKey"][:10] == '76a97b63d1']
@@ -32,7 +34,7 @@ class PIVX_ColdStakingTest(PivxTestFramework):
 
     def set_test_params(self):
         self.num_nodes = 3
-        self.extra_args = [[]] * self.num_nodes
+        self.extra_args = [['-nuparams=v5_dummy:201']] * self.num_nodes
         self.extra_args[0].append('-sporkkey=932HEevBSujW2ud7RfB1YF91AFygbBRQj3de3LyaCRqNzKKgWXi')
 
     def setup_chain(self):
@@ -98,6 +100,7 @@ def run_test(self):
 
         # 2) node[1] sends his entire balance (50 mature rewards) to node[2]
         #  - node[2] stakes a block - node[1] locks the change
+        #  - node[0] shields 250 coins (to be delegated later)
         print("*** 2 ***")
         self.log.info("Emptying node1 balance")
         assert_equal(self.nodes[1].getbalance(), 50 * 250)
@@ -112,6 +115,15 @@ def run_test(self):
         # check that it cannot stake
         sleep(1)
         assert_equal(self.nodes[1].getstakingstatus()["stakeablecoins"], 0)
+        # create shielded balance for node 0
+        self.log.info("Shielding some coins for node0...")
+        self.nodes[0].shielded_sendmany("from_transparent", [{"address": self.nodes[0].getnewshieldedaddress(),
+                                                             "amount": Decimal('250.00')}], 1, 1)
+        self.sync_all()
+        for i in range(6):
+            self.mocktime = self.generate_pow(0, self.mocktime)
+        sync_blocks(self.nodes)
+        assert_equal(self.nodes[0].getshieldedbalance(), 250)
 
         # 3) nodes[0] generates a owner address
         #    nodes[1] generates a cold-staking address.
@@ -130,7 +142,8 @@ def run_test(self):
         assert (not self.isColdStakingEnforced())
         self.log.info("Creating a stake-delegation tx before cold staking enforcement...")
         assert_raises_rpc_error(-4, "Failed to accept tx in the memory pool (reason: cold-stake-inactive (code 16))\nTransaction canceled.",
-                                self.nodes[0].delegatestake, staker_address, INPUT_VALUE, owner_address, False, False, True)
+                                self.nodes[0].delegatestake, staker_address, INPUT_VALUE, owner_address,
+                                False, False, False, True)
         self.log.info("Good. Cold Staking NOT ACTIVE yet.")
 
         # Enable SPORK
@@ -162,11 +175,17 @@ def run_test(self):
         self.log.info("Good. Warning NOT triggered.")
 
         self.log.info("Now creating %d real stake-delegation txes..." % NUM_OF_INPUTS)
-        for i in range(NUM_OF_INPUTS):
+        for i in range(NUM_OF_INPUTS-1):
             res = self.nodes[0].delegatestake(staker_address, INPUT_VALUE, owner_address)
             assert(res != None and res["txid"] != None and res["txid"] != "")
             assert_equal(res["owner_address"], owner_address)
             assert_equal(res["staker_address"], staker_address)
+        # delegate  the shielded balance
+        res = self.nodes[0].delegatestake(staker_address, INPUT_VALUE, owner_address, False, False, True)
+        assert (res != None and res["txid"] != None and res["txid"] != "")
+        assert_equal(res["owner_address"], owner_address)
+        assert_equal(res["staker_address"], staker_address)
+        # sync and mine 2 blocks
         sync_mempools(self.nodes)
         self.mocktime = self.generate_pos(2, self.mocktime)
         sync_blocks(self.nodes)
@@ -175,6 +194,8 @@ def run_test(self):
         self.expected_balance = NUM_OF_INPUTS * INPUT_VALUE
         self.expected_immature_balance = 0
         self.checkBalances()
+        # also shielded balance of node 0 (250 - 249 - minimum fee 0.0001)
+        assert_equal(self.nodes[0].getshieldedbalance(), Decimal('0.9999'))
 
         # 6) check that the owner (nodes[0]) can spend the coins.
         # -------------------------------------------------------
@@ -259,8 +280,9 @@ def run_test(self):
         self.log.info("New block created (rawtx) by cold-staking. Trying to submit...")
         # Try to submit the block
         ret = self.nodes[1].submitblock(bytes_to_hex_str(new_block.serialize()))
+        assert (ret is None)
         self.log.info("Block %s submitted." % new_block.hash)
-        assert(ret is None)
+        assert_equal(new_block.hash, self.nodes[1].getbestblockhash())
 
         # Verify that nodes[0] accepts it
         sync_blocks(self.nodes)

test/functional/mining_pos_fakestake.py

@@ -219,7 +219,7 @@ def get_prev_modifier(prevBlockHash):
                 block_txes = self.make_txes(1, spending_prevouts, self.DUMMY_KEY.get_pubkey())
 
             # Stake the spam block
-            block = self.stake_block(1, nHeight, prevBlockHash, prevModifier, stakeInputs,
+            block = self.stake_block(1, 7, nHeight, prevBlockHash, prevModifier, "0", stakeInputs,
                                      nTime, "", block_txes, fDoubleSpend)
             # Log stake input
             prevout = COutPoint()

test/functional/test_framework/test_framework.py

@@ -827,20 +827,25 @@ def make_txes(self, node_id, spendingPrevOuts, to_pubKey):
 
         return block_txes
 
-    def stake_block(self, node_id,
+    def stake_block(self,
+            node_id,
+            nVersion,
             nHeight,
-            prevHhash,
+            prevHash,
             prevModifier,
+            finalsaplingroot,
             stakeableUtxos,
-            startTime=None,
-            privKeyWIF=None,
-            vtx=[],
-            fDoubleSpend=False):
+            startTime,
+            privKeyWIF,
+            vtx,
+            fDoubleSpend):
         """ manually stakes a block selecting the coinstake input from a list of candidates
         :param   node_id:           (int) index of the CTestNode used as rpc connection. Must own stakeableUtxos.
+                 nVersion:          (int) version of the block being produced (7 or 8)
                  nHeight:           (int) height of the block being produced
                  prevHash:          (string) hex string of the previous block hash
                  prevModifier       (string) hex string of the previous block stake modifier
+                 finalsaplingroot   (string) hex string of the previous block sapling root (blocks V8)
                  stakeableUtxos:    ({bytes --> (int, bytes, int)} dictionary)
                                     maps CStake "uniqueness" (i.e. serialized COutPoint -or hash stake, for zpiv-)
                                     to (amount, prevScript, timeBlockFrom).
@@ -863,7 +868,8 @@ def stake_block(self, node_id,
         # Create empty block with coinbase
         nTime = int(startTime) & 0xfffffff0
         coinbaseTx = create_coinbase_pos(nHeight)
-        block = create_block(int(prevHhash, 16), coinbaseTx, nTime)
+        block = create_block(int(prevHash, 16), coinbaseTx, nTime, nVersion, int(finalsaplingroot, 16))
+        block.nVersion = nVersion
 
         # Find valid kernel hash - iterates stakeableUtxos, then block.nTime
         block.solve_stake(stakeableUtxos, int(prevModifier, 16))
@@ -938,13 +944,19 @@ def stake_next_block(self, node_id,
             fDoubleSpend=False):
         """ Calls stake_block appending to the current tip"""
         assert_greater_than(len(self.nodes), node_id)
+        saplingActive = self.nodes[node_id].getblockchaininfo()['upgrades']['v5 dummy']['status'] == 'active'
+        blockVersion = 8 if saplingActive else 7
         nHeight = self.nodes[node_id].getblockcount()
         prevHhash = self.nodes[node_id].getblockhash(nHeight)
-        prevModifier = self.nodes[node_id].getblock(prevHhash)['stakeModifier']
+        prevBlock = self.nodes[node_id].getblock(prevHhash, True)
+        prevModifier = prevBlock['stakeModifier']
+        saplingRoot = prevBlock['finalsaplingroot'] # !TODO: update this if the block contains sapling txes
         return self.stake_block(node_id,
+                                blockVersion,
                                 nHeight+1,
                                 prevHhash,
                                 prevModifier,
+                                saplingRoot,
                                 stakeableUtxos,
                                 btime,
                                 privKeyWIF,