From c2d3bafeaa19c7a4913adac3255ed780a50ffe11 Mon Sep 17 00:00:00 2001 From: Simon Date: Thu, 26 Oct 2017 11:29:36 -0700 Subject: [PATCH 1/2] Closes #2639. z_shieldcoinbase is now supported, no longer experimental. This reverts commit 5023af7bd5fc8c147e6fd111b610bd8922e91131. --- qa/rpc-tests/wallet_shieldcoinbase.py | 4 ++-- src/init.cpp | 2 -- src/wallet/rpcwallet.cpp | 11 ----------- 3 files changed, 2 insertions(+), 15 deletions(-) diff --git a/qa/rpc-tests/wallet_shieldcoinbase.py b/qa/rpc-tests/wallet_shieldcoinbase.py index 599663390..d0e4bb122 100755 --- a/qa/rpc-tests/wallet_shieldcoinbase.py +++ b/qa/rpc-tests/wallet_shieldcoinbase.py @@ -18,11 +18,11 @@ class WalletShieldCoinbaseTest (BitcoinTestFramework): initialize_chain_clean(self.options.tmpdir, 4) def setup_network(self, split=False): - args = ['-regtestprotectcoinbase', '-debug=zrpcunsafe', '-experimentalfeatures', '-zshieldcoinbase'] + args = ['-regtestprotectcoinbase', '-debug=zrpcunsafe'] self.nodes = [] self.nodes.append(start_node(0, self.options.tmpdir, args)) self.nodes.append(start_node(1, self.options.tmpdir, args)) - args2 = ['-regtestprotectcoinbase', '-debug=zrpcunsafe', '-experimentalfeatures', '-zshieldcoinbase', "-mempooltxinputlimit=7"] + args2 = ['-regtestprotectcoinbase', '-debug=zrpcunsafe', "-mempooltxinputlimit=7"] self.nodes.append(start_node(2, self.options.tmpdir, args2)) connect_nodes_bi(self.nodes,0,1) connect_nodes_bi(self.nodes,1,2) diff --git a/src/init.cpp b/src/init.cpp index 6f87917a0..59a59524d 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -790,8 +790,6 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler) if (!fExperimentalMode) { if (mapArgs.count("-developerencryptwallet")) { return InitError(_("Wallet encryption requires -experimentalfeatures.")); - } else if (mapArgs.count("-zshieldcoinbase")) { - return InitError(_("RPC call z_shieldcoinbase requires -experimentalfeatures.")); } } diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index bac5dc871..6738ec5d8 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3527,16 +3527,9 @@ UniValue z_shieldcoinbase(const UniValue& params, bool fHelp) if (!EnsureWalletIsAvailable(fHelp)) return NullUniValue; - bool fEnableShieldCoinbase = fExperimentalMode && GetBoolArg("-zshieldcoinbase", false); - std::string strDisabledMsg = ""; - if (!fEnableShieldCoinbase) { - strDisabledMsg = "\nWARNING: z_shieldcoinbase is DISABLED but can be enabled as an experimental feature.\n"; - } - if (fHelp || params.size() < 2 || params.size() > 3) throw runtime_error( "z_shieldcoinbase \"fromaddress\" \"tozaddress\" ( fee )\n" - + strDisabledMsg + "\nShield transparent coinbase funds by sending to a shielded zaddr. This is an asynchronous operation and utxos" "\nselected for shielding will be locked. If there is an error, they are unlocked. The RPC call `listlockunspent`" "\ncan be used to return a list of locked utxos. The number of coinbase utxos selected for shielding is limited by" @@ -3558,10 +3551,6 @@ UniValue z_shieldcoinbase(const UniValue& params, bool fHelp) "}\n" ); - if (!fEnableShieldCoinbase) { - throw JSONRPCError(RPC_WALLET_ERROR, "Error: z_shieldcoinbase is disabled."); - } - LOCK2(cs_main, pwalletMain->cs_wallet); // Validate the from address From c5dabd2b66af89a7418658b82fbb12c2dcbf5257 Mon Sep 17 00:00:00 2001 From: Simon Date: Wed, 1 Nov 2017 10:40:24 -0700 Subject: [PATCH 2/2] Closes #2639. Adds optional limit parameter with a default value of 50. The new parameter is to satisfy the principle of least astonishment by providing a sensible default for the maximum number of transparent inputs to shield. If users do not configure -mempooltxinputlimit it is possible for them to create transactions with hundreds of inputs which suffer from mining delay, due to the current state of the network where some miners have configured -mempooltxinputlimit as a way to deal with the problem of quadratic hashing. --- doc/payment-api.md | 2 +- qa/rpc-tests/wallet_shieldcoinbase.py | 40 +++++++++++++++++++++++++-- src/rpcclient.cpp | 1 + src/test/rpc_wallet_tests.cpp | 9 +++++- src/wallet/rpcwallet.cpp | 23 +++++++++++---- 5 files changed, 65 insertions(+), 10 deletions(-) diff --git a/doc/payment-api.md b/doc/payment-api.md index 803a0dc97..1582255bb 100644 --- a/doc/payment-api.md +++ b/doc/payment-api.md @@ -72,7 +72,7 @@ Command | Parameters | Description --- | --- | --- z_listreceivedbyaddress
| zaddr [minconf=1] | Return a list of amounts received by a zaddr belonging to the node’s wallet.

