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

pull S3RK wants to merge 6 commits into bitcoin:master from S3RK:fix_19856 changing 8 files +202 −59
  1. S3RK commented at 7:55 AM on September 9, 2021: contributor

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

    1. When a receiving address is generated externally to the wallet (e.g. same wallet running on two nodes, or by 3rd party from xpub)
    2. 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

  2. fanquake added the label Wallet on Sep 9, 2021
  3. DrahtBot commented at 12:48 PM on September 9, 2021: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

  4. DrahtBot cross-referenced this on Sep 9, 2021 from issue RPC/Wallet: Add "use_txids" to output of getaddressinfo by luke-jr
  5. DrahtBot cross-referenced this on Sep 10, 2021 from issue wallet, bugfix: fix ComputeTimeSmart function during rescanning process. by BitcoinTsunami
  6. S3RK force-pushed on Sep 14, 2021
  7. S3RK force-pushed on Sep 14, 2021
  8. 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

         * [@param](/github-metadata-backup-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

  9. 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 :

                    for (const auto& type : LEGACY_OUTPUT_TYPES) {
    S3RK commented at 6:48 AM on September 20, 2021:

    Done

  10. 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.

                       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.

  11. DrahtBot cross-referenced this on Sep 18, 2021 from issue rpc, wallet: Add listaddresses RPC by lsilva01
  12. S3RK force-pushed on Sep 20, 2021
  13. achow101 commented at 8:23 PM on September 24, 2021: member

    ACK c97c2a99bf5aed073cc28967c5d47a33ec618752

  14. dgs2019 approved
  15. DrahtBot cross-referenced this on Sep 25, 2021 from issue Allow UTXO locks to be written to wallet DB by meshcollider
  16. DrahtBot added the label Needs rebase on Sep 26, 2021
  17. S3RK force-pushed on Sep 30, 2021
  18. 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.

  19. 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

  20. DrahtBot removed the label Needs rebase on Sep 30, 2021
  21. 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

  22. 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

  23. 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

  24. meshcollider commented at 2:53 AM on October 4, 2021: contributor

    Concept ACK

  25. wallet: resolve ambiguity of two ScriptPubKey managers providing same script 456e350926
  26. Add CWallet::IsInternalScriptPubKeyMan 03840c2064
  27. Return used destinations from ScriptPubKeyMan::MarkUnusedAddresses c1b99c088c
  28. S3RK force-pushed on Oct 6, 2021
  29. S3RK force-pushed on Oct 6, 2021
  30. Automatically add labels to detected receiving addresses 9f3a622b1c
  31. Add to spends only transcations from me d04566415e
  32. test: listtranscations with externally generated addresses 3d71d16d1e
  33. S3RK force-pushed on Oct 6, 2021
  34. S3RK requested review from achow101 on Oct 18, 2021
  35. S3RK requested review from meshcollider on Oct 18, 2021
  36. DrahtBot cross-referenced this on Nov 23, 2021 from issue Add external signer taproot support by Sjors
  37. DrahtBot cross-referenced this on Dec 2, 2021 from issue MOVEONLY: Move wallet backup and encryption RPCs out of rpcwallet by meshcollider
  38. laanwj commented at 6:37 PM on December 2, 2021: member

    Code review ACK 3d71d16d1eb4173c70d4c294559fc2365e189856

  39. laanwj merged this on Dec 2, 2021
  40. laanwj closed this on Dec 2, 2021
  41. jamesob cross-referenced this on Dec 2, 2021 from issue test: fix: remove outdated TestNode.generate calls by jamesob
  42. sidhujag referenced this in commit f97eb77096 on Dec 3, 2021
  43. RandyMcMillan referenced this in commit 700d0e1857 on Dec 23, 2021
  44. 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.

  45. S3RK commented at 8:43 AM on January 10, 2022: contributor

    @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

  46. luke-jr commented at 12: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)

  47. achow101 commented at 5:55 PM on January 13, 2022: member

    @luke-jr Does reverting d04566415e16ae685af066384f346dff522c068f fix your issue?

  48. 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.

  49. S3RK commented at 7:56 AM on January 17, 2022: contributor

    Created #24083 with a revert

  50. S3RK cross-referenced this on Jan 17, 2022 from issue Revert "Add to spends only transcations from me" by S3RK
  51. achow101 referenced this in commit 02e1d8d06f on Feb 1, 2022
  52. S3RK cross-referenced this on Aug 23, 2022 from issue RPC: allow to track coins by parent descriptors by darosior
  53. bitcoin locked this on Jan 17, 2023
Contributors
S3RKDrahtBotachow101dgs2019meshcolliderlaanwjluke-jr


meshcolliderachow101

Labels
Wallet
Linked (view graph)
#19856 Some transactions are not shown in listtransactions output#20293 The listtransactions command does not display transactions when restoring a wallet from a backup#23659 test: fix: remove outdated TestNode.generate calls#24083 Revert "Add to spends only transcations from me"#25504 RPC: allow to track coins by parent descriptors
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:53 UTC