wallet: Remove isminetypes #32523

pull achow101 wants to merge 4 commits into bitcoin:master from achow101:delete-isminetypes changing 32 files +215 −338
  1. achow101 commented at 1:53 am on May 16, 2025: member

    The remaining isminetypes are ISMINE_SPENDABLE and ISMINE_USED.

    ISMINE_USED is only used as a filter for caching balances and is never actually returned from IsMine. Since we do still want this behavior, This PR changes the caching to utilize bools and explicit members variables to account for the avoid_reuse case. This allows us to remove ISMINE_USED.

    ISMINE_SPENDABLE and ISMINE_NO are the only things that are returned by IsMine. This is a bool, so it can be replaced as such.

    After removing ISMINE_USED and ISMINE_SPENDABLE, we are able to remove isminetypes altogether.

  2. DrahtBot commented at 1:53 am on May 16, 2025: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32523.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK w0xlt

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #32896 (wallet, rpc: add v3 transaction creation and wallet support by ishaanam)
    • #31664 (Fees: add Fee rate Forecaster Manager by ismaelsadeeq)
    • #31650 (refactor: Avoid copies by using const references or by move-construction by maflcko)
    • #30157 (Fee Estimation via Fee rate Forecasters by ismaelsadeeq)
    • #27865 (wallet: Track no-longer-spendable TXOs separately by achow101)

    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.

  3. DrahtBot added the label Wallet on May 16, 2025
  4. DrahtBot added the label CI failed on May 16, 2025
  5. DrahtBot commented at 2:28 am on May 16, 2025: contributor

    🚧 At least one of the CI tasks failed. Task MSan, depends: https://github.com/bitcoin/bitcoin/runs/42330203892 LLM reason (✨ experimental): The CI failure is due to a thread safety error in spend.cpp.

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  6. maflcko added this to the milestone 30.0 on May 16, 2025
  7. DrahtBot added the label Needs rebase on May 16, 2025
  8. fanquake commented at 10:56 am on May 16, 2025: member 
    0 /home/runner/work/_temp/src/wallet/spend.cpp:474:12: error: calling function 'AvailableCoins' requires holding mutex 'wallet.cs_wallet' exclusively [-Werror,-Wthread-safety-analysis]
1  474 |     return AvailableCoins(wallet, coinControl, /*feerate=*/ std::nullopt, params);
2      |            ^
31 error generated.
  9. achow101 force-pushed on May 16, 2025
  10. DrahtBot removed the label Needs rebase on May 16, 2025
  11. DrahtBot removed the label CI failed on May 16, 2025
  12. DrahtBot added the label Needs rebase on May 19, 2025
  13. achow101 force-pushed on May 20, 2025
  14. DrahtBot removed the label Needs rebase on May 20, 2025
  15. DrahtBot added the label Needs rebase on May 21, 2025
  16. achow101 force-pushed on May 21, 2025
  17. achow101 commented at 5:51 pm on May 21, 2025: member
    Rebased, ready for review.
  18. achow101 marked this as ready for review on May 21, 2025
  19. rkrux commented at 6:13 pm on May 21, 2025: contributor

    This PR comes at a right time. I’m prioritising reviewing this one because it removes isminetype.

    #27286: The new wallet unspents model in memory is using isminetype currently and I’d prefer to not have it checked into master only to be removed subsequently. This PR changes should also ease the review of the other PR.

    https://github.com/bitcoin/bitcoin/pull/27286/commits/d637af8a9786e38f1ba140ed828a3d5009e62926#diff-d41d68c5a65d67956c91b33ca86da7df1981d84a0b15b4a186deea566563fed5R372

  20. DrahtBot removed the label Needs rebase on May 21, 2025
  21. DrahtBot added the label CI failed on May 21, 2025
  22. in src/wallet/rpc/addresses.cpp:405 in 1ec04d4c4e outdated 
    399@@ -401,7 +400,7 @@ RPCHelpMan getaddressinfo()
