From 06553d139900318e51ecbd9bb516a638ba9b5555 Mon Sep 17 00:00:00 2001 From: Greg Pfeil Date: Mon, 24 Oct 2022 20:26:26 -0600 Subject: [PATCH 1/2] Lock notes (except Orchard) in wallet_tx_builder This fixes an RPC test failure that tests specifically for this with z_shieldcoinbase. This also exposed an issue where an overly-high fee resulted in a negative Payment causing an exception too deep. Added an assert when creating a Payment and guarded against it for z_shieldcoinbase. Fixes #2621 and #5654 (but does not handle Orchard locking, which is tracked in a separate issue). --- src/wallet/asyncrpcoperation_sendmany.cpp | 68 ++++++++++++----------- src/wallet/asyncrpcoperation_sendmany.h | 6 +- src/wallet/wallet_tx_builder.cpp | 36 +++++++++++- src/wallet/wallet_tx_builder.h | 19 ++++++- 4 files changed, 93 insertions(+), 36 deletions(-) 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 1f448e3a3..700ff94bf 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..8ec1864c1 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; @@ -158,6 +160,10 @@ private: int anchorHeight; public: + /** + * This class locks the `SpendableInputs` provided to it. `UnlockSpendable` must be called to + * release them before the instance goes out of scope. + */ TransactionEffects( uint32_t anchorConfirmations, SpendableInputs spendable, @@ -185,6 +191,17 @@ public: return spendable; } + /** + * This is automatically called by the constructor, so it’s not generally necessary to call this + * otherwise. + */ + void LockSpendable(CWallet& wallet) const; + + /** + * This should be called just before the `TransactionEffects` goes out of scope. + */ + void UnlockSpendable(CWallet& wallet) const; + const Payments& GetPayments() const { return payments; } From 69ab52cb3e8610c2e0272836282ce7f5673756da Mon Sep 17 00:00:00 2001 From: Greg Pfeil Date: Thu, 30 Mar 2023 14:22:34 -0600 Subject: [PATCH 2/2] Improve Doxygen for note locking --- src/wallet/wallet_tx_builder.h | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/wallet/wallet_tx_builder.h b/src/wallet/wallet_tx_builder.h index 8ec1864c1..ac5ff34bc 100644 --- a/src/wallet/wallet_tx_builder.h +++ b/src/wallet/wallet_tx_builder.h @@ -160,10 +160,6 @@ private: int anchorHeight; public: - /** - * This class locks the `SpendableInputs` provided to it. `UnlockSpendable` must be called to - * release them before the instance goes out of scope. - */ TransactionEffects( uint32_t anchorConfirmations, SpendableInputs spendable, @@ -192,13 +188,18 @@ public: } /** - * This is automatically called by the constructor, so it’s not generally necessary to call this - * otherwise. + * 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 just before the `TransactionEffects` goes out of scope. + * 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;