Have z_mergetoaddress use WalletTxBuilder #6569

sellout · 2023-04-17T10:38:24Z

This also supports ZIP 317.

Fixes #2979 and fixes #5685.

nuttycom

Incomplete review, one blocking change to flush.

src/wallet/gtest/test_rpc_wallet.cpp

src/wallet/rpcwallet.cpp

src/wallet/wallet.h

Previously, `WalletTxBuilder` expected a vector of payments with known amounts. This meant that operations like `z_shieldcoinbase` and `z_mergetoaddress` needed to pre-calculate the amount to be sent to the target address. With ZIP 317 fees, this became harder to do. Now `WalletTxBuilder` accepts either a vector of payments or a single address (and optional memo) for the net amount to be paid to. This also converts `z_shieldcoinbase` to use this approach. `z_mergetoaddress` conversion is blocked by zcash#6569.

nuttycom

A few minor changes are needed. I am not using the Request changes feature because I don't want to block the PR since I'm out starting at 4am tomorrow.

src/wallet/asyncrpcoperation_mergetoaddress.cpp

nuttycom · 2023-04-18T22:32:46Z

qa/rpc-tests/remove_sprout_shielding.py

            self.nodes[0].z_mergetoaddress,
            ["ANY_TADDR", "ANY_SPROUT"], self.nodes[1].z_getnewaddress('sprout'))
-        print("[taddr, Sprout] -> Sprout z_mergetoaddress tx rejected at Canopy activation on node 0")
-
-        # Create z_mergetoaddress Sprout -> Sprout transaction on node 0. Should pass


This is no longer a condition we can exercise.

nuttycom · 2023-04-18T22:33:00Z

qa/rpc-tests/remove_sprout_shielding.py

-        self.sync_all()
-
-        # Create z_mergetoaddress Sprout -> Sprout transaction on node 1. Should pass
-        merge_tx_2 = self.nodes[1].z_mergetoaddress(["ANY_SPROUT"], n2_sprout_addr)


Also not a condition we can exercise.

src/wallet/gtest/test_rpc_wallet.cpp

src/wallet/rpcwallet.cpp

nuttycom · 2023-04-18T23:03:12Z

src/wallet/rpcwallet.cpp

