Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RPC/Wallet: Add "use_txids" to output of getaddressinfo #22693

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/wallet/rpc/addresses.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -542,6 +542,10 @@ RPCHelpMan getaddressinfo()
{
{RPCResult::Type::STR, "label name", "Label name (defaults to \"\")."},
}},
{RPCResult::Type::ARR, "use_txids", "",
luke-jr marked this conversation as resolved.
Show resolved Hide resolved
luke-jr marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In c961db6 "RPC/Wallet: Add "use_txids" to output of getaddressinfo"

Without having read the help text, I'm having a hard time figuring out the meaning of this field. My initial reading had "use" as a verb, but that doesn't make sense. I think it might be clearer to rename this to something like used_in_txids or received_with_txids.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment was marked as resolved but nothing was changed.

{
{RPCResult::Type::STR_HEX, "txid", "The ids of transactions involving this wallet which received with the address"},
luke-jr marked this conversation as resolved.
Show resolved Hide resolved
luke-jr marked this conversation as resolved.
Show resolved Hide resolved
}},
}
},
RPCExamples{
Expand Down Expand Up @@ -635,6 +639,15 @@ RPCHelpMan getaddressinfo()
}
ret.pushKV("labels", std::move(labels));

// NOTE: Intentionally not special-casing a single txid: while addresses
// should never be reused, it's not unexpected to have RBF result in
// multiple txids for a single use.
UniValue use_txids(UniValue::VARR);
pwallet->FindScriptPubKeyUsed(std::set<CScript>{scriptPubKey}, [&use_txids](const CWalletTx&wtx) {
luke-jr marked this conversation as resolved.
Show resolved Hide resolved
use_txids.push_back(wtx.GetHash().GetHex());
});
ret.pushKV("use_txids", std::move(use_txids));

return ret;
},
};
Expand Down
65 changes: 63 additions & 2 deletions src/wallet/wallet.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -786,6 +786,60 @@ void CWallet::AddToSpends(const CWalletTx& wtx, WalletBatch* batch)
AddToSpends(txin.prevout, wtx.GetHash(), batch);
}

void CWallet::InitialiseAddressBookUsed()
{
for (const auto& entry : mapWallet) {
const CWalletTx& wtx = entry.second;
UpdateAddressBookUsed(wtx);
}
}
luke-jr marked this conversation as resolved.
Show resolved Hide resolved

void CWallet::UpdateAddressBookUsed(const CWalletTx& wtx)
{
for (const auto& output : wtx.tx->vout) {
CTxDestination dest;
if (!ExtractDestination(output.scriptPubKey, dest)) continue;
m_address_book[dest].m_used = true;
luke-jr marked this conversation as resolved.
Show resolved Hide resolved
}
}

bool CWallet::FindScriptPubKeyUsed(const std::set<CScript>& keys, const std::variant<std::monostate, std::function<void(const CWalletTx&)>, std::function<void(const CWalletTx&, uint32_t)>>& callback) const
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "Wallet: Add fairly-efficient [negative] check that an address is not known to be used" (16ad058)

I understand this code may need to get more complicated in a future PR, but in this PR I think it would be better to replace

std::variant<std::monostate, std::function<void(const CWalletTx&)>, std::function<void(const CWalletTx&, uint32_t)>>

with a simpler:

std::function<void(const CWalletTx&)>

It would make this PR easier to review and understand, and make test coverage better by not adding untested code. And std::function is nullable, so the monostate case could just correspond to the null case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think it's worth it to change this now just to revert it later.

Copy link
Contributor

@ryanofsky ryanofsky Feb 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #22693 (comment)

Don't think it's worth it to change this now just to revert it later.

Sure, I can see how knowing the output index would be useful. But in that case replacing the std::variant with just:

std::function<void(const CWalletTx&, uint32_t)>

would simplify this function a lot and not complicate the rpc much at all (would just add an unused parameter).

luke-jr marked this conversation as resolved.
Show resolved Hide resolved
{
luke-jr marked this conversation as resolved.
Show resolved Hide resolved
AssertLockHeld(cs_wallet);
bool found_any = false;
for (const auto& key : keys) {
CTxDestination dest;
if (!ExtractDestination(key, dest)) continue;
const auto& address_book_it = m_address_book.find(dest);
if (address_book_it == m_address_book.end()) continue;
if (address_book_it->second.m_used) {
found_any = true;
break;
}
}
if (!found_any) return false;
luke-jr marked this conversation as resolved.
Show resolved Hide resolved
if (std::holds_alternative<std::monostate>(callback)) return true;

found_any = false;
for (const auto& entry : mapWallet) {
const CWalletTx& wtx = entry.second;
for (size_t i = 0; i < wtx.tx->vout.size(); ++i) {
const auto& output = wtx.tx->vout[i];
if (keys.count(output.scriptPubKey)) {
found_any = true;
const auto callback_type = callback.index();
if (callback_type == 1) {
luke-jr marked this conversation as resolved.
Show resolved Hide resolved
std::get<std::function<void(const CWalletTx&)>>(callback)(wtx);
break;
}
std::get<std::function<void(const CWalletTx&, uint32_t)>>(callback)(wtx, i);
}
}
}

return found_any;
}

bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
{
if (IsCrypted())
Expand Down Expand Up @@ -1083,6 +1137,7 @@ CWalletTx* CWallet::AddToWallet(CTransactionRef tx, const TxState& state, const

// Update birth time when tx time is older than it.
MaybeUpdateBirthTime(wtx.GetTxTime());
UpdateAddressBookUsed(wtx);
}

if (!fInsertedNew)
Expand Down Expand Up @@ -2306,7 +2361,7 @@ void CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::ve
}
}

DBErrors CWallet::LoadWallet()
DBErrors CWallet::LoadWallet(const do_init_used_flag do_init_used_flag_val)
{
LOCK(cs_wallet);

Expand All @@ -2328,7 +2383,13 @@ DBErrors CWallet::LoadWallet()
assert(m_internal_spk_managers.empty());
}

return nLoadWalletRet;
if (nLoadWalletRet != DBErrors::LOAD_OK) {
return nLoadWalletRet;
luke-jr marked this conversation as resolved.
Show resolved Hide resolved
}

if (do_init_used_flag_val == do_init_used_flag::Init) InitialiseAddressBookUsed();

return DBErrors::LOAD_OK;
}

util::Result<void> CWallet::RemoveTxs(std::vector<uint256>& txs_to_remove)
Expand Down
15 changes: 14 additions & 1 deletion src/wallet/wallet.h
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
#include <string>
#include <unordered_map>
#include <utility>
#include <variant>
#include <vector>

#include <boost/signals2/signal.hpp>
Expand Down Expand Up @@ -237,6 +238,12 @@ struct CAddressBookData
*/
std::optional<std::string> label;

/** Whether address is the destination of any wallet transation.
* Unlike other fields in address data struct, the used value is determined
* at runtime and not serialized as part of address data.
*/
bool m_used{false};
luke-jr marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

@ryanofsky ryanofsky Feb 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "Wallet: Keep track of what addresses are used in wallet transactions (memory only)" (fc7954a)

Would suggest moving this next to the previously_spent member below and naming it previously_received instead of m_used. Calling it "used" is ambiguous and confusing because at the walletdb level, "used" indicates whether funds sent to the address have previously been spent, not whether funds were sent to the address.

Other names for "previously_received" / "previously_spent" could work too of course, but "used" seems too ambiguous.


/**
* Address purpose which was originally recorded for payment protocol
* support but now serves as a cached IsMine value. Wallet code should
Expand Down Expand Up @@ -337,6 +344,9 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
void AddToSpends(const COutPoint& outpoint, const uint256& wtxid, WalletBatch* batch = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
void AddToSpends(const CWalletTx& wtx, WalletBatch* batch = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);

void InitialiseAddressBookUsed() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
luke-jr marked this conversation as resolved.
Show resolved Hide resolved
void UpdateAddressBookUsed(const CWalletTx&) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);

/**
* Add a transaction to the wallet, or update it. confirm.block_* should
* be set when the transaction was known to be included in a block. When
Expand Down Expand Up @@ -550,6 +560,8 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
bool UnlockAllCoins() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
void ListLockedCoins(std::vector<COutPoint>& vOutpts) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);

bool FindScriptPubKeyUsed(const std::set<CScript>& keys, const std::variant<std::monostate, std::function<void(const CWalletTx&)>, std::function<void(const CWalletTx&, uint32_t)>>& callback = std::monostate()) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
luke-jr marked this conversation as resolved.
Show resolved Hide resolved

/*
* Rescan abort properties
*/
Expand Down Expand Up @@ -792,7 +804,8 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
CAmount GetDebit(const CTransaction& tx, const isminefilter& filter) const;
void chainStateFlushed(ChainstateRole role, const CBlockLocator& loc) override;

DBErrors LoadWallet();
enum class do_init_used_flag { Init, Skip };
luke-jr marked this conversation as resolved.
Show resolved Hide resolved
DBErrors LoadWallet(const do_init_used_flag do_init_used_flag_val = do_init_used_flag::Init);
luke-jr marked this conversation as resolved.
Show resolved Hide resolved

/** Erases the provided transactions from the wallet. */
util::Result<void> RemoveTxs(std::vector<uint256>& txs_to_remove) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
Expand Down
7 changes: 4 additions & 3 deletions src/wallet/wallettool.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ static void WalletCreate(CWallet* wallet_instance, uint64_t wallet_creation_flag
wallet_instance->TopUpKeyPool();
}