Optionally set the minimum number of confirmations which a received amount must have in order to be included in the result. Use 0 to count unconfirmed transactions.

Output:
[{
“txid”: “4a0f…”,
“amount”: 0.54,
“memo”:”F0FF…”,}, {...}, {...}
] z_sendmany
| fromaddress amounts [minconf=1] [fee=0.0001] | _This is an Asynchronous RPC call_

Send funds from an address to multiple outputs. The address can be either a taddr or a zaddr.

Amounts is a list containing key/value pairs corresponding to the addresses and amount to pay. Each output address can be in taddr or zaddr format.

When sending to a zaddr, you also have the option of attaching a memo in hexadecimal format.

**NOTE:**When sending coinbase funds to a zaddr, the node's wallet does not allow any change. Put another way, spending a partial amount of a coinbase utxo is not allowed. This is not a consensus rule but a local wallet rule due to the current implementation of z_sendmany. In future, this rule may be removed.

Example of Outputs parameter:
[{“address”:”t123…”, “amount”:0.005},
,{“address”:”z010…”,”amount”:0.03, “memo”:”f508af…”}]

Optionally set the minimum number of confirmations which a private or transparent transaction must have in order to be used as an input. When sending from a zaddr, minconf must be greater than zero.

Optionally set a transaction fee, which by default is 0.0001 ZEC.

Any transparent change will be sent to a new transparent address. Any private change will be sent back to the zaddr being used as the source of funds.

Returns an operationid. You use the operationid value with z_getoperationstatus and z_getoperationresult to obtain the result of sending funds, which if successful, will be a txid. -z_shieldcoinbase
| fromaddress toaddress [fee=0.0001] | _This is an Asynchronous RPC call_

Shield transparent coinbase funds by sending to a shielded z address. Utxos selected for shielding will be locked. If there is an error, they are unlocked. The RPC call `listlockunspent` can be used to return a list of locked utxos. The number of coinbase utxos selected for shielding is limited by both the -mempooltxinputlimit=xxx option and a consensus rule defining a maximum transaction size of 100000 bytes.

The from address is a taddr or "*" for all taddrs belonging to the wallet. The to address is a zaddr. The default fee is 0.0001.

Returns an object containing an operationid which can be used with z_getoperationstatus and z_getoperationresult, along with key-value pairs regarding how many utxos are being shielded in this trasaction and what remains to be shielded. +z_shieldcoinbase
| fromaddress toaddress [fee=0.0001] [limit=50] | _This is an Asynchronous RPC call_