+                strategy.AllowLinkingAccountAddresses());
+    } else if (allInputs.saplingNoteEntries.size() > 0) {
+        ztxoSelector = pwalletMain->ZTXOSelectorForAddress(
+                allInputs.saplingNoteEntries[0].address,


The addresses here aren't actually being used; it might be worth adding some detail to the comment above.

src/wallet/asyncrpcoperation_mergetoaddress.h

src/wallet/asyncrpcoperation_mergetoaddress.cpp

daira

flushing a comment

doc/release-notes.md

sellout · 2023-04-19T22:31:03Z

qa/rpc-tests/mergetoaddress_helper.py

+        # Merging will fail because fee is larger than `-maxtxfee`
        assert_mergetoaddress_exception(
-            "Insufficient funds, have 50.00, which is less than miners fee 999.00",
+            "Fee (999.00 ZEC) is greater than the maximum fee allowed by this instance (0.10 ZEC). Run zcashd with `-maxtxfee` to adjust this limit.",


This now tests a different case. I think the original one is still testable, but I was having trouble putting together the conditions that lead to it.

There are also a couple other tests that now test the wrong thing and still need to be updated. (E.g., things failing because they try to send to Sprout, rather than because of the condition they intend to test.)

str4d · 2023-04-20T01:29:58Z

src/wallet/wallet_tx_builder.h

@@ -403,11 +403,11 @@ class WalletTxBuilder {
    ResolveInputsAndPayments(
            CWallet& wallet,
            const ZTXOSelector& selector,
-            SpendableInputs& spendable,
+            SpendableInputs spendable,


We specifically need a copy here?

I think so. Basically, there are four paths through PrepareTransaction – fixed fee vs conventional fee and vector<Payment> vs NetAmountRecipient. ResolveInputsAndPayments is only used in the fixed fee & vector<Payment> case, and it’s the only place we modify the provided SpendableInputs. Edit: it’s also used in the conventional & vector<Payment> case, but that one makes its own copy later, which is another reason to change it like I mention at the end of this comment.

Making this copy means that we can use const SpendableInputs& in the rest of the code, which avoids incorrectly signaling that PrepareTransaction meaningfully modifies the SpendableInputs&, when the correct value is actually gotten from the TransactionEffects result.

Although we could instead make this const SpendableInputs& and explicitly copy it in the body of the method, before calling LimitToAmount, which is what does the modification. Actually, I like that better, as it preserves the const across more calls, so I’ll make that change.

Ok, I made this change as a separate commit, as I’m not 100% sold on it.

Co-authored-by: Kris Nuttycombe <kris@nutty.land> Co-authored-by: Jack Grigg <jack@z.cash>

- add ZIP 317 support - address review comments - restructure `AsyncRPCOperation_mergetoaddress` (removing the just-added `prepare`)

This change takes `const SpendableInputs&` instead of copying it. It does an explicit copy later in more limited scope, avoiding an extra copy in the conventional fee side. However, this means both the reference and mutable copy are in scope at the same time, making it easy to use the wrong one by mistake.

str4d

Reviewed ac218b6.

src/wallet/rpcwallet.cpp

src/wallet/asyncrpcoperation_mergetoaddress.cpp

src/wallet/gtest/test_rpc_wallet.cpp

src/wallet/test/rpc_wallet_tests.cpp

This removes a btest case (can’t mix Sprout and Sapling inputs in z_mergetoaddress) that is no longer testable outside the RPC interface (and there is an rpc-test for it already). It also adds a new failure case in `WalletTxBuilder::PrepareTransaction` – we now check that the fee is in `MoneyRange`. This is generally protected by `AmountFromValue` in RPC calls, but we have tests that check it at this level, and better safe than sorry.

str4d

utACK 3bf966e with a sigh about C++ variadics.

str4d · 2023-04-20T18:52:08Z

src/wallet/asyncrpcoperation_common.cpp

+                    "The provided fee, %s, is invalid. Fees must be non-negative, and no greater "
+                    "than the total amount of ZEC that will ever be available.",
+                    DisplayMoney(err.fixedFee),
+                    DisplayMoney(MAX_MONEY)));


This second argument is unused? And strprintf (tinyformat) doesn't complain? I guess it only complains if there are too few arguments...

Also this message never appears in tests.

daira · 2023-04-20T20:22:34Z

src/wallet/asyncrpcoperation_common.cpp

+                RPC_INVALID_PARAMETER,
+                strprintf(
+                    "The provided fee, %s, is invalid. Fees must be non-negative, and no greater "
+                    "than the total amount of ZEC that will ever be available.",


Suggested change

"than the total amount of ZEC that will ever be available.",

"than the total amount of ZEC that will ever be available (%s).",

daira · 2023-04-20T20:38:02Z

src/wallet/asyncrpcoperation_mergetoaddress.cpp

-        mtx.joinSplitSig);
-
-    // Sanity check
-    if (!ed25519::verify(
-        mtx.joinSplitPubKey,
-        mtx.joinSplitSig,
-        {dataToBeSigned.begin(), 32}))
-    {
-        throw std::runtime_error("ed25519_verify failed");
-    }
-
-    CTransaction rawTx(mtx);
-    tx_ = rawTx;
-
-    CDataStream ss(SER_NETWORK, PROTOCOL_VERSION);
-    ss << rawTx;
-
-    std::string encryptedNote1;
-    std::string encryptedNote2;
-    {
-        CDataStream ss2(SER_NETWORK, PROTOCOL_VERSION);
-        ss2 << ((unsigned char)0x00);
-        ss2 << jsdesc.ephemeralKey;
-        ss2 << jsdesc.ciphertexts[0];
-        ss2 << ZCJoinSplit::h_sig(jsdesc.randomSeed, jsdesc.nullifiers, joinSplitPubKey_);
-
-        encryptedNote1 = HexStr(ss2.begin(), ss2.end());
-    }
-    {
-        CDataStream ss2(SER_NETWORK, PROTOCOL_VERSION);
-        ss2 << ((unsigned char)0x01);
-        ss2 << jsdesc.ephemeralKey;
-        ss2 << jsdesc.ciphertexts[1];
-        ss2 << ZCJoinSplit::h_sig(jsdesc.randomSeed, jsdesc.nullifiers, joinSplitPubKey_);
-
-        encryptedNote2 = HexStr(ss2.begin(), ss2.end());
-    }
-
-    UniValue arrInputMap(UniValue::VARR);
-    UniValue arrOutputMap(UniValue::VARR);
-    for (size_t i = 0; i < ZC_NUM_JS_INPUTS; i++) {
-        arrInputMap.push_back(static_cast<uint64_t>(inputMap[i]));
-    }
-    for (size_t i = 0; i < ZC_NUM_JS_OUTPUTS; i++) {
-        arrOutputMap.push_back(static_cast<uint64_t>(outputMap[i]));
-    }
-
-    KeyIO keyIO(Params());
-
-    // !!! Payment disclosure START
-    size_t js_index = tx_.vJoinSplit.size() - 1;
-    uint256 placeholder;
-    for (int i = 0; i < ZC_NUM_JS_OUTPUTS; i++) {
-        uint8_t mapped_index = outputMap[i];
-        // placeholder for txid will be filled in later when tx has been finalized and signed.
-        PaymentDisclosureKey pdKey = {placeholder, js_index, mapped_index};
-        JSOutput output = outputs[mapped_index];
-        libzcash::SproutPaymentAddress zaddr = output.addr; // randomized output
-        PaymentDisclosureInfo pdInfo = {PAYMENT_DISCLOSURE_VERSION_EXPERIMENTAL, esk, joinSplitPrivKey_, zaddr};
-        paymentDisclosureData_.push_back(PaymentDisclosureKeyInfo(pdKey, pdInfo));
-
-        LogPrint("paymentdisclosure", "%s: Payment Disclosure: js=%d, n=%d, zaddr=%s\n", getId(), js_index, int(mapped_index), keyIO.EncodePaymentAddress(zaddr));
-    }
-    // !!! Payment disclosure END
-
-    UniValue obj(UniValue::VOBJ);
-    obj.pushKV("encryptednote1", encryptedNote1);
-    obj.pushKV("encryptednote2", encryptedNote2);
-    obj.pushKV("rawtxn", HexStr(ss.begin(), ss.end()));
-    obj.pushKV("inputmap", arrInputMap);
-    obj.pushKV("outputmap", arrOutputMap);
-    return obj;
-}
-
-std::array<unsigned char, ZC_MEMO_SIZE> AsyncRPCOperation_mergetoaddress::get_memo_from_hex_string(std::string s)
-{
-    std::array<unsigned char, ZC_MEMO_SIZE> memo = {{0x00}};
-
-    std::vector<unsigned char> rawMemo = ParseHex(s.c_str());
-
-    // If ParseHex comes across a non-hex char, it will stop but still return results so far.
-    size_t slen = s.length();
-    if (slen % 2 != 0 || (slen > 0 && rawMemo.size() != slen / 2)) {
-        throw JSONRPCError(RPC_INVALID_PARAMETER, "Memo must be in hexadecimal format");
-    }
-
-    if (rawMemo.size() > ZC_MEMO_SIZE) {
-        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Memo size of %d is too big, maximum allowed is %d", rawMemo.size(), ZC_MEMO_SIZE));
-    }
-
-    // copy vector into boost array
-    int lenMemo = rawMemo.size();
-    for (int i = 0; i < ZC_MEMO_SIZE && i < lenMemo; i++) {
-        memo[i] = rawMemo[i];
+            LogPrint("zrpcunsafe", "%s: spending %s to send %s with fee %s\n",
+                id,
+                FormatMoney(payments.Total()),
+                FormatMoney(spendable.Total()),
+                FormatMoney(effects.GetFee()));
+        }


Consider:

bool fullyTransparent = spendable.sproutNoteEntries.empty() && spendable.saplingNoteEntries.empty() && spendable.orchardNoteMetadata.empty() && !sendsToShielded; LogPrint(fullyTransparent ? "zrpc" : "zrpcunsafe", "%s: spending %s to send %s with fee %s\n", id, FormatMoney(payments.Total()), FormatMoney(spendable.Total()), FormatMoney(effects.GetFee()));

daira · 2023-04-20T21:15:53Z

src/wallet/gtest/test_rpc_wallet.cpp

-    }
+    WalletTxBuilder builder(Params(), minRelayTxFee);
+    auto selector = CWallet::LegacyTransparentZTXOSelector(
+            true,


Suggested change

true,

true, // only select inputs we can spend

daira · 2023-04-20T21:22:54Z

src/wallet/rpcwallet.cpp

@@ -5409,10 +5430,10 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp)

    // Keep track of addresses to spot duplicates
    std::set<std::string> setAddress;
+    std::set<ReceiverType> receiverTypes;


Remove receiverTypes since it isn't used.

daira · 2023-04-20T21:31:00Z

src/wallet/rpcwallet.cpp

@@ -5576,9 +5574,7 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp)

    unsigned int max_tx_size = saplingActive ? MAX_TX_SIZE_AFTER_SAPLING : MAX_TX_SIZE_BEFORE_SAPLING;
    size_t estimatedTxSize = 200;  // tx overhead + wiggle room


Suggested change

size_t estimatedTxSize = 200; // tx overhead + wiggle room

size_t estimatedTxSize = MIN_TX_COST; // tx overhead + wiggle room

No. We still don't support UAs in z_mergetoaddress, and the size of the Sprout or Sapling shielded output is accounted for right after this.

daira · 2023-04-20T21:31:58Z

src/wallet/rpcwallet.cpp

+    if (isToSaplingZaddr) {
        estimatedTxSize += OUTPUTDESCRIPTION_SIZE;
    }


Suggested change

if (isToSaplingZaddr) {

estimatedTxSize += OUTPUTDESCRIPTION_SIZE;

}

daira · 2023-04-20T21:34:46Z

src/wallet/rpcwallet.cpp

-                RPC_INVALID_PARAMETER, "Cannot create shielded transactions before Sapling has activated");
-        }
+    // The privacy policy is determined early so as to be able to use it
+    // for selector construction.


