wallet: Automatically add receiving destinations to the address book

S3RK commented at 7:55 am on September 9, 2021: member

This PR fixes certain use-cases when send-to-self transactions are missing from listtransactions output.

When a receiving address is generated externally to the wallet (e.g. same wallet running on two nodes, or by 3rd party from xpub)
When restoring backup with lost metadata, but keypool gap is not exceeded yet

When the block is connected or tx added to mempool we already mark used keys. This PR extends this logic to determine whether the destination is a receiving one and if yes add it to the address book with empty label.

Works both for legacy and descriptors wallets.

For legacy it uses the internal flag from the keypool entry. Caveat: because we don’t know which script type would be used we add all possible destinations for such keys.
For descriptor wallets it uses internal flag for the script pub key manager. Caveat: it only works for active descriptors.

fixes #19856 fixes #20293

fanquake added the label Wallet on Sep 9, 2021

DrahtBot commented at 12:48 pm on September 9, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23647 (Split rpcwallet into multiple smaller parts by meshcollider)
#23019 (rpc, wallet: Add listaddresses RPC by lsilva01)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

S3RK force-pushed on Sep 14, 2021

in src/wallet/scriptpubkeyman.h:498 in fec5ded232 outdated

492@@ -481,9 +493,13 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv
493     void LearnAllRelatedScripts(const CPubKey& key);
494 
495     /**
496-     * Marks all keys in the keypool up to and including reserve_key as used.
497+     * Marks all keys in the keypool up to and including the provided key as used.
498+     *
499+     * @param keypool_id determines the last key to makr as used

achow101 commented at 8:45 pm on September 16, 2021:

In fec5ded232ca37b8a965ce09a3940ccb6ce43249 “Return used destinations from ScriptPubKeyMan::MarkUnusedAddresses”

typo: makr - > mark

0     * [@param](/bitcoin-bitcoin/contributor/param/) keypool_id determines the last key to mark as used

S3RK commented at 6:48 am on September 20, 2021:

Done

in src/wallet/scriptpubkeyman.cpp:368 in fec5ded232 outdated

365         if (mi != m_pool_key_to_index.end()) {
366             WalletLogPrintf("%s: Detected a used keypool key, mark all keypool keys up to this key as used\n", __func__);
367-            MarkReserveKeysAsUsed(mi->second);
368+            for (const auto& keypool : MarkReserveKeysAsUsed(mi->second)) {
369+                // derive all possible destinations as any of them could have been used
370+                for (const auto& type: LEGACY_OUTPUT_TYPES) {

achow101 commented at 8:47 pm on September 16, 2021:

In fec5ded232ca37b8a965ce09a3940ccb6ce43249 “Return used destinations from ScriptPubKeyMan::MarkUnusedAddresses”

nit: space before :

0                for (const auto& type : LEGACY_OUTPUT_TYPES) {

S3RK commented at 6:48 am on September 20, 2021:

Done

in src/wallet/wallet.cpp:1071 in cbb90fbc00 outdated

1067+                    }
1068+
1069+                    // If this is a receiving address and it's not in the address book yet
1070+                    // (e.g. it wasn't generated on this node or we're restoring from backup)
1071+                    // add it to the address book for proper transaction accounting
1072+                    if (!dest.internal.value_or(true) && !FindAddressBookEntry(dest.dest, /* allow_change= */ false)) {

achow101 commented at 8:53 pm on September 16, 2021:

In cbb90fbc00e345100587cd47926c6c7fc5487628 “Automatically add labels to detected receiving addresses”

Shouldn’t we be marking everything that doesn’t have dest.internal as a receiving address? i.e.

0                   if (!dest.internal.value_or(false) && !FindAddressBookEntry(dest.dest, /* allow_change= */ false)) {

S3RK commented at 6:49 am on September 20, 2021:

I want to be conservative and skip if we can’t determine if that’s a change or a receiving destination. But the code is not obvious and I restructured it to make it explicit.

S3RK force-pushed on Sep 20, 2021

achow101 commented at 8:23 pm on September 24, 2021: member

ACK c97c2a99bf5aed073cc28967c5d47a33ec618752

dgs2019 approved

DrahtBot added the label Needs rebase on Sep 26, 2021

S3RK force-pushed on Sep 30, 2021

in src/wallet/scriptpubkeyman.cpp:1847 in a438bcc7e2 outdated

1844             WalletLogPrintf("%s: Detected a used keypool item at index %d, mark all keypool items up to this item as used\n", __func__, index);
1845-            m_wallet_descriptor.next_index = index + 1;
1846+            auto out_keys = std::make_unique<FlatSigningProvider>();
1847+            std::vector<CScript> scripts_temp;
1848+            while (index >= m_wallet_descriptor.next_index) {
1849+                if (!m_wallet_descriptor.descriptor->ExpandFromCache(m_wallet_descriptor.next_index, m_wallet_descriptor.cache, scripts_temp, *out_keys)) {

S3RK commented at 7:48 am on September 30, 2021:

@achow101 I believe expanding from cache is enough here. But want to confirm.

achow101 commented at 6:00 pm on September 30, 2021:

Yes, that is.

in src/wallet/wallet.cpp:1067 in a438bcc7e2 outdated

1063@@ -1062,8 +1064,23 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, CWalletTx::Co
1064 
1065             // loop though all outputs
1066             for (const CTxOut& txout: tx.vout) {
1067-                for (const auto& spk_man_pair : m_spk_managers) {
1068-                    spk_man_pair.second->MarkUnusedAddresses(txout.scriptPubKey);
1069+                const auto& spk_man = GetScriptPubKeyMan(txout.scriptPubKey);

S3RK commented at 7:51 am on September 30, 2021:

@achow101 I think it’s technically possible to have two descriptors producing same scriptPubKey. Wdyt?

achow101 commented at 6:04 pm on September 30, 2021:

Ah, that is indeed possible. In that case, we should keep this loop.

S3RK commented at 6:24 am on October 6, 2021:

Returned the cycle and removed GetScriptPubKeyMan function to avoid such mistakes in the future

DrahtBot removed the label Needs rebase on Sep 30, 2021

in src/wallet/wallet.cpp:3306 in 243473ed82 outdated

3294+    }
3295+
3296+    LOCK(desc_spk_man->cs_desc_man);
3297+    const auto& type = desc_spk_man->GetWalletDescriptor().descriptor->GetOutputType();
3298+
3299+    return GetScriptPubKeyMan(*type, /* internal= */ true) == desc_spk_man;

meshcollider commented at 2:32 am on October 4, 2021:

Should check if (type) or has_value() to ensure it has a value before calling *type

S3RK commented at 6:25 am on October 6, 2021:

Active descriptors always have type defined, added assertion

in src/wallet/scriptpubkeyman.cpp:1461 in 3690de4454 outdated

1457@@ -1448,7 +1458,10 @@ void LegacyScriptPubKeyMan::MarkReserveKeysAsUsed(int64_t keypool_id)
1458         batch.ErasePool(index);
1459         WalletLogPrintf("keypool index %d removed\n", index);
1460         it = setKeyPool->erase(it);
1461+        result.push_back(keypool);

meshcollider commented at 2:39 am on October 4, 2021:

I think this is better as std::move(keypool)?

S3RK commented at 6:25 am on October 6, 2021:

Done

in test/functional/wallet_listtransactions.py:238 in a438bcc7e2 outdated

233+        addr1 = self.nodes[0].getnewaddress("pizza1", 'legacy')
234+        addr2 = self.nodes[0].getnewaddress("pizza2", 'p2sh-segwit')
235+        addr3 = self.nodes[0].getnewaddress("pizza3", 'bech32')
236+
237+        self.log.info("Send to externally generated addresses")
238+        # send to N+2 address to test the keypool gap

meshcollider commented at 2:49 am on October 4, 2021:

What does N+2 mean here (and below)? (I understand based on the code that N is meant to be the “first” key that node[0] and therefore also node[2] generates but the comment is confusing).

S3RK commented at 6:26 am on October 6, 2021:

Yes. I updated the comments

meshcollider commented at 2:53 am on October 4, 2021: contributor

Concept ACK

wallet: resolve ambiguity of two ScriptPubKey managers providing same script 456e350926

Add CWallet::IsInternalScriptPubKeyMan 03840c2064

Return used destinations from ScriptPubKeyMan::MarkUnusedAddresses c1b99c088c

S3RK force-pushed on Oct 6, 2021

Automatically add labels to detected receiving addresses 9f3a622b1c

Add to spends only transcations from me d04566415e

test: listtranscations with externally generated addresses 3d71d16d1e

S3RK force-pushed on Oct 6, 2021

S3RK requested review from achow101 on Oct 18, 2021

S3RK requested review from meshcollider on Oct 18, 2021

laanwj commented at 6:37 pm on December 2, 2021: member

Code review ACK 3d71d16d1eb4173c70d4c294559fc2365e189856

laanwj merged this on Dec 2, 2021

laanwj closed this on Dec 2, 2021

sidhujag referenced this in commit f97eb77096 on Dec 3, 2021

RandyMcMillan referenced this in commit 700d0e1857 on Dec 23, 2021

luke-jr commented at 4:20 am on January 2, 2022: member

This breaks “walletconflicts”

Note: My bugfix_rpc_getbalance_hacky branch tests this by accident.

S3RK commented at 8:43 am on January 10, 2022: member

@luke-jr I’m not sure what exactly is the broken scenario, happy to debug or write a test for it. It seems to me that CWallet::GetLegacyBalance in bugfix_rpc_getbalance_hacky branch makes an incorrect assumption that incoming txs will be in mapTxSpends. But mapTxSpends contains only spends

luke-jr commented at 0:00 am on January 13, 2022: member

I’m not sure what exactly is the broken scenario, happy to debug or write a test for it.

“walletconflicts” no longer returns other transactions that conflict with the transaction.

This is an issue in master, not just my branch. I only mentioned my branch because it’s how I found it.

But mapTxSpends contains only spends

IIRC, it’s not supposed to only be spends from this wallet, but also spends to this wallet.

I’m not sure if that’s why “walletconflicts” is broken now, or not (though the comment for mapTxSpends suggests it may be)

achow101 commented at 5:55 pm on January 13, 2022: member

@luke-jr Does reverting d04566415e16ae685af066384f346dff522c068f fix your issue?

luke-jr commented at 6:08 pm on January 13, 2022: member

@luke-jr Does reverting d045664 fix your issue?

Right, sorry I wasn’t specific on which commit.

S3RK commented at 7:56 am on January 17, 2022: member

Created #24083 with a revert

achow101 referenced this in commit 02e1d8d06f on Feb 1, 2022

DrahtBot locked this on Jan 17, 2023

wallet: Automatically add receiving destinations to the address book #22929

Conflicts