Implement z_mergetoaddress for combining UTXOs and notes by str4d · Pull Request #2797 · zcash/zcash

str4d · 2017-12-14T00:00:01Z

Closes #2493.

str4d · 2017-12-14T00:00:29Z

I'm still working on the RPC test, but I've opened this PR to start garnering review on the code.

tromer · 2017-12-14T00:16:44Z

Nice! Also closes #1785.

How does this handle coinbase?

str4d · 2017-12-14T01:43:48Z

That's a good point - currently it will try to include them, and thus fail if the target is a t-addr. Given that we have z_shieldcoinbase, I propose that this method simply ignore coinbase UTXOs.

bitcartel · 2017-12-15T17:12:37Z

Let's make this an experimental feature, just as z_shieldcoinbase was at first.

str4d · 2017-12-18T21:53:28Z

Bumping to 1.0.15 due to necessary work.

zkbot · 2017-12-20T23:26:18Z

☔ The latest upstream changes (presumably #2143) made this pull request unmergeable. Please resolve the merge conflicts.

bitcartel · 2018-01-08T17:56:07Z

Should be MERGETOADDRESS_OPERATION_DEFAULT_MINERS_FEE

bitcartel · 2018-01-08T17:57:17Z

Or MERGE_TO_ADDRESS_... to match formatting of other M_T_A_... constants.

str4d · 2018-01-18T21:13:57Z

Rebased on master to fix merge conflicts, added unit tests, finished RPC test, fixed bugs and missing details. I'll add another commit shortly that gates z_mergetoaddress under experimental features.

bitcartel

Early review feedback from testing in regtest and mainnet.

Seems to be an issue with mixing taddrs and zaddrs as inputs, results in an anchor problem.
Experienced a merge which ended up in a different destination zaddr than the one specified.
The first parameter is mangled, depending on the patch to rpcclient.cpp being applied or not.

All of the above is detailed in the comments.

bitcartel · 2018-01-20T22:22:44Z

+            "\nArguments:\n"
+            "1. fromaddresses         (string, required) A JSON array with addresses.\n"
+            "                         The following special strings are also accepted:\n"
+            "                             - \"*\": Merge both UTXOs and notes from all addressed belonging to the wallet.\n"


addressed -> addresses

Tried but failed.

./zcash-cli z_mergetoaddress "*" ztJqA2UeXP5ZQWiuK8HRijfj3V7GxZNTXQkjf4WXc8Sj2Evgyen6DmeW5yhhZjTzFXhsqdS6bqXjV7oT9zu9GdzBqB6viRE error: Error parsing JSON:* ./zcash-cli z_mergetoaddress '*' ztJqA2UeXP5ZQWiuK8HRijfj3V7GxZNTXQkjf4WXc8Sj2Evgyen6DmeW5yhhZjTzFXhsqdS6bqXjV7oT9zu9GdzBqB6viRE error: Error parsing JSON:*

See comment in src/rpcclient.cpp

bitcartel · 2018-01-20T22:24:40Z

+            + HelpRequiringPassphrase() + "\n"
+            "\nArguments:\n"
+            "1. fromaddresses         (string, required) A JSON array with addresses.\n"
+            "                         The following special strings are also accepted:\n"


Not clear if those special strings will also be accepted as part of the array, instead of replacing the array. A user might try '["t1HelloUniqueAddress", "ANY_ZADDR"]'

I'll clarify the help text.

Clarified to explain that the above is now actually allowed 😄

bitcartel · 2018-01-20T22:27:57Z

+
+    if (fHelp || params.size() < 2 || params.size() > 6)
+        throw runtime_error(
+            "z_mergetoaddress [\"fromaddress\", ... ] \"toaddress\" ( fee ) ( transparent_limit ) ( shielded_limit ) ( memo )\n"


Later on in the documentation, the arguments are named differently. "utxo_limit" and "note_limit". Also 'UTXO' and 'note' are used frequently.

Fixed to use transparent_limit and shielded_limit. Re: UTXO and note usage, I figure it makes sense when talking about specific objects, but we should probably do a review of the whole RPC API's help text for consistency at some point.

bitcartel · 2018-01-20T22:30:20Z

+            + strprintf("%d", MERGE_TO_ADDRESS_DEFAULT_TRANSPARENT_LIMIT) + ") Limit on the maximum number of UTXOs to merge.  Set to 0 to use node option -mempooltxinputlimit.\n"
+            "4. note_limit            (numeric, optional, default="
+            + strprintf("%d", MERGE_TO_ADDRESS_DEFAULT_SHIELDED_LIMIT) + ") Limit on the maximum number of notes to merge.  Set to 0 to merge as many as will fit in the transaction.\n"
+            "5. \"memo\"                (string, optional) If toaddress is a zaddr, this will be put in the memo field of the new note.\n"


Instead of leading with "If", how about "When the destination toaddress is a zaddr, this will be stored in the memo field of the new note" ?

bitcartel · 2018-01-20T22:32:52Z

+            "1. fromaddresses         (string, required) A JSON array with addresses.\n"
+            "                         The following special strings are also accepted:\n"
+            "                             - \"*\": Merge both UTXOs and notes from all addressed belonging to the wallet.\n"
+            "                             - \"ANY_TADDR\": Merge UTXOs from all taddrs belonging to the wallet.\n"


Is this case sensitive?

Tested, yes this is case sensitive. Should we make this case insensitive?

Simple enough to do so, if we think it is beneficial.

bitcartel · 2018-01-24T23:41:37Z

+
+#define JOINSPLIT_SIZE JSDescription().GetSerializeSize(SER_NETWORK, PROTOCOL_VERSION)
+
+UniValue z_mergetoaddress(const UniValue& params, bool fHelp)


In testing I came across the case where I only had 1 note at the from address and I was merging to the same address.

"mergingNotes": 1, "mergingShieldedValue": 1.23000000

Afterwards, my balance for the address was:

1.22990000

So I effectively lost 0.0001 Zec as a fee, and still have one note. While this is the correct result, perhaps we should disallow merging when the from/to address are the same, and there is only note available for merging.

That's an excellent point! 😆

bitcartel · 2018-01-24T23:45:46Z

+            "                         The following special strings are also accepted:\n"
+            "                             - \"*\": Merge both UTXOs and notes from all addressed belonging to the wallet.\n"
+            "                             - \"ANY_TADDR\": Merge UTXOs from all taddrs belonging to the wallet.\n"
+            "                             - \"ANY_ZADDR\": Merge notes from all zaddrs belonging to the wallet.\n"


I tried passing this in but fails?

./zcash-cli z_mergetoaddress '["ANY_ZADDR"]' ztJqA2UeXP5ZQWiuK8HRijfj3V7GxZNTXQkjf4WXc8Sj2Evgyen6DmeW5yhhZjTzFXhsqdS6bqXjV7oT9zu9GdzBqB6viRE error code: -8 error message: Invalid parameter, unknown address format: ANY_ZADDR ./zcash-cli z_mergetoaddress ANY_ZADDR ztJqA2UeXP5ZQWiuK8HRijfj3V7GxZNTXQkjf4WXc8Sj2Evgyen6DmeW5yhhZjTzFXhsqdS6bqXjV7oT9zu9GdzBqB6viRE error: Error parsing JSON:ANY_ZADDR ./zcash-cli z_mergetoaddress "ANY_ZADDR" ztJqA2UeXP5ZQWiuK8HRijfj3V7GxZNTXQkjf4WXc8Sj2Evgyen6DmeW5yhhZjTzFXhsqdS6bqXjV7oT9zu9GdzBqB6viRE error: Error parsing JSON:ANY_ZADDR

See comment in src/rpcclient.cpp

bitcartel · 2018-01-25T00:35:55Z

+            + strprintf("%d", MERGE_TO_ADDRESS_DEFAULT_TRANSPARENT_LIMIT) + ") Limit on the maximum number of UTXOs to merge.  Set to 0 to use node option -mempooltxinputlimit.\n"
+            "4. note_limit            (numeric, optional, default="
+            + strprintf("%d", MERGE_TO_ADDRESS_DEFAULT_SHIELDED_LIMIT) + ") Limit on the maximum number of notes to merge.  Set to 0 to merge as many as will fit in the transaction.\n"
+            "5. \"memo\"                (string, optional) If toaddress is a zaddr, this will be put in the memo field of the new note.\n"


Mention that it's hex string only as users might enter human readable string?

bitcartel · 2018-01-25T21:00:59Z

    { "z_gettotalbalance", 0},
    { "z_gettotalbalance", 1},
    { "z_gettotalbalance", 2},
+    { "z_mergetoaddress", 0},


So looks like we may have an issue here. With { "z_mergetoaddress", 0}, the JSON arrays can be parsed correctly. However, it causes string arguments "", "ANY_..." to return a JSON parsing error. Removing this line solves the issue for "", "ANY_.." but then causes arguments of '["addr1", "addr2", ...]' to fail.

Yeah, it's annoying. I think the way to fix this properly is to remove this line, and then if the string doesn't match one of the expected constants, manually call the JSON parser.

Follow-up question: Why does the RPC test I wrote not fail? It uses both string and array arguments...

Ah, it's because this line affects how zcash-cli parses the CLI inputs. This is kind of annoying, because it means that if we remove this line, zcash-cli sends an unparsed JSON string representation as an actual string.

Okay, to fix this, I'm going to just make it always an array, but it can contain the keywords.

bitcartel · 2018-01-25T21:24:02Z

+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#ifndef ASYNCRPCOPERATION_MERGETOADDRESS_H


Attaching comment here about a reproducbile anchor problem when merging using "*" when there are both utxos and notes present:

./zcash-cli z_mergetoaddress "*" ztJqA2UeXP5ZQWiuK8HRijfj3V7GxZNTXQkjf4WXc8Sj2Evgyen6DmeW5yhhZjTzFXhsqdS6bqXjV7oT9zu9GdzBqB6viRE 0.0001 0 0 "BEEF" { "remainingUTXOs": 0, "remainingTransparentValue": 0.00000000, "remainingNotes": 0, "remainingShieldedValue": 0.00000000, "mergingUTXOs": 2, "mergingTransparentValue": 200.00000000, "mergingNotes": 1, "mergingShieldedValue": 1575.31036597, "opid": "opid-8e1ac86c-dc35-4465-b529-9a8fbd849aea" }

This fails:

2018-01-25 21:17:48 opid-8e1ac86c-dc35-4465-b529-9a8fbd849aea: z_mergetoaddress initialized (params={"fromaddresses":"*","toaddress":"ztJqA2UeXP5ZQWiuK8HRijfj3V7GxZNTXQkjf4WXc8Sj2Evgyen6DmeW5yhhZjTzFXhsqdS6bqXjV7oT9zu9GdzBqB6viRE","fee":0.00010000}) 2018-01-25 21:17:48 opid-8e1ac86c-dc35-4465-b529-9a8fbd849aea: spending 1775.31036597 to send 1775.31026597 with fee 0.0001 2018-01-25 21:17:48 opid-8e1ac86c-dc35-4465-b529-9a8fbd849aea: transparent input: 200.00 2018-01-25 21:17:48 opid-8e1ac86c-dc35-4465-b529-9a8fbd849aea: private input: 1575.31036597 2018-01-25 21:17:48 opid-8e1ac86c-dc35-4465-b529-9a8fbd849aea: private output: 1775.31026597 2018-01-25 21:17:48 opid-8e1ac86c-dc35-4465-b529-9a8fbd849aea: fee: 0.0001 2018-01-25 21:17:48 opid-8e1ac86c-dc35-4465-b529-9a8fbd849aea: z_mergetoaddress finished (status=failed, error=Could not find previous JoinSplit anchor)

However, calling "ANY_TADDR" at this point succeeds, as does "ANY_ZADDR".
Here is the "ANY_ZADDR" output:

./zcash-cli z_mergetoaddress "ANY_ZADDR" ztJqA2UeXP5ZQWiuK8HRijfj3V7GxZNTXQkjf4WXc8Sj2Evgyen6DmeW5yhhZjTzFXhsqdS6bqXjV7oT9zu9GdzBqB6viRE 0.0001 0 0 "BEEF" { "remainingUTXOs": 0, "remainingTransparentValue": 0.00000000, "remainingNotes": 0, "remainingShieldedValue": 0.00000000, "mergingUTXOs": 0, "mergingTransparentValue": 0.00000000, "mergingNotes": 1, "mergingShieldedValue": 1575.31036597, "opid": "opid-deec2331-c1c2-4a75-9314-b72808b95255" }

and log output:

2018-01-25 21:22:10 opid-deec2331-c1c2-4a75-9314-b72808b95255: z_mergetoaddress initialized (params={"fromaddresses":"ANY_ZADDR","toaddress":"ztJqA2UeXP5ZQWiuK8HRijfj3V7GxZNTXQkjf4WXc8Sj2Evgyen6DmeW5yhhZjTzFXhsqdS6bqXjV7oT9zu9GdzBqB6viRE","fee":0.00010000}) 2018-01-25 21:22:10 opid-deec2331-c1c2-4a75-9314-b72808b95255: spending 1575.31036597 to send 1575.31026597 with fee 0.0001 2018-01-25 21:22:10 opid-deec2331-c1c2-4a75-9314-b72808b95255: transparent input: 0.00 2018-01-25 21:22:10 opid-deec2331-c1c2-4a75-9314-b72808b95255: private input: 1575.31036597 2018-01-25 21:22:10 opid-deec2331-c1c2-4a75-9314-b72808b95255: private output: 1575.31026597 2018-01-25 21:22:10 opid-deec2331-c1c2-4a75-9314-b72808b95255: fee: 0.0001 2018-01-25 21:22:10 opid-deec2331-c1c2-4a75-9314-b72808b95255: spending note (txid=1ae2ac96d5, vjoinsplit=0, ciphertext=0, amount=1575.31036597, height=652, confirmations=1) 2018-01-25 21:22:10 opid-deec2331-c1c2-4a75-9314-b72808b95255: generating note for change (amount=1575.31026597) 2018-01-25 21:22:10 opid-deec2331-c1c2-4a75-9314-b72808b95255: creating joinsplit at index 0 (vpub_old=0.00, vpub_new=0.0001, in[0]=1575.31036597, in[1]=0.00, out[0]=0.00, out[1]=1575.31026597) 2018-01-25 21:22:46 opid-deec2331-c1c2-4a75-9314-b72808b95255: Payment Disclosure: js=0, n=1, zaddr=ztkjJKs99AqHhoUUHa5h5cJ44tmQvBqgk66D2unWhk1xkP2L36SxhufEQLj5WyeS9uy8c5A4ZvpfEWuQkHE6PEdEuzUPkqt 2018-01-25 21:22:46 opid-deec2331-c1c2-4a75-9314-b72808b95255: Payment Disclosure: js=0, n=0, zaddr=ztJqA2UeXP5ZQWiuK8HRijfj3V7GxZNTXQkjf4WXc8Sj2Evgyen6DmeW5yhhZjTzFXhsqdS6bqXjV7oT9zu9GdzBqB6viRE 2018-01-25 21:22:46 Blockpolicy mempool tx 05b61e6592 not adding 2018-01-25 21:22:46 AddToWallet 05b61e6592fb92a4cd4497a2730009737fd2f9899974c66a195a2368c58edcae new 2018-01-25 21:22:46 opid-deec2331-c1c2-4a75-9314-b72808b95255: z_mergetoaddress finished (status=success, txid=05b61e6592fb92a4cd4497a2730009737fd2f9899974c66a195a2368c58edcae)

The above is also reproducible when manually specifying the array of addresses e.g. '["t....", "z....."]'

bitcartel · 2018-01-26T01:20:34Z

+#ifndef ASYNCRPCOPERATION_MERGETOADDRESS_H
+#define ASYNCRPCOPERATION_MERGETOADDRESS_H
+
+#include "amount.h"


Attaching here as comment:

Funds were merged into a different zaddr (in the wallet), and not the zaddr (in the wallet) specified. This was run on mainnet.

./zcash-cli z_mergetoaddress ANY_ZADDR zcdYqZzp5ABTtsKi8PdEZefwjWfbeuBMNRn91pWcLsbZAztDcUBXwx25aaGk1NoSCUP66AA5VMBhv9kCQE3V1zTMYUFRERy 0.00001 { "remainingUTXOs": 0, "remainingTransparentValue": 0.00000000, "remainingNotes": 0, "remainingShieldedValue": 0.00000000, "mergingUTXOs": 0, "mergingTransparentValue": 0.00000000, "mergingNotes": 1, "mergingShieldedValue": 0.03872239, "opid": "opid-77f1dc74-3513-41a3-b4f2-88625b4cff4b" }

debug.log:

2018-01-26 00:54:58 opid-77f1dc74-3513-41a3-b4f2-88625b4cff4b: z_mergetoaddress initialized (params={"fromaddresses":"ANY_ZADDR","toaddress":"zcdYqZzp5ABTtsKi8PdEZefwjWfbeuBMNRn91pWcLsbZAztDcUBXwx25aaGk1NoSCUP66AA5VMBhv9kCQE3V1zTMYUFRERy","fee":0.00001000}) 2018-01-26 00:54:58 opid-77f1dc74-3513-41a3-b4f2-88625b4cff4b: spending 0.03872239 to send 0.03871239 with fee 0.00001 2018-01-26 00:54:58 opid-77f1dc74-3513-41a3-b4f2-88625b4cff4b: transparent input: 0.00 2018-01-26 00:54:58 opid-77f1dc74-3513-41a3-b4f2-88625b4cff4b: private input: 0.03872239 2018-01-26 00:54:58 opid-77f1dc74-3513-41a3-b4f2-88625b4cff4b: private output: 0.03871239 2018-01-26 00:54:58 opid-77f1dc74-3513-41a3-b4f2-88625b4cff4b: fee: 0.00001 2018-01-26 00:54:58 opid-77f1dc74-3513-41a3-b4f2-88625b4cff4b: spending note (txid=1404b71a86, vjoinsplit=0, ciphertext=0, amount=0.03872239, height=260958, confirmations=10) 2018-01-26 00:54:58 opid-77f1dc74-3513-41a3-b4f2-88625b4cff4b: generating note for change (amount=0.03871239) 2018-01-26 00:54:58 opid-77f1dc74-3513-41a3-b4f2-88625b4cff4b: creating joinsplit at index 0 (vpub_old=0.00, vpub_new=0.00001, in[0]=0.03872239, in[1]=0.00, out[0]=0.00, out[1]=0.03871239) ... 2018-01-26 00:55:35 opid-77f1dc74-3513-41a3-b4f2-88625b4cff4b: Payment Disclosure: js=0, n=1, zaddr=zcDEb4i1nYPHgAABg86TuhoBLJ1aSTpA8kK4bE32eXcjaJdABKymaG5YAWfgiyGCk9mZocXQFHjBeuGkBFpAGqAv5ERDtYx 2018-01-26 00:55:35 opid-77f1dc74-3513-41a3-b4f2-88625b4cff4b: Payment Disclosure: js=0, n=0, zaddr=zcUxZKkUd6TwWtxYxJta2A4kYkz4qHoyEdJBiB4FvyKTheYdjHNNJUR73qMmFZw4rqwbTjwLxjhYMBkgwQxVWKPqawmULqp ... 2018-01-26 00:55:35 opid-77f1dc74-3513-41a3-b4f2-88625b4cff4b: z_mergetoaddress finished (status=success, txid=0123d0c0a003cabd3eb7460a73e78d258f0d665c6904d224100d25249cd1f12e)

Payment disclosure shows index 0 using a different address in the wallet from the one specified on the command line, and which was passed into the z_ method, whereas index 1 is a dummy address.

./zcash-cli z_getbalance zcUxZKkUd6TwWtxYxJta2A4kYkz4qHoyEdJBiB4FvyKTheYdjHNNJUR73qMmFZw4rqwbTjwLxjhYMBkgwQxVWKPqawmULqp 0.03871239

The test above was to an address in the same wallet. Have not tried with an address in a different wallet.

Added a case to the RPC test and replicated the bug. Seems like the to address is being ignored in this case, and all funds are being returned to the change address (see the generating note for change line in the trace).

Simple bug - I had forgotten to alter the target address of the final change output, so it was going to the z-address for the first note (which gets used as the change address for intermediate change).

bitcartel · 2018-01-26T22:58:11Z

+    std::set<CBitcoinAddress> taddrs = {};
+    std::set<libzcash::PaymentAddress> zaddrs = {};
+    if (!(useAny || useAnyUTXO || useAnyNote)) {
+        UniValue outputs = params[0].get_array();


Variable name --> inputs

Changing to addresses.

str4d · 2018-02-22T13:24:18Z

Force-pushed to address @bitcartel's comments. Here is the diff:

diff --git a/src/wallet/asyncrpcoperation_mergetoaddress.cpp b/src/wallet/asyncrpcoperation_mergetoaddress.cpp
index 823f289..5257cbc 100644
--- a/src/wallet/asyncrpcoperation_mergetoaddress.cpp
+++ b/src/wallet/asyncrpcoperation_mergetoaddress.cpp
@@ -280,9 +280,6 @@ bool AsyncRPCOperation_mergetoaddress::main_impl()
 
     // Prepare raw transaction to handle JoinSplits
     CMutableTransaction mtx(tx_);
-    if (!mtx.fOverwintered) {
-        mtx.nVersion = 2;
-    }
     crypto_sign_keypair(joinSplitPubKey_.begin(), joinSplitPrivKey_);
     mtx.joinSplitPubKey = joinSplitPubKey_;
     tx_ = CTransaction(mtx);
diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
index 201b493..d5769aa 100644
--- a/src/wallet/rpcwallet.cpp
+++ b/src/wallet/rpcwallet.cpp
@@ -3786,7 +3786,7 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp)
             "                             - \"ANY_ZADDR\": Merge notes from all z-addrs belonging to the wallet.\n"
             "                         If a special string is given, any given addresses of that type will be ignored.\n"
             "    [\n"
-            "      \"address\"            (string) Can be a t-addr or a z-addr\n"
+            "      \"address\"          (string) Can be a t-addr or a z-addr\n"
             "      ,...\n"
             "    ]\n"
             "2. \"toaddress\"           (string, required) The t-addr or z-addr to send the funds to.\n"
@@ -4060,6 +4060,10 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp)
     CMutableTransaction contextualTx = CreateNewContextualCMutableTransaction(
         Params().GetConsensus(),
         chainActive.Height() + 1);
+    bool isShielded = numNotes > 0 || isToZaddr;
+    if (contextualTx.nVersion == 1 && isShielded) {
+        contextualTx.nVersion = 2; // Tx format should support vjoinsplit
+    }
 
     // Create operation and add to global queue
     std::shared_ptr<AsyncRPCQueue> q = getAsyncRPCQueue();

arielgabizon · 2018-02-22T16:05:17Z

+    size_t numNotes = noteInputs.size();
+
+    if (numUtxos == 0 && numNotes == 0) {
+        throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, "Could not find any funds to merge.");


should we add "(non-coinbase)" to clarify

It's already documented in the help text, but this will also help.

arielgabizon · 2018-02-22T16:13:26Z

+            FormatMoney(mergedValue), FormatMoney(nFee)));
+    }
+
+    // Check that the user specified fee is sane (if too high, it can result in error -25 absurd fee)


