From effbc332761d2869ae8e133d678b89599f0b9080 Mon Sep 17 00:00:00 2001 From: Greg Pfeil Date: Wed, 22 Mar 2023 15:34:18 -0600 Subject: [PATCH] Remove CWallet member from WalletTxBuilder This resolves a conflict where most usage is `const`, but some modifies the wallet. Previously it held a const member and then used `pwalletMain` directly for the mutating calls. This now passes `CWallet` explicitly where necessary, using `const` when possible. This also benefits a follow-up PR (#6408) that introduces locking, which also mutates the wallet. --- src/wallet/asyncrpcoperation_sendmany.cpp | 3 ++- src/wallet/gtest/test_rpc_wallet.cpp | 2 +- src/wallet/rpcwallet.cpp | 2 +- src/wallet/test/rpc_wallet_tests.cpp | 4 ++-- src/wallet/wallet_tx_builder.cpp | 16 ++++++++++------ src/wallet/wallet_tx_builder.h | 9 ++++++--- 6 files changed, 22 insertions(+), 14 deletions(-) diff --git a/src/wallet/asyncrpcoperation_sendmany.cpp b/src/wallet/asyncrpcoperation_sendmany.cpp index 7b2db90b9..06ddfa405 100644 --- a/src/wallet/asyncrpcoperation_sendmany.cpp +++ b/src/wallet/asyncrpcoperation_sendmany.cpp @@ -138,9 +138,10 @@ void AsyncRPCOperation_sendmany::main() { // // At least #4 differs from the Rust transaction builder. uint256 AsyncRPCOperation_sendmany::main_impl() { - auto spendable = builder_.FindAllSpendableInputs(ztxoSelector_, mindepth_); + auto spendable = builder_.FindAllSpendableInputs(*pwalletMain, ztxoSelector_, mindepth_); auto preparedTx = builder_.PrepareTransaction( + *pwalletMain, ztxoSelector_, spendable, recipients_, diff --git a/src/wallet/gtest/test_rpc_wallet.cpp b/src/wallet/gtest/test_rpc_wallet.cpp index 842d5e134..716654c31 100644 --- a/src/wallet/gtest/test_rpc_wallet.cpp +++ b/src/wallet/gtest/test_rpc_wallet.cpp @@ -284,7 +284,7 @@ TEST(WalletRPCTests, RPCZsendmanyTaddrToSapling) pwalletMain->LoadWalletTx(wtx); // Context that z_sendmany requires - auto builder = WalletTxBuilder(Params(), *pwalletMain, minRelayTxFee); + auto builder = WalletTxBuilder(Params(), minRelayTxFee); mtx = CreateNewContextualCMutableTransaction(consensusParams, nextBlockHeight, false); // we need AllowFullyTransparent because the transaction will result diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 32144fb04..f2156ee93 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -5016,7 +5016,7 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) // Create operation and add to global queue auto nAnchorDepth = std::min((unsigned int) nMinDepth, nAnchorConfirmations); - WalletTxBuilder builder(Params(), *pwalletMain, minRelayTxFee); + WalletTxBuilder builder(Params(), minRelayTxFee); std::shared_ptr q = getAsyncRPCQueue(); std::shared_ptr operation( diff --git a/src/wallet/test/rpc_wallet_tests.cpp b/src/wallet/test/rpc_wallet_tests.cpp index 9cfa8d1a1..58583c556 100644 --- a/src/wallet/test/rpc_wallet_tests.cpp +++ b/src/wallet/test/rpc_wallet_tests.cpp @@ -1248,7 +1248,7 @@ BOOST_AUTO_TEST_CASE(rpc_z_sendmany_internals) // confirm this. TransparentCoinbasePolicy::Allow, false).value(); - WalletTxBuilder builder(Params(), *pwalletMain, minRelayTxFee); + WalletTxBuilder builder(Params(), minRelayTxFee); std::vector recipients = { Payment(zaddr1, 100*COIN, Memo::FromHexOrThrow("DEADBEEF")) }; TransactionStrategy strategy(PrivacyPolicy::AllowRevealedSenders); std::shared_ptr operation(new AsyncRPCOperation_sendmany(std::move(builder), selector, recipients, 1, 1, strategy)); @@ -1265,7 +1265,7 @@ BOOST_AUTO_TEST_CASE(rpc_z_sendmany_internals) true, TransparentCoinbasePolicy::Disallow, false).value(); - WalletTxBuilder builder(Params(), *pwalletMain, minRelayTxFee); + WalletTxBuilder builder(Params(), minRelayTxFee); std::vector recipients = { Payment(taddr1, 100*COIN, Memo::FromHexOrThrow("DEADBEEF")) }; TransactionStrategy strategy(PrivacyPolicy::AllowRevealedRecipients); std::shared_ptr operation(new AsyncRPCOperation_sendmany(std::move(builder), selector, recipients, 1, 1, strategy)); diff --git a/src/wallet/wallet_tx_builder.cpp b/src/wallet/wallet_tx_builder.cpp index 55f480cd0..65b0b4334 100644 --- a/src/wallet/wallet_tx_builder.cpp +++ b/src/wallet/wallet_tx_builder.cpp @@ -13,6 +13,7 @@ int GetAnchorHeight(const CChain& chain, uint32_t anchorConfirmations) } PrepareTransactionResult WalletTxBuilder::PrepareTransaction( + CWallet& wallet, const ZTXOSelector& selector, SpendableInputs& spendable, const std::vector& payments, @@ -24,7 +25,7 @@ PrepareTransactionResult WalletTxBuilder::PrepareTransaction( assert(fee < MAX_MONEY); int anchorHeight = GetAnchorHeight(chain, anchorConfirmations); - auto selected = ResolveInputsAndPayments(selector, spendable, payments, chain, strategy, fee, anchorHeight); + auto selected = ResolveInputsAndPayments(wallet, selector, spendable, payments, chain, strategy, fee, anchorHeight); if (std::holds_alternative(selected)) { return std::get(selected); } @@ -81,7 +82,7 @@ PrepareTransactionResult WalletTxBuilder::PrepareTransaction( auto changeAddressForTransparentSelector = [&](const std::set& receiverTypes) { return addChangePayment( - pwalletMain->GenerateChangeAddressForAccount( + wallet.GenerateChangeAddressForAccount( sendFromAccount, getAllowedChangePools(receiverTypes))); }; @@ -93,7 +94,7 @@ PrepareTransactionResult WalletTxBuilder::PrepareTransaction( return addChangePayment( sendFromAccount == ZCASH_LEGACY_ACCOUNT ? addr - : pwalletMain->GenerateChangeAddressForAccount( + : wallet.GenerateChangeAddressForAccount( sendFromAccount, getAllowedChangePools({ReceiverType::Sapling}))); }; @@ -126,7 +127,7 @@ PrepareTransactionResult WalletTxBuilder::PrepareTransaction( return changeAddressForSaplingAddress(fvk.DefaultAddress()); }, [&](const libzcash::UnifiedAddress& ua) -> ChangeAddress { - auto zufvk = pwalletMain->GetUFVKForAddress(ua); + auto zufvk = wallet.GetUFVKForAddress(ua); assert(zufvk.has_value()); return changeAddressForZUFVK(zufvk.value(), ua.GetKnownReceiverTypes()); }, @@ -137,14 +138,14 @@ PrepareTransactionResult WalletTxBuilder::PrepareTransaction( }, [&](const AccountZTXOPattern& acct) -> ChangeAddress { return addChangePayment( - pwalletMain->GenerateChangeAddressForAccount( + wallet.GenerateChangeAddressForAccount( acct.GetAccountId(), getAllowedChangePools(acct.GetReceiverTypes()))); } }, selector.GetPattern()); } - auto ovks = SelectOVKs(selector, spendable); + auto ovks = SelectOVKs(wallet, selector, spendable); return TransactionEffects( anchorConfirmations, @@ -169,6 +170,7 @@ CAmount WalletTxBuilder::DefaultDustThreshold() const { } SpendableInputs WalletTxBuilder::FindAllSpendableInputs( + const CWallet& wallet, const ZTXOSelector& selector, int32_t minDepth) const { @@ -177,6 +179,7 @@ SpendableInputs WalletTxBuilder::FindAllSpendableInputs( } InputSelectionResult WalletTxBuilder::ResolveInputsAndPayments( + const CWallet& wallet, const ZTXOSelector& selector, SpendableInputs& spendableMut, const std::vector& payments, @@ -370,6 +373,7 @@ GetOVKsForUFVK(const UnifiedFullViewingKey& ufvk, const SpendableInputs& spendab } std::pair WalletTxBuilder::SelectOVKs( + const CWallet& wallet, const ZTXOSelector& selector, const SpendableInputs& spendable) const { diff --git a/src/wallet/wallet_tx_builder.h b/src/wallet/wallet_tx_builder.h index d370a48f2..b85d3102d 100644 --- a/src/wallet/wallet_tx_builder.h +++ b/src/wallet/wallet_tx_builder.h @@ -305,7 +305,6 @@ typedef std::variant< class WalletTxBuilder { private: const CChainParams& params; - const CWallet& wallet; CFeeRate minRelayFee; uint32_t maxOrchardActions; @@ -320,6 +319,7 @@ private: * and the requested transaction strategy. */ InputSelectionResult ResolveInputsAndPayments( + const CWallet& wallet, const ZTXOSelector& selector, SpendableInputs& spendable, const std::vector& payments, @@ -332,18 +332,21 @@ private: * the spendable inputs. */ std::pair SelectOVKs( + const CWallet& wallet, const ZTXOSelector& selector, const SpendableInputs& spendable) const; public: - WalletTxBuilder(const CChainParams& params, const CWallet& wallet, CFeeRate minRelayFee): - params(params), wallet(wallet), minRelayFee(minRelayFee), maxOrchardActions(nOrchardActionLimit) {} + WalletTxBuilder(const CChainParams& params, CFeeRate minRelayFee): + params(params), minRelayFee(minRelayFee), maxOrchardActions(nOrchardActionLimit) {} SpendableInputs FindAllSpendableInputs( + const CWallet& wallet, const ZTXOSelector& selector, int32_t minDepth) const; PrepareTransactionResult PrepareTransaction( + CWallet& wallet, const ZTXOSelector& selector, SpendableInputs& spendable, const std::vector& payments,