Explain the LegacyCompat case.

daira

utACK with suggestions.

sellout added the safe-to-build Used to send PR to prod CI environment label Apr 17, 2023

sellout added this to the Release 5.5.0 milestone Apr 17, 2023

sellout requested review from nuttycom, daira and str4d April 17, 2023 10:38

ECC-CI removed the safe-to-build Used to send PR to prod CI environment label Apr 17, 2023

sellout force-pushed the wallet_tx_builder-z_mergetoaddress branch from 6f5146f to eea5084 Compare April 17, 2023 15:06

nuttycom requested changes Apr 17, 2023

View reviewed changes

src/wallet/gtest/test_rpc_wallet.cpp Outdated Show resolved Hide resolved

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved

src/wallet/rpcwallet.cpp Show resolved Hide resolved

src/wallet/wallet.h Outdated Show resolved Hide resolved

nuttycom force-pushed the wallet_tx_builder-z_mergetoaddress branch from eea5084 to a2679ae Compare April 18, 2023 15:29

nuttycom added the A-wallet Area: Wallet label Apr 18, 2023

nuttycom force-pushed the wallet_tx_builder-z_mergetoaddress branch 4 times, most recently from ff0fdca to 16815e0 Compare April 18, 2023 18:33

sellout mentioned this pull request Apr 18, 2023