You are not exactly checking that the fee is sane as the comment says- the fee maybe sane but netAmount very small,
and the fee may be insane but nAmount larger.
Like you say, perhaps no need to replicate absurd fee logic, but maybe change comment

Actually, if you are relying anyway on error -25 to catch and crash, I'm not sure what you gain by this check.

the fee maybe sane but netAmount very small

I dispute that anyone would consider the fee to be sane if the fee is larger than the amount you are sending. If there is a genuine use-case for it, in the context of merging UTXOs, then we can address it later. For now, users should just use a smaller fee if the UTXO set they are merging has values so small that their total sum is less than 0.0002 ZEC.

Would consider adding to the comment that your goal is to catch some of the bad fees before doing many joinsplits as @bitcartel said in discussion above; otherwise it can be unclear why add this check.

str4d · 2018-02-23T00:21:13Z

@zkbot r+

zkbot · 2018-02-23T00:21:13Z

📌 Commit 2bda6a9 has been approved by str4d

zkbot · 2018-02-23T00:21:18Z

⌛ Testing commit 2bda6a9 with merge a3251992124cb392c7cc19ea14b944ef306ace80...

zkbot · 2018-02-23T01:47:12Z

💔 Test failed - pr-merge

str4d · 2018-02-23T01:49:33Z

