From 3060fc01976d40252728cd06746ec9b3a0bcb076 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Mon, 23 May 2022 12:08:22 -0400 Subject: [PATCH 01/13] Merge bitcoin/bitcoin#25122: rpc: getreceivedbylabel, return early if no addresses were found in the address book baa3ddc49c46d00e3e0de06e494656f0f00b0ee8 doc: add release notes about `getreceivedbylabel` returning an error if the label is not in the address book. (furszy) 8897a21658ad93f7b628eb2a3411fec2265d73fb rpc: getreceivedbylabel, don't loop over the entire wallet txs map if no destinations were found for the input label. (furszy) Pull request description: Built on top of #23662, coming from comment https://github.com/bitcoin/bitcoin/pull/23662#pullrequestreview-971407999. If `wallet.GetLabelAddresses()` returns an empty vector (the wallet does not have stored destinations with that label in the addressbook) or if none of the returned destinations are from the wallet, we can return the function right away. Otherwise, we are walking through all the wallet txs + outputs for no reason (`output_scripts` is empty). ACKs for top commit: achow101: ACK baa3ddc49c46d00e3e0de06e494656f0f00b0ee8 theStack: re-ACK baa3ddc49c46d00e3e0de06e494656f0f00b0ee8 w0xlt: ACK https://github.com/bitcoin/bitcoin/pull/25122/commits/baa3ddc49c46d00e3e0de06e494656f0f00b0ee8 Tree-SHA512: 00e10365b179bf008da2f3ef8fbb3ee04a330426374020e3f2d0151b16991baba4ef2b944e4659452f3e4d6cb20f128d0918ddf0453933a25a4d9fd8414a1911 Co-authored-by: Andrew Chow --- doc/release-notes-25122.md | 7 ++++++ src/wallet/rpc/coins.cpp | 28 ++++++++++++++---------- test/functional/wallet_listreceivedby.py | 3 +++ 3 files changed, 26 insertions(+), 12 deletions(-) create mode 100644 doc/release-notes-25122.md diff --git a/doc/release-notes-25122.md b/doc/release-notes-25122.md new file mode 100644 index 000000000000..5c5f0f911154 --- /dev/null +++ b/doc/release-notes-25122.md @@ -0,0 +1,7 @@ + +Wallet +------ + +- RPC `getreceivedbylabel` now returns an error, "Label not found + in wallet" (-4), if the label is not in the address book. (dash#7550) + diff --git a/src/wallet/rpc/coins.cpp b/src/wallet/rpc/coins.cpp index 845d8003230c..2f0dcbd6c67b 100644 --- a/src/wallet/rpc/coins.cpp +++ b/src/wallet/rpc/coins.cpp @@ -17,28 +17,32 @@ namespace wallet { static CAmount GetReceived(const CWallet& wallet, const UniValue& params, bool by_label) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet) { - std::vector output_scripts; + std::vector addresses; if (by_label) { // Get the set of addresses assigned to label - std::string label = LabelFromValue(params[0]); - for (const auto& address : wallet.ListAddrBookAddresses(CWallet::AddrBookFilter{label})) { - auto output_script{GetScriptForDestination(address)}; - if (wallet.IsMine(output_script)) { - output_scripts.emplace_back(output_script); - } - } + addresses = wallet.ListAddrBookAddresses(CWallet::AddrBookFilter{LabelFromValue(params[0])}); + if (addresses.empty()) throw JSONRPCError(RPC_WALLET_ERROR, "Label not found in wallet"); } else { // Get the address CTxDestination dest = DecodeDestination(params[0].get_str()); if (!IsValidDestination(dest)) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid Dash address"); } - CScript script_pub_key = GetScriptForDestination(dest); - if (!wallet.IsMine(script_pub_key)) { - throw JSONRPCError(RPC_WALLET_ERROR, "Address not found in wallet"); + addresses.emplace_back(dest); + } + + // Filter by own scripts only + std::vector output_scripts; + for (const auto& address : addresses) { + auto output_script{GetScriptForDestination(address)}; + if (wallet.IsMine(output_script)) { + output_scripts.emplace_back(output_script); } - output_scripts.emplace_back(script_pub_key); + } + + if (output_scripts.empty()) { + throw JSONRPCError(RPC_WALLET_ERROR, "Address not found in wallet"); } // Minimum confirmations diff --git a/test/functional/wallet_listreceivedby.py b/test/functional/wallet_listreceivedby.py index f3e3fd001dfc..b172029c04e7 100755 --- a/test/functional/wallet_listreceivedby.py +++ b/test/functional/wallet_listreceivedby.py @@ -139,6 +139,9 @@ def run_test(self): txid = self.nodes[0].sendtoaddress(addr, 0.1) self.sync_all() + # getreceivedbylabel returns an error if the wallet doesn't own the label + assert_raises_rpc_error(-4, "Label not found in wallet", self.nodes[0].getreceivedbylabel, "dummy") + # listreceivedbylabel should return received_by_label_json because of 0 confirmations assert_array_result(self.nodes[1].listreceivedbylabel(), {"label": label}, From 1723bc73da3e66082dcd2916bc4f70ba82a9249e Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Mon, 15 Aug 2022 19:35:52 +0100 Subject: [PATCH 02/13] Merge bitcoin-core/gui#598: Avoid recalculating the wallet balance - use model cache 4584d300a40bfd84517072f7a6eee114fb7cab08 GUI: remove now unneeded 'm_balances' field from overviewpage (furszy) 050e8b139145d6991e740b0e5f2b3364663dd348 GUI: 'getAvailableBalance', use cached balance if the user did not select UTXO manually (furszy) 96e3264a82c51b456703f500bd98e8cb98115697 GUI: use cached balance in overviewpage and sendcoinsdialog (furszy) 321335bf0292034d79afa6c44f7f072942b6cc3c GUI: add getter for WalletModel::m_cached_balances field (furszy) e62958dc81d215a1c56318d0914dfd9a33d45973 GUI: sendCoinsDialog, remove duplicate wallet().getBalances() call (furszy) Pull request description: As per the title says, we are recalculating the entire wallet balance on different situations calling to `wallet().getBalances()`, when should instead make use of the wallet model cached balance. This has the benefits of (1) not spending resources calculating a balance that we already have cached, and (2) avoid blocking the main thread for a long time, in case of big wallets, walking through the entire wallet's tx map more than what it's really needed. Changes: 1) Fix: `SendCoinsDialog` was calling `wallet().getBalances()` twice during `setModel`. 2) Use the cached balance if the user did not select any UTXO manually inside the wallet model `getAvailableBalance` call. ----------------------- As an extra note, this work born in [#25005](https://github.com/bitcoin/bitcoin/pull/25005) but grew out of scope of it. ACKs for top commit: jarolrod: ACK 4584d300a40bfd84517072f7a6eee114fb7cab08 hebasto: re-ACK 4584d300a40bfd84517072f7a6eee114fb7cab08, only suggested changes and commit message formatting since my [recent](https://github.com/bitcoin-core/gui/pull/598#pullrequestreview-1071268192) review. Tree-SHA512: 6633ce7f9a82a3e46e75aa7295df46c80a4cd4a9f3305427af203c9bc8670573fa8a1927f14a279260c488cc975a08d238faba2e9751588086fea1dcf8ea2b28 Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> --- src/qt/overviewpage.cpp | 36 +++++++++++++++++------------------- src/qt/overviewpage.h | 1 - src/qt/sendcoinsdialog.cpp | 12 +++++------- src/qt/sendcoinsdialog.h | 2 +- src/qt/test/wallettests.cpp | 31 ++++++++++++++++--------------- src/qt/walletmodel.cpp | 26 ++++++++++++++++++++++++-- src/qt/walletmodel.h | 7 +++++++ 7 files changed, 70 insertions(+), 45 deletions(-) diff --git a/src/qt/overviewpage.cpp b/src/qt/overviewpage.cpp index 9ce945530a5b..e827820cbc25 100644 --- a/src/qt/overviewpage.cpp +++ b/src/qt/overviewpage.cpp @@ -163,8 +163,6 @@ OverviewPage::OverviewPage(QWidget* parent) : GUIUtil::updateFonts(); - m_balances.balance = -1; - // Recent transactions ui->listTransactions->setItemDelegate(txdelegate); // Note: minimum height of listTransactions will be set later in updateAdvancedCJUI() to reflect actual settings @@ -202,8 +200,9 @@ void OverviewPage::setPrivacy(bool privacy) { m_privacy = privacy; clientModel->getOptionsModel()->setOption(OptionsModel::OptionID::MaskValues, privacy); - if (m_balances.balance != -1) { - setBalance(m_balances); + const auto& balances = walletModel->getCachedBalance(); + if (balances.balance != -1) { + setBalance(balances); coinJoinStatus(true); } @@ -226,7 +225,6 @@ OverviewPage::~OverviewPage() void OverviewPage::setBalance(const interfaces::WalletBalances& balances) { BitcoinUnit unit = walletModel->getOptionsModel()->getDisplayUnit(); - m_balances = balances; if (walletModel->wallet().isLegacy()) { if (walletModel->wallet().privateKeysDisabled()) { ui->labelBalance->setText(BitcoinUnits::floorHtmlWithPrivacy(unit, balances.watch_only_balance, BitcoinUnits::SeparatorStyle::ALWAYS, m_privacy)); @@ -312,12 +310,11 @@ void OverviewPage::setWalletModel(WalletModel *model) // update the display unit, to not use the default ("DASH") updateDisplayUnit(); // Keep up to date with wallet - interfaces::Wallet& wallet = model->wallet(); - interfaces::WalletBalances balances = wallet.getBalances(); - setBalance(balances); + setBalance(model->getCachedBalance()); connect(model, &WalletModel::balanceChanged, this, &OverviewPage::setBalance); - updateWatchOnlyLabels((wallet.haveWatchOnly() && !model->wallet().privateKeysDisabled()) || gArgs.GetBoolArg("-debug-ui", false)); + interfaces::Wallet& wallet = model->wallet(); + updateWatchOnlyLabels((wallet.haveWatchOnly() && !wallet.privateKeysDisabled()) || gArgs.GetBoolArg("-debug-ui", false)); connect(model, &WalletModel::notifyWatchonlyChanged, [this](bool showWatchOnly) { updateWatchOnlyLabels(showWatchOnly && !walletModel->wallet().privateKeysDisabled()); }); @@ -348,11 +345,11 @@ void OverviewPage::setWalletModel(WalletModel *model) void OverviewPage::updateDisplayUnit() { - if(walletModel && walletModel->getOptionsModel()) - { + if (walletModel && walletModel->getOptionsModel()) { m_display_bitcoin_unit = walletModel->getOptionsModel()->getDisplayUnit(); - if (m_balances.balance != -1) { - setBalance(m_balances); + const auto& balances = walletModel->getCachedBalance(); + if (balances.balance != -1) { + setBalance(balances); } // Update txdelegate->unit with the current unit @@ -404,7 +401,8 @@ void OverviewPage::updateCoinJoinProgress() QString strAmountAndRounds; QString strCoinJoinAmount = BitcoinUnits::formatHtmlWithUnit(m_display_bitcoin_unit, clientModel->coinJoinOptions().getAmount() * COIN, false, BitcoinUnits::SeparatorStyle::ALWAYS); - if(m_balances.balance == 0) + const auto& balances = walletModel->getCachedBalance(); + if(balances.balance == 0) { ui->coinJoinProgress->setValue(0); ui->coinJoinProgress->setToolTip(tr("No inputs detected")); @@ -420,7 +418,7 @@ void OverviewPage::updateCoinJoinProgress() CAmount nAnonymizableBalance = walletModel->wallet().getAnonymizableBalance(false, false); - CAmount nMaxToAnonymize = nAnonymizableBalance + m_balances.anonymized_balance; + CAmount nMaxToAnonymize = nAnonymizableBalance + balances.anonymized_balance; // If it's more than the anon threshold, limit to that. if (nMaxToAnonymize > clientModel->coinJoinOptions().getAmount() * COIN) nMaxToAnonymize = clientModel->coinJoinOptions().getAmount() * COIN; @@ -451,7 +449,6 @@ void OverviewPage::updateCoinJoinProgress() if (!fShowAdvancedCJUI) return; - const interfaces::WalletBalances balances = walletModel->wallet().getBalances(); CAmount nDenominatedConfirmedBalance = balances.denominated_trusted; CAmount nDenominatedUnconfirmedBalance = balances.denominated_untrusted_pending; CAmount nNormalizedAnonymizedBalance; @@ -477,7 +474,7 @@ void OverviewPage::updateCoinJoinProgress() anonNormPart = anonNormPart > 1 ? 1 : anonNormPart; anonNormPart *= 100; - anonFullPart = (float)m_balances.anonymized_balance / nMaxToAnonymize; + anonFullPart = (float)balances.anonymized_balance / nMaxToAnonymize; anonFullPart = anonFullPart > 1 ? 1 : anonFullPart; anonFullPart *= 100; @@ -692,7 +689,7 @@ void OverviewPage::coinJoinStatus(bool fForce) setWidgetsVisible(true); } -void OverviewPage::toggleCoinJoin(){ +void OverviewPage::toggleCoinJoin() { QSettings settings; // Popup some information on first mixing QString hasMixed = settings.value("hasMixed").toString(); @@ -707,9 +704,10 @@ void OverviewPage::toggleCoinJoin(){ bool mixing{false}; walletModel->withCoinJoin([&](auto& client) { mixing = client.isMixing(); }); if (!mixing) { + const auto& balances = walletModel->getCachedBalance(); auto& options = walletModel->node().coinJoinOptions(); const CAmount nMinAmount = options.getSmallestDenomination() + options.getMaxCollateralAmount(); - if(m_balances.balance < nMinAmount) { + if(balances.balance < nMinAmount) { QString strMinAmount(BitcoinUnits::formatWithUnit(m_display_bitcoin_unit, nMinAmount)); QMessageBox::warning(this, strCoinJoinName, tr("%1 requires at least %2 to use.").arg(strCoinJoinName).arg(strMinAmount), diff --git a/src/qt/overviewpage.h b/src/qt/overviewpage.h index b1a5c2f047ea..a12d74e5705b 100644 --- a/src/qt/overviewpage.h +++ b/src/qt/overviewpage.h @@ -54,7 +54,6 @@ public Q_SLOTS: Ui::OverviewPage *ui; ClientModel* clientModel{nullptr}; WalletModel* walletModel{nullptr}; - interfaces::WalletBalances m_balances; bool m_privacy{false}; BitcoinUnit m_display_bitcoin_unit; bool fShowAdvancedCJUI; diff --git a/src/qt/sendcoinsdialog.cpp b/src/qt/sendcoinsdialog.cpp index db2a304b2410..39f36a645e96 100644 --- a/src/qt/sendcoinsdialog.cpp +++ b/src/qt/sendcoinsdialog.cpp @@ -176,11 +176,9 @@ void SendCoinsDialog::setModel(WalletModel *_model) } } - interfaces::WalletBalances balances = _model->wallet().getBalances(); - setBalance(balances); connect(_model, &WalletModel::balanceChanged, this, &SendCoinsDialog::setBalance); - connect(_model->getOptionsModel(), &OptionsModel::displayUnitChanged, this, &SendCoinsDialog::updateDisplayUnit); - updateDisplayUnit(); + connect(_model->getOptionsModel(), &OptionsModel::displayUnitChanged, this, &SendCoinsDialog::refreshBalance); + refreshBalance(); // Coin Control connect(_model->getOptionsModel(), &OptionsModel::displayUnitChanged, this, &SendCoinsDialog::coinControlUpdateLabels); @@ -822,9 +820,9 @@ void SendCoinsDialog::setBalance(const interfaces::WalletBalances& balances) } } -void SendCoinsDialog::updateDisplayUnit() +void SendCoinsDialog::refreshBalance() { - setBalance(model->wallet().getBalances()); + setBalance(model->getCachedBalance()); coinControlUpdateLabels(); ui->customFee->setDisplayUnit(model->getOptionsModel()->getDisplayUnit()); updateSmartFeeLabel(); @@ -896,7 +894,7 @@ void SendCoinsDialog::useAvailableBalance(SendCoinsEntry* entry) m_coin_control->fAllowWatchOnly = model->wallet().privateKeysDisabled() && !model->wallet().hasExternalSigner(); // Calculate available amount to send. - CAmount amount = model->wallet().getAvailableBalance(*m_coin_control); + CAmount amount = model->getAvailableBalance(m_coin_control.get()); for (int i = 0; i < ui->entries->count(); ++i) { SendCoinsEntry* e = qobject_cast(ui->entries->itemAt(i)->widget()); if (e && !e->isHidden() && e != entry) { diff --git a/src/qt/sendcoinsdialog.h b/src/qt/sendcoinsdialog.h index 90e1eff6ce6e..a8caa5598d7d 100644 --- a/src/qt/sendcoinsdialog.h +++ b/src/qt/sendcoinsdialog.h @@ -99,7 +99,7 @@ private Q_SLOTS: void on_buttonMinimizeFee_clicked(); void removeEntry(SendCoinsEntry* entry); void useAvailableBalance(SendCoinsEntry* entry); - void updateDisplayUnit(); + void refreshBalance(); void coinControlFeatureChanged(bool); void coinControlButtonClicked(); void coinControlChangeChecked(int); diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index 1394ac1fdea1..3be37a434f34 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -100,6 +100,15 @@ QModelIndex FindTx(const QAbstractItemModel& model, const uint256& txid) return {}; } +void CompareBalance(WalletModel& walletModel, CAmount expected_balance, QLabel* balance_label_to_check, bool use_privacy_formatting) +{ + BitcoinUnit unit = walletModel.getOptionsModel()->getDisplayUnit(); + QString balanceComparison = use_privacy_formatting + ? BitcoinUnits::floorHtmlWithPrivacy(unit, expected_balance, BitcoinUnits::SeparatorStyle::ALWAYS, false) + : BitcoinUnits::formatWithUnit(unit, expected_balance, false/*, BitcoinUnits::SeparatorStyle::ALWAYS*/); + QCOMPARE(balance_label_to_check->text().trimmed(), balanceComparison); +} + //! Simple qt wallet tests. // // Test widgets can be debugged interactively calling show() on them and @@ -162,15 +171,10 @@ void TestGUI(interfaces::Node& node) sendCoinsDialog.setModel(&walletModel); transactionView.setModel(&walletModel); - { - // Check balance in send dialog - QLabel* balanceLabel = sendCoinsDialog.findChild("labelBalance"); - QString balanceText = balanceLabel->text(); - BitcoinUnit unit = walletModel.getOptionsModel()->getDisplayUnit(); - CAmount balance = walletModel.wallet().getBalance(); - QString balanceComparison = BitcoinUnits::formatWithUnit(unit, balance, false /*, BitcoinUnits::SeparatorStyle::ALWAYS*/); - QCOMPARE(balanceText, balanceComparison); - } + // Update walletModel cached balance which will trigger an update for the 'labelBalance' QLabel. + walletModel.pollBalanceChanged(); + // Check balance in send dialog + CompareBalance(walletModel, walletModel.wallet().getBalance(), sendCoinsDialog.findChild("labelBalance"), false); // Send two transactions, and verify they are added to transaction list. TransactionTableModel* transactionTableModel = walletModel.getTransactionTableModel(); @@ -187,12 +191,8 @@ void TestGUI(interfaces::Node& node) OverviewPage overviewPage; overviewPage.setClientModel(&clientModel); overviewPage.setWalletModel(&walletModel); - QLabel* balanceLabel = overviewPage.findChild("labelBalance"); - QString balanceText = balanceLabel->text().trimmed(); - BitcoinUnit unit = walletModel.getOptionsModel()->getDisplayUnit(); - CAmount balance = walletModel.wallet().getBalance(); - QString balanceComparison = BitcoinUnits::floorHtmlWithPrivacy(unit, balance, BitcoinUnits::SeparatorStyle::ALWAYS, false); - QCOMPARE(balanceText, balanceComparison); + walletModel.pollBalanceChanged(); // Manual balance polling update + CompareBalance(walletModel, walletModel.wallet().getBalance(), overviewPage.findChild("labelBalance"), true); // Check that each autobackup failure state selects its specific tooltip on the CoinJoin status label { @@ -238,6 +238,7 @@ void TestGUI(interfaces::Node& node) QPushButton* requestPaymentButton = receiveCoinsDialog.findChild("receiveButton"); requestPaymentButton->click(); QString address; + BitcoinUnit unit = walletModel.getOptionsModel()->getDisplayUnit(); for (QWidget* widget : QApplication::topLevelWidgets()) { if (widget->inherits("ReceiveRequestDialog")) { ReceiveRequestDialog* receiveRequestDialog = qobject_cast(widget); diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index d4f25cee6410..7ab86fa32a6f 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -73,6 +73,10 @@ WalletModel::~WalletModel() void WalletModel::startPollBalance() { + // Update the cached balance right away, so every view can make use of it, + // so them don't need to waste resources recalculating it. + pollBalanceChanged(); + // This timer will be fired repeatedly to update the balance // Since the QTimer::timeout is a private signal, it cannot be used // in the GUIUtil::ExceptionSafeConnect directly. @@ -137,12 +141,17 @@ void WalletModel::pollBalanceChanged() void WalletModel::checkBalanceChanged(const interfaces::WalletBalances& new_balances) { - if(new_balances.balanceChanged(m_cached_balances)) { + if (new_balances.balanceChanged(m_cached_balances)) { m_cached_balances = new_balances; Q_EMIT balanceChanged(new_balances); } } +interfaces::WalletBalances WalletModel::getCachedBalance() const +{ + return m_cached_balances; +} + void WalletModel::updateTransaction() { // Balance and number of transactions might have changed @@ -258,7 +267,9 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact } } - CAmount nBalance = m_wallet->getAvailableBalance(coinControl); + // If no coin was manually selected, use the cached balance + // Future: can merge this call with 'createTransaction'. + CAmount nBalance = getAvailableBalance(&coinControl); if(total > nBalance) { @@ -633,3 +644,14 @@ uint256 WalletModel::getLastBlockProcessed() const { return m_client_model ? m_client_model->getBestBlockHash() : uint256{}; } + +CAmount WalletModel::getAvailableBalance(const CCoinControl* control) +{ + if (control && control->HasSelected()) { + return wallet().getAvailableBalance(*control); + } + if (control && control->IsUsingCoinJoin()) { + return getCachedBalance().anonymized_balance; + } + return getCachedBalance().balance; +} diff --git a/src/qt/walletmodel.h b/src/qt/walletmodel.h index 262c766ae482..7c07875092c9 100644 --- a/src/qt/walletmodel.h +++ b/src/qt/walletmodel.h @@ -163,6 +163,13 @@ class WalletModel : public QObject uint256 getLastBlockProcessed() const; + // Retrieve the cached wallet balance + interfaces::WalletBalances getCachedBalance() const; + + // If coin control has selected outputs, searches the total amount inside the wallet. + // Otherwise, uses the wallet's cached available balance. + CAmount getAvailableBalance(const wallet::CCoinControl* control); + private: std::unique_ptr m_wallet; std::unique_ptr m_handler_unload; From 1403845ff1bf7368015c49f444f43636c105233a Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Tue, 16 Aug 2022 12:55:17 -0400 Subject: [PATCH 03/13] Merge bitcoin/bitcoin#25504: RPC: allow to track coins by parent descriptors a6b0c1fcc06485ecd320727fa7534a51a20608c1 doc: add releases notes for 25504 (listsinceblock updates) (Antoine Poinsot) 0fd2d144540b720626fc065a3cef5188831b5ee2 rpc: add an include_change parameter to listsinceblock (Antoine Poinsot) 55f98d087efd2609d808c082d5770306cc489409 rpc: output parent wallet descriptors for coins in listunspent (Antoine Poinsot) b724476158a7dfeef9edfda3f519dfd6f93202a8 rpc: output wallet descriptors for received entries in listsinceblock (Antoine Poinsot) 55a82eaf91d252a04a0cc8ad7d948d956c6cb24f wallet: allow to fetch the wallet descriptors for a given Script (Antoine Poinsot) Pull request description: Wallet descriptors are useful for applications using the Bitcoin Core wallet as a backend for tracking coins, as they allow to track coins for multiple descriptors in a single wallet. However there is no information currently given for such applications to link a coin with an imported descriptor, severely limiting the possibilities for such applications of using multiple descriptors in a single wallet. This PR outputs the matching imported descriptor(s) for a given received coin in `listsinceblock` (and friends). It comes from a need for an application i'm working on, but i think it's something any software using `bitcoind` to track multiple descriptors in a single wallet would have eventually. For instance i'm thinking about the BDK project. Currently, the way to achieve this is to import raw addresses with labels and to have your application be responsible for wallet things like the gap limit. I'll add this to the output of `listunspent` too if this gets a few Concept ACKs. ACKs for top commit: instagibbs: ACK https://github.com/bitcoin/bitcoin/pull/25504/commits/a6b0c1fcc06485ecd320727fa7534a51a20608c1 achow101: re-ACK a6b0c1fcc06485ecd320727fa7534a51a20608c1 Tree-SHA512: 7a5850e8de98b439ddede2cb72de0208944f8cda67272e8b8037678738d55b7a5272375be808b0f7d15def4904430e089dafdcc037436858ff3292c5f8b75e37 Co-authored-by: Andrew Chow --- doc/release-notes-25504.md | 6 +++ src/rpc/client.cpp | 1 + src/wallet/receive.cpp | 6 +-- src/wallet/receive.h | 3 +- src/wallet/rpc/coins.cpp | 4 ++ src/wallet/rpc/transactions.cpp | 23 +++++++-- src/wallet/rpc/util.cpp | 9 ++++ src/wallet/rpc/util.h | 4 ++ src/wallet/wallet.cpp | 12 +++++ src/wallet/wallet.h | 3 ++ test/functional/wallet_basic.py | 33 +++++++++++++ test/functional/wallet_listsinceblock.py | 62 +++++++++++++++++++++++- 12 files changed, 156 insertions(+), 10 deletions(-) create mode 100644 doc/release-notes-25504.md diff --git a/doc/release-notes-25504.md b/doc/release-notes-25504.md new file mode 100644 index 000000000000..bf80f180318a --- /dev/null +++ b/doc/release-notes-25504.md @@ -0,0 +1,6 @@ +Updated RPCs +------------ + +- The `listsinceblock`, `listtransactions` and `gettransaction` output now contain a new + `parent_descs` field for every "receive" entry. +- A new optional `include_change` parameter was added to the `listsinceblock` command. diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index 2579ed364274..972e99c94924 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -95,6 +95,7 @@ static const CRPCConvertParam vRPCConvertParams[] = { "listsinceblock", 1, "target_confirmations" }, { "listsinceblock", 2, "include_watchonly" }, { "listsinceblock", 3, "include_removed" }, + { "listsinceblock", 4, "include_change" }, { "sendmany", 1, "amounts" }, { "sendmany", 2, "minconf" }, { "sendmany", 3, "addlocked" }, diff --git a/src/wallet/receive.cpp b/src/wallet/receive.cpp index 43bf3b633529..309c43450a0f 100644 --- a/src/wallet/receive.cpp +++ b/src/wallet/receive.cpp @@ -194,7 +194,8 @@ CAmount CachedTxGetAvailableCredit(const CWallet& wallet, const CWalletTx& wtx, void CachedTxGetAmounts(const CWallet& wallet, const CWalletTx& wtx, std::list& listReceived, - std::list& listSent, CAmount& nFee, const isminefilter& filter) + std::list& listSent, CAmount& nFee, const isminefilter& filter, + bool include_change) { nFee = 0; listReceived.clear(); @@ -219,8 +220,7 @@ void CachedTxGetAmounts(const CWallet& wallet, const CWalletTx& wtx, // 2) the output is to us (received) if (nDebit > 0) { - // Don't report 'change' txouts - if (OutputIsChange(wallet, txout)) + if (!include_change && OutputIsChange(wallet, txout)) continue; } else if (!(fIsMine & filter)) diff --git a/src/wallet/receive.h b/src/wallet/receive.h index f19901794660..0acc4c6373e7 100644 --- a/src/wallet/receive.h +++ b/src/wallet/receive.h @@ -42,7 +42,8 @@ struct COutputEntry void CachedTxGetAmounts(const CWallet& wallet, const CWalletTx& wtx, std::list& listReceived, std::list& listSent, - CAmount& nFee, const isminefilter& filter); + CAmount& nFee, const isminefilter& filter, + bool include_change); bool CachedTxIsFromMe(const CWallet& wallet, const CWalletTx& wtx, const isminefilter& filter); bool CachedTxIsTrusted(const CWallet& wallet, const CWalletTx& wtx, std::set& trusted_parents) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); bool CachedTxIsTrusted(const CWallet& wallet, const CWalletTx& wtx); diff --git a/src/wallet/rpc/coins.cpp b/src/wallet/rpc/coins.cpp index 2f0dcbd6c67b..5a060111fb3a 100644 --- a/src/wallet/rpc/coins.cpp +++ b/src/wallet/rpc/coins.cpp @@ -592,6 +592,9 @@ RPCHelpMan listunspent() {RPCResult::Type::BOOL, "solvable", "Whether we know how to spend this output, ignoring the lack of keys"}, {RPCResult::Type::STR, "desc", /*optional=*/true, "(only when solvable) A descriptor for spending this output"}, {RPCResult::Type::BOOL, "reused", /* optional*/ true, "(only present if avoid_reuse is set) Whether this output is reused/dirty (sent to an address that was previously spent from)"}, + {RPCResult::Type::ARR, "parent_descs", /*optional=*/false, "List of parent descriptors for the scriptPubKey of this coin.", { + {RPCResult::Type::STR, "desc", "The descriptor string."}, + }}, {RPCResult::Type::BOOL, "safe", "Whether this output is considered safe to spend. Unconfirmed transactions" "from outside keys and unconfirmed replacement transactions are considered unsafe\n" "and are not eligible for spending by fundrawtransaction and sendtoaddress."}, @@ -762,6 +765,7 @@ RPCHelpMan listunspent() entry.pushKV("desc", descriptor->ToString()); } } + PushParentDescriptors(*pwallet, scriptPubKey, entry); if (avoid_reuse) entry.pushKV("reused", reused); entry.pushKV("safe", out.safe); entry.pushKV("coinjoin_rounds", pwallet->GetRealOutpointCoinJoinRounds(out.outpoint)); diff --git a/src/wallet/rpc/transactions.cpp b/src/wallet/rpc/transactions.cpp index b5b7de1afd7c..6b760e3f919a 100644 --- a/src/wallet/rpc/transactions.cpp +++ b/src/wallet/rpc/transactions.cpp @@ -324,13 +324,16 @@ static void MaybePushAddress(UniValue & entry, const CTxDestination &dest) * @param filter_label Optional label string to filter incoming transactions. */ template -static void ListTransactions(const CWallet& wallet, const CWalletTx& wtx, int nMinDepth, bool fLong, Vec& ret, const isminefilter& filter_ismine, const std::string* filter_label) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet) +static void ListTransactions(const CWallet& wallet, const CWalletTx& wtx, int nMinDepth, bool fLong, + Vec& ret, const isminefilter& filter_ismine, const std::string* filter_label, + bool include_change = false) + EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet) { CAmount nFee; std::list listReceived; std::list listSent; - CachedTxGetAmounts(wallet, wtx, listReceived, listSent, nFee, filter_ismine); + CachedTxGetAmounts(wallet, wtx, listReceived, listSent, nFee, filter_ismine, include_change); bool involvesWatchonly = CachedTxIsFromMe(wallet, wtx, ISMINE_WATCH_ONLY); @@ -377,6 +380,7 @@ static void ListTransactions(const CWallet& wallet, const CWalletTx& wtx, int nM entry.pushKV("involvesWatchonly", true); } MaybePushAddress(entry, r.destination); + PushParentDescriptors(wallet, wtx.tx->vout.at(r.vout).scriptPubKey, entry); if (wtx.IsCoinBase()) { if (wallet.GetTxDepthInMainChain(wtx) < 1) @@ -428,7 +432,11 @@ static std::vector TransactionDescriptionString() {RPCResult::Type::NUM_TIME, "time", "The transaction time expressed in " + UNIX_EPOCH_TIME + "."}, {RPCResult::Type::NUM_TIME, "timereceived", "The time received expressed in " + UNIX_EPOCH_TIME + ". Available \n" "for 'send' and 'receive' category of transactions."}, - {RPCResult::Type::STR, "comment", /*optional=*/true, "If a comment is associated with the transaction."}}; + {RPCResult::Type::STR, "comment", /*optional=*/true, "If a comment is associated with the transaction."}, + {RPCResult::Type::ARR, "parent_descs", /*optional=*/true, "Only if 'category' is 'received'. List of parent descriptors for the scriptPubKey of this coin.", { + {RPCResult::Type::STR, "desc", "The descriptor string."}, + }}, + }; } RPCHelpMan listtransactions() @@ -555,6 +563,7 @@ RPCHelpMan listsinceblock() {"include_watchonly", RPCArg::Type::BOOL, RPCArg::DefaultHint{"true for watch-only wallets, otherwise false"}, "Include transactions to watch-only addresses (see 'importaddress')"}, {"include_removed", RPCArg::Type::BOOL, RPCArg::Default{true}, "Show transactions that were removed due to a reorg in the \"removed\" array\n" "(not guaranteed to work on pruned nodes)"}, + {"include_change", RPCArg::Type::BOOL, RPCArg::Default{false}, "Also add entries for change outputs.\n"}, }, RPCResult{ RPCResult::Type::OBJ, "", "", @@ -636,6 +645,7 @@ RPCHelpMan listsinceblock() } bool include_removed = (request.params[3].isNull() || request.params[3].get_bool()); + bool include_change = (!request.params[4].isNull() && request.params[4].get_bool()); int depth = height ? wallet.GetLastBlockHeight() + 1 - *height : -1; @@ -645,7 +655,7 @@ RPCHelpMan listsinceblock() const CWalletTx& tx = pairWtx.second; if (depth == -1 || abs(wallet.GetTxDepthInMainChain(tx)) < depth) { - ListTransactions(wallet, tx, 0, true, transactions, filter, nullptr /* filter_label */); + ListTransactions(wallet, tx, 0, true, transactions, filter, nullptr /* filter_label */, /*include_change=*/include_change); } } @@ -662,7 +672,7 @@ RPCHelpMan listsinceblock() if (it != wallet.mapWallet.end()) { // We want all transactions regardless of confirmation count to appear here, // even negative confirmation ones, hence the big negative. - ListTransactions(wallet, it->second, -100000000, true, removed, filter, nullptr /* filter_label */); + ListTransactions(wallet, it->second, -100000000, true, removed, filter, nullptr /* filter_label */, /*include_change=*/include_change); } } blockId = block.hashPrevBlock; @@ -721,6 +731,9 @@ RPCHelpMan gettransaction() {RPCResult::Type::STR_AMOUNT, "fee", /*optional=*/true, "The amount of the fee in " + CURRENCY_UNIT + ". This is negative and only available for the \n" "'send' category of transactions."}, {RPCResult::Type::BOOL, "abandoned", "'true' if the transaction has been abandoned (inputs are respendable)."}, + {RPCResult::Type::ARR, "parent_descs", /*optional=*/true, "Only if 'category' is 'received'. List of parent descriptors for the scriptPubKey of this coin.", { + {RPCResult::Type::STR, "desc", "The descriptor string."}, + }}, }}, }}, {RPCResult::Type::STR_HEX, "hex", "Raw data for transaction"}, diff --git a/src/wallet/rpc/util.cpp b/src/wallet/rpc/util.cpp index e67be931af6d..25151961df8b 100644 --- a/src/wallet/rpc/util.cpp +++ b/src/wallet/rpc/util.cpp @@ -138,6 +138,15 @@ std::string LabelFromValue(const UniValue& value) return label; } +void PushParentDescriptors(const CWallet& wallet, const CScript& script_pubkey, UniValue& entry) +{ + UniValue parent_descs(UniValue::VARR); + for (const auto& desc: wallet.GetWalletDescriptors(script_pubkey)) { + parent_descs.push_back(desc.descriptor->ToString()); + } + entry.pushKV("parent_descs", parent_descs); +} + void HandleWalletError(const std::shared_ptr wallet, DatabaseStatus& status, bilingual_str& error) { if (!wallet) { diff --git a/src/wallet/rpc/util.h b/src/wallet/rpc/util.h index c24cfe9f8485..d621308ee703 100644 --- a/src/wallet/rpc/util.h +++ b/src/wallet/rpc/util.h @@ -9,6 +9,8 @@ #include #include +#include