Shield transparent coinbase funds by sending to a shielded z address. Utxos selected for shielding will be locked. If there is an error, they are unlocked. The RPC call `listlockunspent` can be used to return a list of locked utxos.

The number of coinbase utxos selected for shielding can be set with the limit parameter, which has a default value of 50. If the parameter is set to 0, the number of utxos selected is limited by the `-mempooltxinputlimit` option. Any limit is constrained by a consensus rule defining a maximum transaction size of 100000 bytes.

The from address is a taddr or "*" for all taddrs belonging to the wallet. The to address is a zaddr. The default fee is 0.0001.

Returns an object containing an operationid which can be used with z_getoperationstatus and z_getoperationresult, along with key-value pairs regarding how many utxos are being shielded in this trasaction and what remains to be shielded. ### Operations diff --git a/qa/rpc-tests/wallet_shieldcoinbase.py b/qa/rpc-tests/wallet_shieldcoinbase.py index d0e4bb122..d1698d945 100755 --- a/qa/rpc-tests/wallet_shieldcoinbase.py +++ b/qa/rpc-tests/wallet_shieldcoinbase.py @@ -114,6 +114,20 @@ class WalletShieldCoinbaseTest (BitcoinTestFramework): errorString = e.error['message'] assert_equal("Insufficient coinbase funds" in errorString, True) + # Shielding will fail because limit parameter must be at least 0 + try: + self.nodes[0].z_shieldcoinbase("*", myzaddr, Decimal('0.001'), -1) + except JSONRPCException,e: + errorString = e.error['message'] + assert_equal("Limit on maximum number of utxos cannot be negative" in errorString, True) + + # Shielding will fail because limit parameter is absurdly large + try: + self.nodes[0].z_shieldcoinbase("*", myzaddr, Decimal('0.001'), 99999999999999) + except JSONRPCException,e: + errorString = e.error['message'] + assert_equal("JSON integer out of range" in errorString, True) + # Shield coinbase utxos from node 0 of value 40, standard fee of 0.00010000 result = self.nodes[0].z_shieldcoinbase(mytaddr, myzaddr) self.wait_and_assert_operationid_status(0, result['opid']) @@ -151,14 +165,15 @@ class WalletShieldCoinbaseTest (BitcoinTestFramework): # Shielding the 800 utxos will occur over two transactions, since max tx size is 100,000 bytes. # We don't verify shieldingValue as utxos are not selected in any specific order, so value can change on each test run. - result = self.nodes[0].z_shieldcoinbase(mytaddr, myzaddr, 0) + # We set an unrealistically high limit parameter of 99999, to verify that max tx size will constrain the number of utxos. + result = self.nodes[0].z_shieldcoinbase(mytaddr, myzaddr, 0, 99999) assert_equal(result["shieldingUTXOs"], Decimal('662')) assert_equal(result["remainingUTXOs"], Decimal('138')) remainingValue = result["remainingValue"] opid1 = result['opid'] # Verify that utxos are locked (not available for selection) by queuing up another shielding operation - result = self.nodes[0].z_shieldcoinbase(mytaddr, myzaddr) + result = self.nodes[0].z_shieldcoinbase(mytaddr, myzaddr, 0, 0) assert_equal(result["shieldingValue"], Decimal(remainingValue)) assert_equal(result["shieldingUTXOs"], Decimal('138')) assert_equal(result["remainingValue"], Decimal('0')) @@ -176,8 +191,9 @@ class WalletShieldCoinbaseTest (BitcoinTestFramework): self.sync_all() # Verify maximum number of utxos which node 2 can shield is limited by option -mempooltxinputlimit + # This option is used when the limit parameter is set to 0. mytaddr = self.nodes[2].getnewaddress() - result = self.nodes[2].z_shieldcoinbase(mytaddr, myzaddr, 0) + result = self.nodes[2].z_shieldcoinbase(mytaddr, myzaddr, Decimal('0.0001'), 0) assert_equal(result["shieldingUTXOs"], Decimal('7')) assert_equal(result["remainingUTXOs"], Decimal('13')) self.wait_and_assert_operationid_status(2, result['opid']) @@ -185,5 +201,23 @@ class WalletShieldCoinbaseTest (BitcoinTestFramework): self.nodes[1].generate(1) self.sync_all() + # Verify maximum number of utxos which node 0 can shield is set by default limit parameter of 50 + self.nodes[0].generate(200) + self.sync_all() + mytaddr = self.nodes[0].getnewaddress() + result = self.nodes[0].z_shieldcoinbase(mytaddr, myzaddr, Decimal('0.0001')) + assert_equal(result["shieldingUTXOs"], Decimal('50')) + assert_equal(result["remainingUTXOs"], Decimal('50')) + self.wait_and_assert_operationid_status(0, result['opid']) + + # Verify maximum number of utxos which node 0 can shield can be set by the limit parameter + result = self.nodes[0].z_shieldcoinbase(mytaddr, myzaddr, Decimal('0.0001'), 33) + assert_equal(result["shieldingUTXOs"], Decimal('33')) + assert_equal(result["remainingUTXOs"], Decimal('17')) + self.wait_and_assert_operationid_status(0, result['opid']) + sync_blocks(self.nodes) # don't sync on mempool as node 2 will reject thix tx due to its mempooltxinputlimit + self.nodes[1].generate(1) + self.sync_all() + if __name__ == '__main__': WalletShieldCoinbaseTest().main() diff --git a/src/rpcclient.cpp b/src/rpcclient.cpp index 376b2b3d5..2c5bd122f 100644 --- a/src/rpcclient.cpp +++ b/src/rpcclient.cpp @@ -110,6 +110,7 @@ static const CRPCConvertParam vRPCConvertParams[] = { "z_sendmany", 2}, { "z_sendmany", 3}, { "z_shieldcoinbase", 2}, + { "z_shieldcoinbase", 3}, { "z_getoperationstatus", 0}, { "z_getoperationresult", 0}, { "z_importkey", 2 }, diff --git a/src/test/rpc_wallet_tests.cpp b/src/test/rpc_wallet_tests.cpp index 8233097de..6017333b4 100644 --- a/src/test/rpc_wallet_tests.cpp +++ b/src/test/rpc_wallet_tests.cpp @@ -1251,7 +1251,7 @@ BOOST_AUTO_TEST_CASE(rpc_z_shieldcoinbase_parameters) BOOST_CHECK_THROW(CallRPC("z_shieldcoinbase"), runtime_error); BOOST_CHECK_THROW(CallRPC("z_shieldcoinbase toofewargs"), runtime_error); - BOOST_CHECK_THROW(CallRPC("z_shieldcoinbase too many args here"), runtime_error); + BOOST_CHECK_THROW(CallRPC("z_shieldcoinbase too many args shown here"), runtime_error); // bad from address BOOST_CHECK_THROW(CallRPC("z_shieldcoinbase " @@ -1279,6 +1279,13 @@ BOOST_AUTO_TEST_CASE(rpc_z_shieldcoinbase_parameters) "21000001" ), runtime_error); + // invalid limit, must be at least 0 + BOOST_CHECK_THROW(CallRPC("z_shieldcoinbase " + "tmRr6yJonqGK23UVhrKuyvTpF8qxQQjKigJ " + "tnpoQJVnYBZZqkFadj2bJJLThNCxbADGB5gSGeYTAGGrT5tejsxY9Zc1BtY8nnHmZkB " + "100 -1" + ), runtime_error); + // Test constructor of AsyncRPCOperation_sendmany std::string testnetzaddr = "ztjiDe569DPNbyTE6TSdJTaSDhoXEHLGvYoUnBU1wfVNU52TEyT6berYtySkd21njAeEoh8fFJUT42kua9r8EnhBaEKqCpP"; std::string mainnetzaddr = "zcMuhvq8sEkHALuSU2i4NbNQxshSAYrpCExec45ZjtivYPbuiFPwk6WHy4SvsbeZ4siy1WheuRGjtaJmoD1J8bFqNXhsG6U"; diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 6738ec5d8..843af53bd 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3522,18 +3522,21 @@ When estimating the number of coinbase utxos we can shield in a single transacti */ #define CTXIN_SPEND_P2SH_SIZE 400 +#define SHIELD_COINBASE_DEFAULT_LIMIT 50 + UniValue z_shieldcoinbase(const UniValue& params, bool fHelp) { if (!EnsureWalletIsAvailable(fHelp)) return NullUniValue; - if (fHelp || params.size() < 2 || params.size() > 3) + if (fHelp || params.size() < 2 || params.size() > 4) throw runtime_error( - "z_shieldcoinbase \"fromaddress\" \"tozaddress\" ( fee )\n" + "z_shieldcoinbase \"fromaddress\" \"tozaddress\" ( fee ) ( limit )\n" "\nShield transparent coinbase funds by sending to a shielded zaddr. This is an asynchronous operation and utxos" "\nselected for shielding will be locked. If there is an error, they are unlocked. The RPC call `listlockunspent`" - "\ncan be used to return a list of locked utxos. The number of coinbase utxos selected for shielding is limited by" - "\nboth the -mempooltxinputlimit=xxx option and a consensus rule defining a maximum transaction size of " + "\ncan be used to return a list of locked utxos. The number of coinbase utxos selected for shielding can be limited" + "\nby the caller. If the limit parameter is set to zero, the -mempooltxinputlimit option will determine the number" + "\nof uxtos. Any limit is constrained by the consensus rule defining a maximum transaction size of " + strprintf("%d bytes.", MAX_TX_SIZE) + HelpRequiringPassphrase() + "\n" "\nArguments:\n" @@ -3541,6 +3544,8 @@ UniValue z_shieldcoinbase(const UniValue& params, bool fHelp) "2. \"toaddress\" (string, required) The address is a zaddr.\n" "3. fee (numeric, optional, default=" + strprintf("%s", FormatMoney(SHIELD_COINBASE_DEFAULT_MINERS_FEE)) + ") The fee amount to attach to this transaction.\n" + "4. limit (numeric, optional, default=" + + strprintf("%d", SHIELD_COINBASE_DEFAULT_LIMIT) + ") Limit on the maximum number of utxos to shield. Set to 0 to use node option -mempooltxinputlimit.\n" "\nResult:\n" "{\n" " \"operationid\": xxx (string) An operationid to pass to z_getoperationstatus to get the result of the operation.\n" @@ -3583,6 +3588,14 @@ UniValue z_shieldcoinbase(const UniValue& params, bool fHelp) } } + int nLimit = SHIELD_COINBASE_DEFAULT_LIMIT; + if (params.size() > 3) { + nLimit = params[3].get_int(); + if (nLimit < 0) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Limit on maximum number of utxos cannot be negative"); + } + } + // Prepare to get coinbase utxos std::vector inputs; CAmount shieldedValue = 0; @@ -3590,7 +3603,7 @@ UniValue z_shieldcoinbase(const UniValue& params, bool fHelp) size_t estimatedTxSize = 2000; // 1802 joinsplit description + tx overhead + wiggle room size_t utxoCounter = 0; bool maxedOutFlag = false; - size_t mempoolLimit = (size_t)GetArg("-mempooltxinputlimit", 0); + size_t mempoolLimit = (nLimit != 0) ? nLimit : (size_t)GetArg("-mempooltxinputlimit", 0); // Set of addresses to filter utxos by set setAddress = {};