WalletTxBuilder support for net payments #6576

Merged

nuttycom force-pushed the wallet_tx_builder-z_mergetoaddress branch 2 times, most recently from b60bb03 to 186b40a Compare April 18, 2023 20:55

nuttycom added A-rpc-interface Area: RPC interface safe-to-build Used to send PR to prod CI environment labels Apr 18, 2023

nuttycom force-pushed the wallet_tx_builder-z_mergetoaddress branch from 186b40a to 218169e Compare April 18, 2023 21:42

nuttycom self-requested a review April 18, 2023 21:42

nuttycom marked this pull request as ready for review April 18, 2023 21:42

nuttycom added safe-to-build Used to send PR to prod CI environment and removed safe-to-build Used to send PR to prod CI environment labels Apr 18, 2023

nuttycom force-pushed the wallet_tx_builder-z_mergetoaddress branch from 218169e to ae799f0 Compare April 18, 2023 21:47

ECC-CI removed the safe-to-build Used to send PR to prod CI environment label Apr 18, 2023

nuttycom reviewed Apr 19, 2023

View reviewed changes

nuttycom force-pushed the wallet_tx_builder-z_mergetoaddress branch from ae799f0 to 705b17b Compare April 19, 2023 01:59

daira reviewed Apr 19, 2023

View reviewed changes

doc/release-notes.md Outdated Show resolved Hide resolved