static std::shared_ptr<CWallet> MakeWallet(const std::string& name, const fs::path& path, DatabaseOptions options)
static std::shared_ptr<CWallet> MakeWallet(const std::string& name, const fs::path& path, DatabaseOptions options, CWallet::do_init_used_flag do_init_used_flag_val = CWallet::do_init_used_flag::Init)
{
DatabaseStatus status;
bilingual_str error;
Expand All @@ -61,7 +61,7 @@ static std::shared_ptr<CWallet> MakeWallet(const std::string& name, const fs::pa
std::shared_ptr<CWallet> wallet_instance{new CWallet(/*chain=*/nullptr, name, std::move(database)), WalletToolReleaseWallet};
DBErrors load_wallet_ret;
try {
load_wallet_ret = wallet_instance->LoadWallet();
load_wallet_ret = wallet_instance->LoadWallet(do_init_used_flag_val);
} catch (const std::runtime_error&) {
tfm::format(std::cerr, "Error loading %s. Is wallet being used by another process?\n", name);
return nullptr;
Expand Down Expand Up @@ -167,7 +167,8 @@ bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command)
DatabaseOptions options;
ReadDatabaseArgs(args, options);
options.require_existing = true;
const std::shared_ptr<CWallet> wallet_instance = MakeWallet(name, path, options);
// NOTE: We need to skip initialisation of the m_used flag, or else the address book count might be wrong
luke-jr marked this conversation as resolved.
Show resolved Hide resolved
const std::shared_ptr<CWallet> wallet_instance = MakeWallet(name, path, options, CWallet::do_init_used_flag::Skip);
Comment on lines +170 to +171
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "Wallet: Keep track of what addresses are used in wallet transactions (memory only)" (fc7954a)

This workaround is not really sufficient because it fixes the address book count for the wallet tool without fixing it for the bitcoind wallet:

walletInstance->WalletLogPrintf("m_address_book.size() = %u\n", walletInstance->m_address_book.size());


Ideally no workaround might be necessary if we could avoid adding extra entries to m_address_book (see #22693 (comment)). But assuming a workaround is necessary, it could be implemented more simply in both places with a new CAddressBookData method:

bool InAddressBook() const { return label || purpose || !receive_requests.empty(); } 

used with std::count_if instead of m_address_book.size() to return number of address book entries.

if (!wallet_instance) return false;
WalletShowInfo(wallet_instance.get());
wallet_instance->Close();
Expand Down
10 changes: 10 additions & 0 deletions test/functional/wallet_basic.py
Original file line number Diff line number Diff line change
Expand Up @@ -653,6 +653,16 @@ def run_test(self):
assert not address_info["iswatchonly"]
assert not address_info["isscript"]
assert not address_info["ischange"]
assert_equal(address_info['use_txids'], [])

# Test getaddressinfo 'use_txids' field
addr = "mneYUmWYsuk7kySiURxCi3AGxrAqZxLgPZ"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In c961db6 "RPC/Wallet: Add "use_txids" to output of getaddressinfo"

nit: Does this address have to be hardcoded? I think we want to avoid hardcoding things like this in tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should avoid hardcoded stuff, but I noticed this address is hardcoded right before when testing getaddressinfo, probably to test scriptPubKey. Anyway, I think we could move it to a variable and use it whatever we need.

diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py
index 0372ebf15..54730533e 100755
--- a/test/functional/wallet_basic.py
+++ b/test/functional/wallet_basic.py
@@ -630,8 +630,9 @@ class WalletTest(BitcoinTestFramework):
 
         # Test getaddressinfo on external address. Note that these addresses are taken from disablewallet.py
         assert_raises_rpc_error(-5, "Invalid prefix for Base58-encoded address", self.nodes[0].getaddressinfo, "3J98t1WpEZ73CNmQviecrnyiWrnqRhWNLy")
-        address_info = self.nodes[0].getaddressinfo("mneYUmWYsuk7kySiURxCi3AGxrAqZxLgPZ")
-        assert_equal(address_info['address'], "mneYUmWYsuk7kySiURxCi3AGxrAqZxLgPZ")
+        addr = "mneYUmWYsuk7kySiURxCi3AGxrAqZxLgPZ"
+        address_info = self.nodes[0].getaddressinfo(addr)
+        assert_equal(address_info['address'], addr)
         assert_equal(address_info["scriptPubKey"], "76a9144e3854046c7bd1594ac904e4793b6a45b36dea0988ac")
         assert not address_info["ismine"]
         assert not address_info["iswatchonly"]
@@ -640,7 +641,6 @@ class WalletTest(BitcoinTestFramework):
         assert_equal(address_info['use_txids'], [])
 
         # Test getaddressinfo 'use_txids' field
-        addr = "mneYUmWYsuk7kySiURxCi3AGxrAqZxLgPZ"
         txid_1 = self.nodes[0].sendtoaddress(addr, 1)
         address_info = self.nodes[0].getaddressinfo(addr)
         assert_equal(address_info['use_txids'], [txid_1])

txid_1 = self.nodes[0].sendtoaddress(addr, 1)
address_info = self.nodes[0].getaddressinfo(addr)
assert_equal(address_info['use_txids'], [txid_1])
txid_2 = self.nodes[0].sendtoaddress(addr, 1)
address_info = self.nodes[0].getaddressinfo(addr)
assert_equal(sorted(address_info['use_txids']), sorted([txid_1, txid_2]))

# Test getaddressinfo 'ischange' field on change address.
self.generate(self.nodes[0], 1, sync_fun=self.no_op)
Expand Down