diff --git a/src/wallet/asyncrpcoperation_sendmany.cpp b/src/wallet/asyncrpcoperation_sendmany.cpp index 06ddfa405..407191914 100644 --- a/src/wallet/asyncrpcoperation_sendmany.cpp +++ b/src/wallet/asyncrpcoperation_sendmany.cpp @@ -84,7 +84,7 @@ void AsyncRPCOperation_sendmany::main() { std::optional txid; try { - txid = main_impl(); + txid = main_impl(*pwalletMain); } catch (const UniValue& objError) { int code = find_value(objError, "code").get_int(); std::string message = find_value(objError, "message").get_str(); @@ -137,11 +137,11 @@ void AsyncRPCOperation_sendmany::main() { // 4. #3615 There is no padding of inputs or outputs, which may leak information. // // At least #4 differs from the Rust transaction builder. -uint256 AsyncRPCOperation_sendmany::main_impl() { - auto spendable = builder_.FindAllSpendableInputs(*pwalletMain, ztxoSelector_, mindepth_); +uint256 AsyncRPCOperation_sendmany::main_impl(CWallet& wallet) { + auto spendable = builder_.FindAllSpendableInputs(wallet, ztxoSelector_, mindepth_); auto preparedTx = builder_.PrepareTransaction( - *pwalletMain, + wallet, ztxoSelector_, spendable, recipients_, @@ -156,37 +156,43 @@ uint256 AsyncRPCOperation_sendmany::main_impl() { ThrowInputSelectionError(err, ztxoSelector_, strategy_); }, [&](const TransactionEffects& effects) { - const auto& spendable = effects.GetSpendable(); - const auto& payments = effects.GetPayments(); - spendable.LogInputs(getId()); + try { + const auto& spendable = effects.GetSpendable(); + const auto& payments = effects.GetPayments(); + spendable.LogInputs(getId()); - LogPrint("zrpcunsafe", "%s: spending %s to send %s with fee %s\n", getId(), - FormatMoney(payments.Total()), - FormatMoney(spendable.Total()), - FormatMoney(effects.GetFee())); - LogPrint("zrpc", "%s: total transparent input: %s (to choose from)\n", getId(), - FormatMoney(spendable.GetTransparentTotal())); - LogPrint("zrpcunsafe", "%s: total shielded input: %s (to choose from)\n", getId(), - FormatMoney(spendable.GetSaplingTotal() + spendable.GetOrchardTotal())); - LogPrint("zrpc", "%s: total transparent output: %s\n", getId(), - FormatMoney(payments.GetTransparentTotal())); - LogPrint("zrpcunsafe", "%s: total shielded Sapling output: %s\n", getId(), - FormatMoney(payments.GetSaplingTotal())); - LogPrint("zrpcunsafe", "%s: total shielded Orchard output: %s\n", getId(), - FormatMoney(payments.GetOrchardTotal())); - LogPrint("zrpc", "%s: fee: %s\n", getId(), FormatMoney(effects.GetFee())); + LogPrint("zrpcunsafe", "%s: spending %s to send %s with fee %s\n", getId(), + FormatMoney(payments.Total()), + FormatMoney(spendable.Total()), + FormatMoney(effects.GetFee())); + LogPrint("zrpc", "%s: total transparent input: %s (to choose from)\n", getId(), + FormatMoney(spendable.GetTransparentTotal())); + LogPrint("zrpcunsafe", "%s: total shielded input: %s (to choose from)\n", getId(), + FormatMoney(spendable.GetSaplingTotal() + spendable.GetOrchardTotal())); + LogPrint("zrpc", "%s: total transparent output: %s\n", getId(), + FormatMoney(payments.GetTransparentTotal())); + LogPrint("zrpcunsafe", "%s: total shielded Sapling output: %s\n", getId(), + FormatMoney(payments.GetSaplingTotal())); + LogPrint("zrpcunsafe", "%s: total shielded Orchard output: %s\n", getId(), + FormatMoney(payments.GetOrchardTotal())); + LogPrint("zrpc", "%s: fee: %s\n", getId(), FormatMoney(effects.GetFee())); - auto buildResult = effects.ApproveAndBuild( - Params().GetConsensus(), - *pwalletMain, - chainActive, - strategy_); - auto tx = buildResult.GetTxOrThrow(); + auto buildResult = effects.ApproveAndBuild( + Params().GetConsensus(), + wallet, + chainActive, + strategy_); + auto tx = buildResult.GetTxOrThrow(); - UniValue sendResult = SendTransaction(tx, payments.GetResolvedPayments(), std::nullopt, testmode); - set_result(sendResult); + UniValue sendResult = SendTransaction(tx, payments.GetResolvedPayments(), std::nullopt, testmode); + set_result(sendResult); - txid = tx.GetHash(); + txid = tx.GetHash(); + effects.UnlockSpendable(wallet); + } catch (...) { + effects.UnlockSpendable(wallet); + throw; + } } }, preparedTx); diff --git a/src/wallet/asyncrpcoperation_sendmany.h b/src/wallet/asyncrpcoperation_sendmany.h index 70608fe23..ec59470d2 100644 --- a/src/wallet/asyncrpcoperation_sendmany.h +++ b/src/wallet/asyncrpcoperation_sendmany.h @@ -64,7 +64,7 @@ private: CAmount fee_; UniValue contextinfo_; // optional data to include in return value from getStatus() - uint256 main_impl(); + uint256 main_impl(CWallet& wallet); }; // To test private methods, a friend class can act as a proxy @@ -74,8 +74,8 @@ public: TEST_FRIEND_AsyncRPCOperation_sendmany(std::shared_ptr ptr) : delegate(ptr) {} - uint256 main_impl() { - return delegate->main_impl(); + uint256 main_impl(CWallet& wallet) { + return delegate->main_impl(wallet); } void set_state(OperationStatus state) { diff --git a/src/wallet/wallet_tx_builder.cpp b/src/wallet/wallet_tx_builder.cpp index d573e3504..6e7cce64a 100644 --- a/src/wallet/wallet_tx_builder.cpp +++ b/src/wallet/wallet_tx_builder.cpp @@ -147,7 +147,7 @@ PrepareTransactionResult WalletTxBuilder::PrepareTransaction( auto ovks = SelectOVKs(wallet, selector, spendable); - return TransactionEffects( + auto effects = TransactionEffects( anchorConfirmations, spendable, resolvedPayments, @@ -156,6 +156,8 @@ PrepareTransactionResult WalletTxBuilder::PrepareTransaction( ovks.first, ovks.second, anchorHeight); + effects.LockSpendable(wallet); + return effects; } Payments InputSelection::GetPayments() const { @@ -646,3 +648,35 @@ TransactionBuilderResult TransactionEffects::ApproveAndBuild( // Build the transaction return builder.Build(); } + +// TODO: Lock Orchard notes (#6226) +void TransactionEffects::LockSpendable(CWallet& wallet) const +{ + LOCK2(cs_main, wallet.cs_wallet); + for (auto utxo : spendable.utxos) { + COutPoint outpt(utxo.tx->GetHash(), utxo.i); + wallet.LockCoin(outpt); + } + for (auto note : spendable.sproutNoteEntries) { + wallet.LockNote(note.jsop); + } + for (auto note : spendable.saplingNoteEntries) { + wallet.LockNote(note.op); + } +} + +// TODO: Unlock Orchard notes (#6226) +void TransactionEffects::UnlockSpendable(CWallet& wallet) const +{ + LOCK2(cs_main, wallet.cs_wallet); + for (auto utxo : spendable.utxos) { + COutPoint outpt(utxo.tx->GetHash(), utxo.i); + wallet.UnlockCoin(outpt); + } + for (auto note : spendable.sproutNoteEntries) { + wallet.UnlockNote(note.jsop); + } + for (auto note : spendable.saplingNoteEntries) { + wallet.UnlockNote(note.op); + } +} diff --git a/src/wallet/wallet_tx_builder.h b/src/wallet/wallet_tx_builder.h index b85d3102d..ac5ff34bc 100644 --- a/src/wallet/wallet_tx_builder.h +++ b/src/wallet/wallet_tx_builder.h @@ -48,7 +48,9 @@ public: PaymentAddress address, CAmount amount, std::optional memo) : - address(address), amount(amount), memo(memo) {} + address(address), amount(amount), memo(memo) { + assert(amount >= 0); + } const PaymentAddress& GetAddress() const { return address; @@ -185,6 +187,22 @@ public: return spendable; } + /** + * This should be called upon creating `TransactionEffects`, it locks exactly the notes that + * will be spent in the built transaction. + */ + void LockSpendable(CWallet& wallet) const; + + /** + * This should be called when we are finished with the transaction (whether it succeeds or + * fails). + * + * TODO: This currently needs to be called while the `TransactionEffects` exists. In future, it + * would be useful to keep these notes locked until we have confirmation that the tx is on + * the chain or not. + */ + void UnlockSpendable(CWallet& wallet) const; + const Payments& GetPayments() const { return payments; }