sellout force-pushed the wallet_tx_builder-z_mergetoaddress branch from 705b17b to bab7d9c Compare April 19, 2023 22:28

sellout commented Apr 19, 2023

View reviewed changes

sellout added the safe-to-build Used to send PR to prod CI environment label Apr 19, 2023

ECC-CI removed the safe-to-build Used to send PR to prod CI environment label Apr 20, 2023

str4d reviewed Apr 20, 2023

View reviewed changes

sellout and others added 3 commits April 19, 2023 22:26

Update z_mergetoaddress to use WalletTxBuilder

94f5bfc

Co-authored-by: Kris Nuttycombe <kris@nutty.land> Co-authored-by: Jack Grigg <jack@z.cash>

Many z_mergetoaddress updates

d0522df

- add ZIP 317 support - address review comments - restructure `AsyncRPCOperation_mergetoaddress` (removing the just-added `prepare`)

sellout force-pushed the wallet_tx_builder-z_mergetoaddress branch from bab7d9c to ac218b6 Compare April 20, 2023 07:33

sellout added the safe-to-build Used to send PR to prod CI environment label Apr 20, 2023

ECC-CI removed the safe-to-build Used to send PR to prod CI environment label Apr 20, 2023

str4d requested changes Apr 20, 2023

View reviewed changes

sellout added the safe-to-build Used to send PR to prod CI environment label Apr 20, 2023

ECC-CI removed the safe-to-build Used to send PR to prod CI environment label Apr 20, 2023

str4d approved these changes Apr 20, 2023

View reviewed changes

daira reviewed Apr 20, 2023

View reviewed changes

daira approved these changes Apr 20, 2023

View reviewed changes

str4d merged commit 555e085 into zcash:master Apr 20, 2023

daira mentioned this pull request Apr 21, 2023

Ecosystem-wide standard transaction fee #2942

Closed

str4d mentioned this pull request Apr 25, 2023

Support UA destinations in z_mergetoaddress #6605

Merged

sellout deleted the wallet_tx_builder-z_mergetoaddress branch May 3, 2023 07:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Have z_mergetoaddress use WalletTxBuilder #6569

Have z_mergetoaddress use WalletTxBuilder #6569

sellout commented Apr 17, 2023 •

edited

Loading

nuttycom left a comment

nuttycom left a comment

nuttycom Apr 18, 2023

nuttycom Apr 18, 2023

nuttycom Apr 18, 2023

daira left a comment

sellout Apr 19, 2023

str4d Apr 20, 2023

sellout Apr 20, 2023 •

edited

Loading

sellout Apr 20, 2023

str4d left a comment

str4d left a comment

str4d Apr 20, 2023

daira Apr 20, 2023

daira Apr 20, 2023

daira Apr 20, 2023 •

edited

Loading

daira Apr 20, 2023

daira Apr 20, 2023

daira Apr 20, 2023

str4d Apr 20, 2023

daira Apr 20, 2023

daira Apr 20, 2023

daira left a comment

	"than the total amount of ZEC that will ever be available.",
	"than the total amount of ZEC that will ever be available (%s).",

		@@ -5576,9 +5574,7 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp)

		unsigned int max_tx_size = saplingActive ? MAX_TX_SIZE_AFTER_SAPLING : MAX_TX_SIZE_BEFORE_SAPLING;
		size_t estimatedTxSize = 200; // tx overhead + wiggle room

	size_t estimatedTxSize = 200; // tx overhead + wiggle room
	size_t estimatedTxSize = MIN_TX_COST; // tx overhead + wiggle room

	if (isToSaplingZaddr) {
	estimatedTxSize += OUTPUTDESCRIPTION_SIZE;
	}

Have z_mergetoaddress use WalletTxBuilder #6569

Have z_mergetoaddress use WalletTxBuilder #6569

Conversation

sellout commented Apr 17, 2023 • edited Loading

nuttycom left a comment

Choose a reason for hiding this comment

nuttycom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sellout Apr 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

str4d left a comment

Choose a reason for hiding this comment

str4d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daira Apr 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daira left a comment

Choose a reason for hiding this comment

sellout commented Apr 17, 2023 •

edited

Loading

sellout Apr 20, 2023 •

edited

Loading

daira Apr 20, 2023 •

edited

Loading