From f655f482ab5819834d489b3b48adfe57681b4352 Mon Sep 17 00:00:00 2001 From: Greg Pfeil Date: Tue, 18 Apr 2023 11:48:03 -0600 Subject: [PATCH] WalletTxBuilder support for net payments 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 #6569. --- qa/rpc-tests/wallet_shieldcoinbase.py | 5 +- .../asyncrpcoperation_shieldcoinbase.cpp | 18 +- src/wallet/asyncrpcoperation_shieldcoinbase.h | 4 +- src/wallet/rpcwallet.cpp | 18 +- src/wallet/wallet_tx_builder.cpp | 250 ++++++++++++------ src/wallet/wallet_tx_builder.h | 8 +- 6 files changed, 194 insertions(+), 109 deletions(-) diff --git a/qa/rpc-tests/wallet_shieldcoinbase.py b/qa/rpc-tests/wallet_shieldcoinbase.py index c6e88e237..80a7a3a02 100755 --- a/qa/rpc-tests/wallet_shieldcoinbase.py +++ b/qa/rpc-tests/wallet_shieldcoinbase.py @@ -9,6 +9,7 @@ from test_framework.util import assert_equal, initialize_chain_clean, \ start_node, connect_nodes_bi, sync_blocks, sync_mempools, \ wait_and_assert_operationid_status, get_coinbase_address, DEFAULT_FEE, \ NU5_BRANCH_ID, nuparams +from test_framework.zip317 import conventional_fee from decimal import Decimal @@ -117,7 +118,7 @@ class WalletShieldCoinbaseTest (BitcoinTestFramework): # Confirm balances and that do_not_shield_taddr containing funds of 10 was left alone assert_equal(self.nodes[0].getbalance(), 10) assert_equal(self.nodes[0].z_getbalance(do_not_shield_taddr), Decimal('10.0')) - self.test_check_balance_zaddr(self.nodes[0], Decimal('40.0') - DEFAULT_FEE) + self.test_check_balance_zaddr(self.nodes[0], Decimal('40.0') - conventional_fee(4)) assert_equal(self.nodes[1].getbalance(), 20) assert_equal(self.nodes[2].getbalance(), 30) @@ -129,7 +130,7 @@ class WalletShieldCoinbaseTest (BitcoinTestFramework): self.sync_all() assert_equal(self.nodes[0].getbalance(), 10) - self.test_check_balance_zaddr(self.nodes[0], Decimal('70.0') - DEFAULT_FEE) + self.test_check_balance_zaddr(self.nodes[0], Decimal('70.0') - conventional_fee(4)) assert_equal(self.nodes[1].getbalance(), 30) assert_equal(self.nodes[2].getbalance(), 0) diff --git a/src/wallet/asyncrpcoperation_shieldcoinbase.cpp b/src/wallet/asyncrpcoperation_shieldcoinbase.cpp index 41391390d..b1a85d209 100644 --- a/src/wallet/asyncrpcoperation_shieldcoinbase.cpp +++ b/src/wallet/asyncrpcoperation_shieldcoinbase.cpp @@ -47,7 +47,7 @@ AsyncRPCOperation_shieldcoinbase::AsyncRPCOperation_shieldcoinbase( PaymentAddress toAddress, TransactionStrategy strategy, int nUTXOLimit, - CAmount fee, + std::optional fee, UniValue contextInfo) : builder_(std::move(builder)), ztxoSelector_(ztxoSelector), @@ -57,7 +57,7 @@ AsyncRPCOperation_shieldcoinbase::AsyncRPCOperation_shieldcoinbase( fee_(fee), contextinfo_(contextInfo) { - assert(MoneyRange(fee)); + assert(!fee.has_value() || MoneyRange(fee.value())); assert(ztxoSelector.RequireSpendingKeys()); examine(toAddress_, match { @@ -177,23 +177,11 @@ Remaining AsyncRPCOperation_shieldcoinbase::prepare(CWallet& wallet) { spendable.LimitTransparentUtxos(numUtxos); - // TODO: Don’t check this outside of `PrepareTransaction` - if (shieldingValue < fee_) { - ThrowInputSelectionError( - InvalidFundsError(shieldingValue, InsufficientFundsError(fee_)), - ztxoSelector_, - strategy_); - } - - // FIXME: This should be internal, and shouldn’t be a payment, but more like a change address, - // as we don’t know the amount at this point. - std::vector payments = { Payment(toAddress_, shieldingValue - fee_, std::nullopt) }; - auto preparationResult = builder_.PrepareTransaction( wallet, ztxoSelector_, spendable, - payments, + std::make_pair(toAddress_, std::nullopt), chainActive, strategy_, fee_, diff --git a/src/wallet/asyncrpcoperation_shieldcoinbase.h b/src/wallet/asyncrpcoperation_shieldcoinbase.h index bde3890e4..390749291 100644 --- a/src/wallet/asyncrpcoperation_shieldcoinbase.h +++ b/src/wallet/asyncrpcoperation_shieldcoinbase.h @@ -62,7 +62,7 @@ public: PaymentAddress toAddress, TransactionStrategy strategy, int nUTXOLimit, - CAmount fee = DEFAULT_FEE, + std::optional fee, UniValue contextInfo = NullUniValue); virtual ~AsyncRPCOperation_shieldcoinbase(); @@ -88,7 +88,7 @@ private: PaymentAddress toAddress_; TransactionStrategy strategy_; int nUTXOLimit_; - CAmount fee_; + std::optional fee_; std::optional effects_; UniValue contextinfo_; // optional data to include in return value from getStatus() diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index da3db9c9b..86e66dd0b 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -5188,8 +5188,8 @@ UniValue z_shieldcoinbase(const UniValue& params, bool fHelp) "\nArguments:\n" "1. \"fromaddress\" (string, required) The address is a taddr or \"*\" for all taddrs belonging to the wallet.\n" "2. \"toaddress\" (string, required) The address is a zaddr.\n" - "3. fee (numeric, optional, default=" - + strprintf("%s", FormatMoney(DEFAULT_FEE)) + ") The fee amount to attach to this transaction.\n" + "3. fee (numeric, optional, default=null) The fee amount in " + CURRENCY_UNIT + " to attach to this transaction. The default behavior\n" + " is to use a fee calculated according to ZIP 317.\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 as many as will fit in the transaction.\n" "5. privacyPolicy (string, optional, default=\"AllowRevealedSenders\") Policy for what information leakage is acceptable.\n" @@ -5290,13 +5290,9 @@ UniValue z_shieldcoinbase(const UniValue& params, bool fHelp) throw JSONRPCError(RPC_INVALID_PARAMETER, string("Invalid parameter, unknown address format: ") + destStr); } - CAmount nFee = DEFAULT_FEE; - if (params.size() > 2) { - if (params[2].get_real() == 0.0) { - nFee = 0; - } else { - nFee = AmountFromValue( params[2] ); - } + std::optional nFee; + if (params.size() > 2 && !params[2].isNull()) { + nFee = AmountFromValue( params[2] ); } int nUTXOLimit = params.size() > 3 ? params[3].get_int() : SHIELD_COINBASE_DEFAULT_LIMIT; @@ -5308,7 +5304,9 @@ UniValue z_shieldcoinbase(const UniValue& params, bool fHelp) UniValue contextInfo(UniValue::VOBJ); contextInfo.pushKV("fromaddress", params[0]); contextInfo.pushKV("toaddress", params[1]); - contextInfo.pushKV("fee", ValueFromAmount(nFee)); + if (nFee.has_value()) { + contextInfo.pushKV("fee", ValueFromAmount(nFee.value())); + } // Create the wallet builder WalletTxBuilder builder(chainparams, minRelayTxFee); diff --git a/src/wallet/wallet_tx_builder.cpp b/src/wallet/wallet_tx_builder.cpp index fb992f663..91020fffc 100644 --- a/src/wallet/wallet_tx_builder.cpp +++ b/src/wallet/wallet_tx_builder.cpp @@ -14,6 +14,66 @@ int GetAnchorHeight(const CChain& chain, uint32_t anchorConfirmations) return nextBlockHeight - anchorConfirmations; } +static tl::expected +ValidateAmount(const SpendableInputs& spendable, const CAmount& fee) +{ + // TODO: The actual requirement should probably be higher than simply `fee` – do we need to + // take into account the dustThreshold when adding an output? But, this was the + // pre-WalletTxBuilder behavior, so it’s fine to maintain it for now. + auto targetAmount = fee; + if (spendable.Total() < targetAmount) + return tl::make_unexpected(ReportInvalidFunds(spendable, false, fee, 0, targetAmount, 0)); + else + return {}; +} + +static tl::expected, InputSelectionError> +ResolveNetPayment( + const ZTXOSelector& selector, + const SpendableInputs& spendable, + const NetAmountRecipient& netpay, + const std::optional& fee, + const TransactionStrategy& strategy, + bool afterNU5) +{ + bool canResolveOrchard = afterNU5 && !selector.SelectsSprout(); + CAmount maxSaplingAvailable = spendable.GetSaplingTotal(); + CAmount maxOrchardAvailable = spendable.GetOrchardTotal(); + uint32_t orchardOutputs{0}; + + // We initially resolve the payment with `MINIMUM_FEE` so that we can use the payment to + // calculate the actual fee. + auto initialFee = fee.value_or(MINIMUM_FEE); + return ValidateAmount(spendable, initialFee) + .and_then([&](void) { + return ResolvePayment( + Payment(netpay.first, spendable.Total() - initialFee, netpay.second), + canResolveOrchard, + strategy, + maxSaplingAvailable, + maxOrchardAvailable, + orchardOutputs) + .map_error([](const auto& error) -> InputSelectionError { return error; }) + .and_then([&](const auto& rpayment) { + auto finalFee = fee.value_or(CalcZIP317Fee(spendable, {rpayment}, std::nullopt)); + return ValidateAmount(spendable, finalFee) + .and_then([&](void) { + return ResolvePayment( + Payment(netpay.first, spendable.Total() - finalFee, netpay.second), + canResolveOrchard, + strategy, + maxSaplingAvailable, + maxOrchardAvailable, + orchardOutputs) + .map([&](const auto& actualPayment) { + return std::make_pair(actualPayment, finalFee); + }) + .map_error([](const auto& error) -> InputSelectionError { return error; }); + }); + }); + }); +} + tl::expected WalletTxBuilder::GetChangeAddress( CWallet& wallet, @@ -144,7 +204,7 @@ WalletTxBuilder::PrepareTransaction( CWallet& wallet, const ZTXOSelector& selector, SpendableInputs& spendable, - const std::vector& payments, + const Recipients& payments, const CChain& chain, TransactionStrategy strategy, std::optional fee, @@ -156,7 +216,26 @@ WalletTxBuilder::PrepareTransaction( int anchorHeight = GetAnchorHeight(chain, anchorConfirmations); bool afterNU5 = params.GetConsensus().NetworkUpgradeActive(anchorHeight, Consensus::UPGRADE_NU5); - auto selected = ResolveInputsAndPayments(wallet, selector, spendable, payments, chain, strategy, fee, afterNU5); + auto selected = examine(payments, match { + [&](const std::vector& payments) { + return ResolveInputsAndPayments( + wallet, + selector, + spendable, + payments, + chain, + strategy, + fee, + afterNU5); + }, + [&](const NetAmountRecipient& netRecipient) { + return ResolveNetPayment(selector, spendable, netRecipient, fee, strategy, afterNU5) + .map([&](const auto& pair) { + const auto& [payment, finalFee] = pair; + return InputSelection(spendable, {{payment}}, finalFee, std::nullopt); + }); + }, + }); return selected.map([&](const InputSelection& resolvedSelection) { auto ovks = SelectOVKs(wallet, selector, spendable); @@ -446,6 +525,92 @@ WalletTxBuilder::IterateLimit( return std::make_tuple(spendableMut, updatedFee, changeAddr); } +static tl::expected +ResolvePayment( + const Payment& payment, + bool canResolveOrchard, + const TransactionStrategy& strategy, + CAmount& maxSaplingAvailable, + CAmount& maxOrchardAvailable, + uint32_t& orchardOutputs) +{ + return examine(payment.GetAddress(), match { + [&](const CKeyID& p2pkh) -> tl::expected { + if (strategy.AllowRevealedRecipients()) { + return {{std::nullopt, p2pkh, payment.GetAmount(), payment.GetMemo(), false}}; + } else { + return tl::make_unexpected(AddressResolutionError::TransparentRecipientNotAllowed); + } + }, + [&](const CScriptID& p2sh) -> tl::expected { + if (strategy.AllowRevealedRecipients()) { + return {{std::nullopt, p2sh, payment.GetAmount(), payment.GetMemo(), false}}; + } else { + return tl::make_unexpected(AddressResolutionError::TransparentRecipientNotAllowed); + } + }, + [&](const SproutPaymentAddress&) -> tl::expected { + return tl::make_unexpected(AddressResolutionError::SproutRecipientsNotSupported); + }, + [&](const SaplingPaymentAddress& addr) + -> tl::expected { + if (strategy.AllowRevealedAmounts() || payment.GetAmount() <= maxSaplingAvailable) { + if (!strategy.AllowRevealedAmounts()) { + maxSaplingAvailable -= payment.GetAmount(); + } + return {{std::nullopt, addr, payment.GetAmount(), payment.GetMemo(), false}}; + } else { + return tl::make_unexpected(AddressResolutionError::RevealingSaplingAmountNotAllowed); + } + }, + [&](const UnifiedAddress& ua) -> tl::expected { + if (canResolveOrchard + && ua.GetOrchardReceiver().has_value() + && (strategy.AllowRevealedAmounts() || payment.GetAmount() <= maxOrchardAvailable) + ) { + if (!strategy.AllowRevealedAmounts()) { + maxOrchardAvailable -= payment.GetAmount(); + } + orchardOutputs += 1; + return {{ + ua, + ua.GetOrchardReceiver().value(), + payment.GetAmount(), + payment.GetMemo(), + false + }}; + } else if (ua.GetSaplingReceiver().has_value() + && (strategy.AllowRevealedAmounts() || payment.GetAmount() <= maxSaplingAvailable) + ) { + if (!strategy.AllowRevealedAmounts()) { + maxSaplingAvailable -= payment.GetAmount(); + } + return {{ua, ua.GetSaplingReceiver().value(), payment.GetAmount(), payment.GetMemo(), false}}; + } else { + if (strategy.AllowRevealedRecipients()) { + if (ua.GetP2SHReceiver().has_value()) { + return {{ + ua, ua.GetP2SHReceiver().value(), payment.GetAmount(), std::nullopt, false}}; + } else if (ua.GetP2PKHReceiver().has_value()) { + return {{ + ua, ua.GetP2PKHReceiver().value(), payment.GetAmount(), std::nullopt, false}}; + } else { + // This should only occur when we have + // • an Orchard-only UA, + // • `AllowRevealedRecipients`, and + // • can’t resolve Orchard (which means either a Sprout selector or pre-NU5). + return tl::make_unexpected(AddressResolutionError::CouldNotResolveReceiver); + } + } else if (strategy.AllowRevealedAmounts()) { + return tl::make_unexpected(AddressResolutionError::TransparentReceiverNotAllowed); + } else { + return tl::make_unexpected(AddressResolutionError::RevealingReceiverAmountsNotAllowed); + } + } + } + }); +} + tl::expected WalletTxBuilder::ResolveInputsAndPayments( CWallet& wallet, @@ -474,83 +639,10 @@ WalletTxBuilder::ResolveInputsAndPayments( std::vector resolvedPayments; std::optional resolutionError; for (const auto& payment : payments) { - examine(payment.GetAddress(), match { - [&](const CKeyID& p2pkh) { - if (strategy.AllowRevealedRecipients()) { - resolvedPayments.emplace_back( - std::nullopt, p2pkh, payment.GetAmount(), payment.GetMemo(), false); - } else { - resolutionError = AddressResolutionError::TransparentRecipientNotAllowed; - } - }, - [&](const CScriptID& p2sh) { - if (strategy.AllowRevealedRecipients()) { - resolvedPayments.emplace_back( - std::nullopt, p2sh, payment.GetAmount(), payment.GetMemo(), false); - } else { - resolutionError = AddressResolutionError::TransparentRecipientNotAllowed; - } - }, - [&](const SproutPaymentAddress&) { - resolutionError = AddressResolutionError::SproutRecipientsNotSupported; - }, - [&](const SaplingPaymentAddress& addr) { - if (strategy.AllowRevealedAmounts() || payment.GetAmount() <= maxSaplingAvailable) { - resolvedPayments.emplace_back( - std::nullopt, addr, payment.GetAmount(), payment.GetMemo(), false); - if (!strategy.AllowRevealedAmounts()) { - maxSaplingAvailable -= payment.GetAmount(); - } - } else { - resolutionError = AddressResolutionError::RevealingSaplingAmountNotAllowed; - } - }, - [&](const UnifiedAddress& ua) { - if (canResolveOrchard - && ua.GetOrchardReceiver().has_value() - && (strategy.AllowRevealedAmounts() || payment.GetAmount() <= maxOrchardAvailable) - ) { - resolvedPayments.emplace_back( - ua, ua.GetOrchardReceiver().value(), payment.GetAmount(), payment.GetMemo(), false); - if (!strategy.AllowRevealedAmounts()) { - maxOrchardAvailable -= payment.GetAmount(); - } - orchardOutputs += 1; - } else if (ua.GetSaplingReceiver().has_value() - && (strategy.AllowRevealedAmounts() || payment.GetAmount() <= maxSaplingAvailable) - ) { - resolvedPayments.emplace_back( - ua, ua.GetSaplingReceiver().value(), payment.GetAmount(), payment.GetMemo(), false); - if (!strategy.AllowRevealedAmounts()) { - maxSaplingAvailable -= payment.GetAmount(); - } - } else { - if (strategy.AllowRevealedRecipients()) { - if (ua.GetP2SHReceiver().has_value()) { - resolvedPayments.emplace_back( - ua, ua.GetP2SHReceiver().value(), payment.GetAmount(), std::nullopt, false); - } else if (ua.GetP2PKHReceiver().has_value()) { - resolvedPayments.emplace_back( - ua, ua.GetP2PKHReceiver().value(), payment.GetAmount(), std::nullopt, false); - } else { - // This should only occur when we have - // • an Orchard-only UA, - // • `AllowRevealedRecipients`, and - // • can’t resolve Orchard (which means either insufficient non-Sprout - // funds or pre-NU5). - resolutionError = AddressResolutionError::CouldNotResolveReceiver; - } - } else if (strategy.AllowRevealedAmounts()) { - resolutionError = AddressResolutionError::TransparentReceiverNotAllowed; - } else { - resolutionError = AddressResolutionError::RevealingReceiverAmountsNotAllowed; - } - } - } - }); - - if (resolutionError.has_value()) { - return tl::make_unexpected(resolutionError.value()); + auto res = ResolvePayment(payment, canResolveOrchard, strategy, maxSaplingAvailable, maxOrchardAvailable, orchardOutputs); + res.map([&](const ResolvedPayment& rpayment) { resolvedPayments.emplace_back(rpayment); }); + if (!res.has_value()) { + return tl::make_unexpected(res.error()); } } auto resolved = Payments(resolvedPayments); diff --git a/src/wallet/wallet_tx_builder.h b/src/wallet/wallet_tx_builder.h index 4a629ca57..9a644287b 100644 --- a/src/wallet/wallet_tx_builder.h +++ b/src/wallet/wallet_tx_builder.h @@ -65,6 +65,12 @@ public: } }; +typedef std::pair> NetAmountRecipient; + +typedef std::variant< + std::vector, + NetAmountRecipient> Recipients; + class Payments { private: std::vector payments; @@ -426,7 +432,7 @@ public: CWallet& wallet, const ZTXOSelector& selector, SpendableInputs& spendable, - const std::vector& payments, + const Recipients& payments, const CChain& chain, TransactionStrategy strategy, /// A fixed fee is used if provided, otherwise it is calculated based on ZIP 317.