400                         {RPCResult::Type::OBJ, "embedded", /*optional=*/true, "Information about the address embedded in P2SH or P2WSH, if relevant and known.",
401                         {
402                             {RPCResult::Type::ELISION, "", "Includes all getaddressinfo output fields for the embedded address, excluding metadata (timestamp, hdkeypath, hdseedid)\n"
403-                            "and relation to the wallet (ismine, iswatchonly)."},
404+                            "and relation to the wallet (ismine)."},
    rkrux commented at 10:59 am on May 22, 2025:

    Nit:

    0- "and relation to the wallet (ismine)."},
1+ "and relation to the wallet."},
    rkrux commented at 2:52 pm on May 22, 2025:
    Please ignore this suggestion. I had suggested because it felt a bit odd to me to have just 1 item here but leaving ismine is actually helpful to give context in the help section.
  23. in src/wallet/coincontrol.h:92 in 8ebf126295 outdated 
    88@@ -89,8 +89,6 @@ class CCoinControl
89     //! If true, the selection process can add extra unselected inputs from the wallet
90     //! while requires all selected inputs be used
91     bool m_allow_other_inputs = true;
92-    //! Includes watch only addresses which are solvable
    rkrux commented at 10:59 am on May 22, 2025:
    IMO this was quite complicated to reason about and have in the wallet.
  24. in src/wallet/transaction.h:136 in ccf618bf3b outdated 
    135 {
136-    // NO and ALL are never (supposed to be) cached
137-    std::bitset<ISMINE_ENUM_ELEMENTS> m_cached;
138-    CAmount m_value[ISMINE_ENUM_ELEMENTS];
139+    std::optional<CAmount> m_avoid_reuse_value;
140+    std::optional<CAmount> m_all_value;
    rkrux commented at 1:47 pm on May 22, 2025:

    In ccf618bf3b88a95a01463e6a96fc8d9bc52fe57d wallet: Remove ISMINE_USED

    While I’m in favour of cleaning up the ismine enum and type, I find the consistent passing of avoid_reuse bool in all the caller functions quite cognitively loaded; the related if/else checks in the functions of this struct adds more.

    I did like CachableAmount previously storing the amounts in an array. Keeping the array but with a different enum CachableAmountType would get rid of the if/else blocks and CachableAmountType can be passed in the caller functions just like AmountType is passed.

    One more reason I’m leaning towards this is because avoid_reuse is something that’s integrated all the way from createwallet RPC and also has a corresponding wallet flag as well named WALLET_FLAG_AVOID_REUSE. Hence, I don’t think it’d be a stretch to introduce a corresponding enum value.

    achow101 commented at 6:48 pm on May 22, 2025:
    I actually did try to implement it with an enum originally, but it got pretty complicated and I think just having it has a bool is significantly easier to read and reason about.
  25. in src/wallet/spend.cpp:307 in 1c5483eae3 outdated 
    304@@ -305,7 +305,7 @@ util::Result<PreSelectedInputs> FetchSelectedInputs(const CWallet& wallet, const
305         }
306 
307         /* Set some defaults for depth, spendable, solvable, safe, time, and from_me as these don't matter for preset inputs since no selection is being done. */
    rkrux commented at 2:10 pm on May 22, 2025: 
    0- /* Set some defaults for depth, spendable, solvable, 
1+ /* Set some defaults for depth, solvable,
    achow101 commented at 6:52 pm on May 22, 2025:
    Done
  26. in src/wallet/rpc/coins.cpp:679 in 1c5483eae3 outdated 
    698@@ -699,7 +699,7 @@ RPCHelpMan listunspent()
699                 entry.pushKV("ancestorfees", uint64_t(ancestor_fees));
700             }
701         }
702-        entry.pushKV("spendable", out.spendable);
703+        entry.pushKV("spendable", true); // Any coins we list are always spendable
    rkrux commented at 2:20 pm on May 22, 2025:
    Would removing this property from the response categorise the diff as a breaking change? Looks a little odd to me to display this as true for watch only wallets as well because the wallet doesn’t have private keys and thus I feel it should not be a decision maker on this.
    achow101 commented at 6:51 pm on May 22, 2025:

    Would removing this property from the response categorise the diff as a breaking change?

    Yes, that’s mainly why this is preserved.

    Looks a little odd to me to display this as true for watch only wallets as well because the wallet doesn’t have private keys and thus I feel it should not be a decision maker on this.

    This is the current behavior anyways.

  27. in src/wallet/scriptpubkeyman.cpp:209 in f3ab751206 outdated 
    207+        return false;
208     case IsMineResult::WATCH_ONLY:
209     case IsMineResult::SPENDABLE:
210-        return ISMINE_SPENDABLE;
211+        return true;
212     }
    rkrux commented at 2:39 pm on May 22, 2025:
    Note: this is okay because this is used only during wallet migration from legacy to descriptor.
  28. rkrux commented at 2:46 pm on May 22, 2025: contributor

    Initial code review f3ab751206d4c3db8696ee541f9d253ce162f295

    I’m in favour of this clean up: maintaining watch-only per output is cumbersome in the code and seems complicated to me from a user’s point of view as well, great that the descriptor wallets provide watch-only behaviour on the wallet level.

    The CI error seems unrelated as it’s a timeout in the external signer test and it’s happening in other PRs as well.

  29. achow101 force-pushed on May 22, 2025
  30. DrahtBot removed the label CI failed on May 22, 2025
  31. in src/wallet/wallet.h:777 in 6e5f730c81 outdated 
    774+    bool IsMine(const CTxDestination& dest) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
775+    bool IsMine(const CScript& script) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
776     /**
777      * Returns amount of debit if the input matches the
778      * filter, otherwise returns 0
779      */
    rkrux commented at 10:05 am on May 24, 2025:
    There is no filter passed anymore.
    achow101 commented at 7:57 pm on May 26, 2025:
    Done
  32. in src/wallet/receive.h:16 in 6e5f730c81 outdated 
    13 
14 namespace wallet {
15-isminetype InputIsMine(const CWallet& wallet, const CTxIn& txin) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
16+bool InputIsMine(const CWallet& wallet, const CTxIn& txin) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
17 
18 /** Returns whether all of the inputs match the filter */
    rkrux commented at 10:08 am on May 24, 2025:
    No filter passed now.
    achow101 commented at 7:57 pm on May 26, 2025:
    Done
  33. in src/wallet/receive.h:29 in 6e5f730c81 outdated 
    30 CAmount TxGetChange(const CWallet& wallet, const CTransaction& tx);
31 
32-CAmount CachedTxGetCredit(const CWallet& wallet, const CWalletTx& wtx, const isminefilter& filter)
33+CAmount CachedTxGetCredit(const CWallet& wallet, const CWalletTx& wtx, bool avoid_reuse)
34     EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
35 //! filter decides which addresses will count towards the debit
    rkrux commented at 10:08 am on May 24, 2025:
    Same here.
    achow101 commented at 7:57 pm on May 26, 2025:
    Done
  34. in src/wallet/rpc/addresses.cpp:383 in 1ec04d4c4e outdated 
    378@@ -379,7 +379,6 @@ RPCHelpMan getaddressinfo()
379                         {RPCResult::Type::STR, "address", "The bitcoin address validated."},
380                         {RPCResult::Type::STR_HEX, "scriptPubKey", "The hex-encoded output script generated by the address."},
381                         {RPCResult::Type::BOOL, "ismine", "If the address is yours."},
382-                        {RPCResult::Type::BOOL, "iswatchonly", "If the address is watchonly."},
    maflcko commented at 12:32 pm on May 26, 2025:
    nit in 1ec04d4c4efe9e12f02f561d3c9fd23972c4a1bd: The RPC result removals should probably have a release note?
    achow101 commented at 7:38 pm on May 26, 2025:
    Added in #32618
  35. in src/wallet/rpc/spend.cpp:515 in 1ec04d4c4e outdated 
    516-      if (options.type() == UniValue::VBOOL) {
517-        // backward compatibility bool only fallback
518-        coinControl.fAllowWatchOnly = options.get_bool();
519-      }
520-      else {
521+    if (!options.isNull() && options.isObject()) {
    maflcko commented at 12:38 pm on May 26, 2025:
    nit in https://github.com/bitcoin/bitcoin/commit/1ec04d4c4efe9e12f02f561d3c9fd23972c4a1bd: Why is the backward-compat guard for options.isObject() needed? Seems better to remove && options.isObject()?
    achow101 commented at 6:33 pm on May 26, 2025:
    Since this RPC allowed all other types to be aliases for watchonly stuff, I added the backwards compatibility guard for any scripts/programs that may be setting the old parameter instead of an options object. The parameter is ignored because watchonly is no longer a thing, but this way it doesn’t throw for any callers written in that way.
    maflcko commented at 8:27 am on May 27, 2025:

    Since this RPC allowed all other types to be aliases for watchonly stuff

    I don’t think it did. It should have only allowed bool:

    0      if (options.type() == UniValue::VBOOL) {
1        // backward compatibility bool only fallback

    this way it doesn’t throw for any callers written in that way.

    You removed it from the options object as well, which has strict=true checking enabled. So I think it would be more consistent to remove it here as well, or keep it for both.

    achow101 commented at 6:35 pm on May 27, 2025:

    Hmm, yes, I was confused by the weird indentation here.

    I’ve put the backwards compatibility stuff back and changed the docs to say that the field is no longer used.

    Also fixed the indentation.

  36. in src/qt/transactionrecord.cpp:44 in 6e5f730c81 outdated 
    43     } else {
44-        for (const isminetype mine : wtx.txin_is_mine)
45+        for (const bool mine : wtx.txin_is_mine)
46         {
47-            if(fAllFromMe > mine) fAllFromMe = mine;
48+            all_from_me &= mine;
    rkrux commented at 12:50 pm on May 26, 2025:

    Not a bug but would prefer to have a logical and here instead of a bitwise one because all_from_me is strictly a boolean in cpp.

    0all_from_me = all_from_me && mine;

    Ref: https://stackoverflow.com/a/24560

    achow101 commented at 7:57 pm on May 26, 2025:
    Done
  37. in src/wallet/wallet.cpp:1604 in 6e5f730c81 outdated 
    1605     if (it != m_cached_spks.end()) {
1606-        isminetype res = ISMINE_NO;
1607+        bool res = false;
1608         for (const auto& spkm : it->second) {
1609-            res = std::max(res, spkm->IsMine(script));
1610+            res |= spkm->IsMine(script);
    rkrux commented at 1:08 pm on May 26, 2025:
    Same point of using logical and here.
    achow101 commented at 7:58 pm on May 26, 2025:
    Done
  38. in src/wallet/interfaces.cpp:79 in 6e5f730c81 outdated 
    77+                                                      false);
78     }
79-    result.credit = CachedTxGetCredit(wallet, wtx, ISMINE_ALL);
80-    result.debit = CachedTxGetDebit(wallet, wtx, ISMINE_ALL);
81+    result.credit = CachedTxGetCredit(wallet, wtx, /*avoid_reuse=*/false);
82+    result.debit = CachedTxGetDebit(wallet, wtx, /*avoid_reuse=*/false);
    rkrux commented at 1:34 pm on May 26, 2025: 
    0enum isminetype : unsigned int {
1    ISMINE_NO         = 0,
2    ISMINE_WATCH_ONLY = 1 << 0,
3    ISMINE_SPENDABLE  = 1 << 1,
4    ISMINE_USED       = 1 << 2,
5    ISMINE_ALL        = ISMINE_WATCH_ONLY | ISMINE_SPENDABLE,
6    ISMINE_ALL_USED   = ISMINE_ALL | ISMINE_USED,
7    ISMINE_ENUM_ELEMENTS,
8};

    ISMINE_ALL didn’t include ISMINE_USED, i.e., it avoided used ones probably because it wanted to avoid reuse. Should it not be replaced with /*avoid_reuse=*/true otherwise it ends up getting the “all” amount that contains “used” as well?

    Or have I misunderstood the intent here?

    rkrux commented at 2:42 pm on May 26, 2025:

    As per this earlier statement, avoid_reuse: true does translate to “don’t use ISMINE_USED”.

    https://github.com/bitcoin/bitcoin/pull/32523/files#diff-69473389a98be9232528ccdef04f9fa51ce8c5558e64994e15589be924eebae3L296

    0isminefilter reuse_filter = avoid_reuse ? ISMINE_NO : ISMINE_USED;
    achow101 commented at 7:58 pm on May 26, 2025:
    Done
  39. in src/wallet/transaction.h:131 in 6e5f730c81 outdated 
    127@@ -127,21 +128,35 @@ std::string TxStateString(const T& state)
128 }
129 
130 /**
131- * Cachable amount subdivided into watchonly and spendable parts.
132+ * Cachable amount subdivided into avoid reuse and all balances
    rkrux commented at 1:35 pm on May 26, 2025:

    The ccf618bf3b88a95a01463e6a96fc8d9bc52fe57d message can include the boolean param name.

    This isminetype is not a real isminetype as it is never returned by IsMine. This is only used for isminefilters in one function, which can be better represented with a bool parameter.

    achow101 commented at 7:58 pm on May 26, 2025:
    Done
  40. in test/functional/wallet_send.py:114 in 6e5f730c81 outdated 
    107@@ -110,6 +108,11 @@ def test_send(self, from_wallet, to_wallet=None, amount=None, data=None,
108         if len(options.keys()) == 0:
109             options = None
110 
111+        expect_sign = from_wallet.getwalletinfo()["private_keys_enabled"]
112+        expect_sign &= solving_data is None
113+        if inputs is not None:
114+            expect_sign &= all(["weight" not in i for i in inputs])
    rkrux commented at 1:46 pm on May 26, 2025:
    Bitwise and for booleans is fine in Python I guess but I find it odd to read it.
    achow101 commented at 7:57 pm on May 26, 2025:
    Done in #32618
  41. rkrux commented at 2:12 pm on May 26, 2025: contributor

    I had to stop the re-review midway because I started sensing that, if feasible, maybe the first 4 commits can be a separate PR that removes the watch only stuff separately. Otherwise this is a bit too much to unpack in one shot from a data consistency POV. The alternative could be that the ISMINE_USED and the following changes can be a separate PR.

    The e6fd00372951bdc06462b74a9d078c33a5f75e78 commit contains few comment formatting related changes as well that are slightly distracting.

  42. in src/wallet/rpc/util.cpp:40 in 1ec04d4c4e outdated 
    35- */
36-bool ParseIncludeWatchonly(const UniValue& include_watchonly, const CWallet& wallet)
37-{
38-    if (include_watchonly.isNull()) {
39-        // if include_watchonly isn't explicitly set, then check if we have a watchonly wallet
40-        return wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS);
    maflcko commented at 2:44 pm on May 26, 2025:

    1ec04d4c4efe9e12f02f561d3c9fd23972c4a1bd: I don’t think this is correct. Descriptor wallets can’t mix watchonly and non-watchonly, but they still need this for size estimation.

    At least locally when I try fundrawtransaction "020000000001002d310100000000160014a3a5a66c36a754cb95d32f5489a6f856aa30586000000000" '{"fee_rate":3.331}', I get now a smaller fee result for a watch-only wallet.

    I guess it would be good to add a test for this on current master or earlier in this pull, then fixup any later commits to not fail the test.

    achow101 commented at 7:33 pm on May 26, 2025:
    I’ve added a test and a fix for this in #32618
  43. achow101 marked this as a draft on May 26, 2025
  44. achow101 force-pushed on May 26, 2025
  45. achow101 commented at 7:58 pm on May 26, 2025: member

    maybe the first 4 commits can be a separate PR that removes the watch only stuff separately

    I’ve split those commits into #32618

  46. achow101 force-pushed on May 27, 2025
  47. DrahtBot added the label Needs rebase on Jun 4, 2025
  48. achow101 force-pushed on Jun 4, 2025
  49. DrahtBot removed the label Needs rebase on Jun 5, 2025
  50. achow101 force-pushed on Jun 14, 2025
  51. DrahtBot added the label Needs rebase on Jun 19, 2025
  52. achow101 force-pushed on Jun 20, 2025
  53. DrahtBot removed the label Needs rebase on Jun 20, 2025
  54. DrahtBot added the label Needs rebase on Jul 1, 2025
  55. achow101 force-pushed on Jul 1, 2025
  56. DrahtBot removed the label Needs rebase on Jul 1, 2025
  57. glozow referenced this in commit 09add84fc5 on Jul 7, 2025
  58. achow101 force-pushed on Jul 7, 2025
  59. achow101 commented at 11:13 pm on July 7, 2025: member
    #32618 merged, ready for review now.
  60. achow101 marked this as ready for review on Jul 7, 2025
  61. rkrux commented at 10:27 am on July 8, 2025: contributor
    I believe that the PR title and more importantly the PR description should be updated to remove the stuff related to watch-only as that code removal was done in another PR. The last paragraph of the description can be kept.
  62. rkrux commented at 2:20 pm on July 8, 2025: contributor

    Looking good mostly. One way I think the review can be made easier is by reordering the commits to make the isminetype enum changes appear together because the review is done in the order of the commits presented. Suggested order (builds fine at the second commit below on my system):

  63. achow101 renamed this:
    wallet: Remove watchonly behavior and isminetypes
    wallet: Remove isminetypes
    on Jul 8, 2025
  64. achow101 commented at 6:19 pm on July 8, 2025: member

    I believe that the PR title and more importantly the PR description should be updated to remove the stuff related to watch-only as that code removal was done in another PR. The last paragraph of the description can be kept.

    Updated

  65. interfaces, gui: Remove is_mine output parameter from getAddress 
    The is_mine output parameter is never used by any callers.
    725aa16c72
  66. wallet: Remove COutput::spendable and AvailableCoinsListUnspent 
    In descriptor wallets, we consider all outputs to be spendable as we no
longer have mixed mine and watchonly in a wallet. As such,
COutput::spendable is meaningless and can be removed.

Furthermore, CoinFilterParams::only_spendable can be removed as that was
essentially checking for COutput::spendable.

Lastly, AvailableCoinsListUnspent can also be removed as the wrapper is
now only setting the feerate to std::nullopt which is trivial enough that
a dedicated wrapper is not needed.
    a9d2c52f53
  67. wallet: Remove ISMINE_USED 
    This isminetype is not a real isminetype as it is never returned by
IsMine. This is only used for isminefilters in one function, which can
be better represented with a bool parameter avoid_reuse.
    ac7c6a5b09
  68. wallet: Remove isminetype 
    Since the only remaining isminetypes are ISMINE_NO and ISMINE_SPENDABLE,
this enum is now just a bool and can be removed. IsMine is changed to
return a bool and any usage of isminetypes and isminefilters are changed
to be the remaining ISMINE_SPENDABLE case.
    d9ca23fb6f
  69. achow101 force-pushed on Jul 8, 2025
  70. achow101 commented at 6:42 pm on July 8, 2025: member
    Reordered as suggested


achow101 DrahtBot fanquake rkrux maflcko w0xlt

Labels
Wallet

Milestone
30.0

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: 2025-07-11 06:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me