Two builders passed
One builder timed out in the new RPC test (transient failure)
One builder failed the new RPC test (a sporadic failure I hoped I'd avoided)

I'll dig into the last of these and see if I can further reduce its likelihood.

str4d · 2018-02-23T02:19:00Z

Found and fixed a missing sync_all() in the RPC test that was likely causing a race condition.

@zkbot r+

zkbot · 2018-02-23T02:19:01Z

📌 Commit 50e65cb has been approved by str4d

zkbot · 2018-02-23T02:57:53Z

⌛ Testing commit 50e65cb3e5f32426c126bea1d6003bac11871509 with merge 6c6811d4c0b71148fba462e24578f8521131abc9...

zkbot · 2018-02-23T04:17:48Z

💔 Test failed - pr-merge

str4d · 2018-02-23T04:30:34Z

Missed a couple of sync spots. Force-pushed the final commit to squash them in.

bitcartel · 2018-02-23T04:59:49Z

@zkbot r+

zkbot · 2018-02-23T04:59:50Z

📌 Commit 15ced9b has been approved by bitcartel

zkbot · 2018-02-23T04:59:54Z

⌛ Testing commit 15ced9b with merge c5904fb...

Implement z_mergetoaddress for combining UTXOs and notes Closes #2493.

zkbot · 2018-02-23T06:27:33Z

☀️ Test successful - pr-merge
Approved by: bitcartel
Pushing c5904fb to master...

arielgabizon · 2018-02-23T10:47:38Z

+        // Consume spendable non-change notes
+        //
+        std::vector<Note> vInputNotes;
+        std::vector<SpendingKey> vInputZKeys;


What is the role of vInputZKeys? Why do they appear here and not in z_sendmany?

In z_sendmany all the notes you spend come from the same address, so you can just have a class variable store the one spending key you need. Here, every note could be from a different address, so you need their spending keys individually.

str4d added note selection and shielded tx construction I-performance Problems and improvements with respect to performance A-rpc-interface Area: RPC interface usability work-in-progress labels Dec 14, 2017

str4d added this to the 1.0.14 milestone Dec 14, 2017

str4d requested review from bitcartel and daira December 14, 2017 00:00

str4d force-pushed the 2493-active-merging branch from 9ae47af to 8a6ae80 Compare December 14, 2017 22:44

ioptio mentioned this pull request Dec 18, 2017

transaction has not been confirmed #2801

Closed

str4d removed this from the 1.0.14 milestone Dec 18, 2017

nathan-at-least mentioned this pull request Dec 20, 2017

Interim solution to "too many txins" problem #2818

Closed

str4d added this to the 1.0.15 milestone Jan 2, 2018

bitcartel reviewed Jan 8, 2018

View reviewed changes

str4d force-pushed the 2493-active-merging branch from 8a6ae80 to afcc747 Compare January 18, 2018 21:10

str4d removed the work-in-progress label Jan 18, 2018

str4d force-pushed the 2493-active-merging branch from afcc747 to b12a162 Compare January 18, 2018 21:48

str4d requested a review from bitcartel January 25, 2018 14:35

str4d self-assigned this Jan 25, 2018

bitcartel reviewed Jan 25, 2018

View reviewed changes

bitcartel reviewed Jan 26, 2018

View reviewed changes

str4d force-pushed the 2493-active-merging branch from 990d943 to 2bda6a9 Compare February 22, 2018 13:23

arielgabizon reviewed Feb 22, 2018

View reviewed changes

bitcartel mentioned this pull request Feb 23, 2018

Add an extra check for absurdly high fee before spending time creating multiple joinsplits #2981

Open

bitcartel approved these changes Feb 23, 2018

View reviewed changes

wallet_mergetoaddress: Add additional syncs to prevent race conditions

15ced9b

str4d force-pushed the 2493-active-merging branch from 50e65cb to 15ced9b Compare February 23, 2018 04:28

zkbot added a commit that referenced this pull request Feb 23, 2018

Auto merge of #2797 - str4d:2493-active-merging, r=bitcartel

c5904fb

Implement z_mergetoaddress for combining UTXOs and notes Closes #2493.

zkbot merged commit 15ced9b into zcash:master Feb 23, 2018

arielgabizon reviewed Feb 23, 2018

View reviewed changes

str4d deleted the 2493-active-merging branch February 23, 2018 11:30

mms710 mentioned this pull request Jan 24, 2019

Add rpc call to shield multiple taddrs to a zaddr #1785

Closed

cronicc mentioned this pull request Apr 4, 2022

Backport upstream improvements HorizenOfficial/zen#446

Merged


		#define JOINSPLIT_SIZE JSDescription().GetSerializeSize(SER_NETWORK, PROTOCOL_VERSION)

		UniValue z_mergetoaddress(const UniValue& params, bool fHelp)

Conversation

str4d commented Dec 14, 2017

Uh oh!

str4d commented Dec 14, 2017

Uh oh!

tromer commented Dec 14, 2017

Uh oh!

str4d commented Dec 14, 2017

Uh oh!

bitcartel commented Dec 15, 2017

Uh oh!

str4d commented Dec 18, 2017

Uh oh!

zkbot commented Dec 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

str4d commented Jan 18, 2018

Uh oh!

bitcartel left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bitcartel Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bitcartel Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bitcartel Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bitcartel Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bitcartel Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

bitcartel left a comment •

edited

Loading

bitcartel Jan 25, 2018 •

edited

Loading

bitcartel Jan 25, 2018 •

edited

Loading

bitcartel Jan 25, 2018 •

edited

Loading

bitcartel Jan 25, 2018 •

edited

Loading

bitcartel Jan 26, 2018 •

edited

Loading

arielgabizon Feb 22, 2018 •

edited

Loading

arielgabizon Feb 23, 2018 •

edited

Loading