wallet: Remove isminetypes #32523

pull achow101 wants to merge 4 commits into bitcoin:master from achow101:delete-isminetypes changing 32 files +226 −348
  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 murchandamus, enirox001, jlest01, fjahr, davidgumberg
    Stale ACK w0xlt, rkrux

    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:

    • #33218 (refactor: rename fees.{h, cpp} to fees/block_policy_estimator{h, cpp} by ismaelsadeeq)
    • #31664 (Fees: add Fee rate Forecaster Manager by ismaelsadeeq)
    • #30157 (Fee Estimation via Fee rate Forecasters by ismaelsadeeq)
    • #27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
    • #10102 (Multiprocess bitcoin by ryanofsky)

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

    davidgumberg commented at 10:45 pm on August 20, 2025:

    This is the current behavior anyways.

    That’s surprising!

    Shouldn’t we do something like:

    0entry.pushKV("spendable", pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS));

    instead?

    Otherwise RPC docs should be more like @pablomartin4btc below can be done in a follow-up.

    maflcko commented at 11:13 am on August 28, 2025:
    Ref also https://github.com/bitcoin/bitcoin/issues/22518
  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. achow101 force-pushed on Jul 8, 2025
  66. achow101 commented at 6:42 pm on July 8, 2025: member
    Reordered as suggested
  68. rkrux commented at 2:53 pm on July 11, 2025: contributor

    LGTM ACK d9ca23fb6f3ce6d4863583226f5ae967a7c49497

    I have seen more usages of avoid_reuse: false than its true variant (which has only 1 usage) and every time I have to mentally translate it to “get all/full amount”. Maybe a bool cachable_amount_type might have a place here with ALL and AVOID_REUSE values equalling to false and true respectively. But not suggesting this change because I notice that the term avoid_reuse is quite embedded in the wallet codebase already in terms of RPC arguments and wallet flags, and thus, I feel this term can grow on me with time.

    Also, good that WalletTXO doesn’t need to store anything related to ismine now due to its nature.

  69. in src/wallet/rpc/coins.cpp:676 in d9ca23fb6f outdated 
    675@@ -676,7 +676,7 @@ RPCHelpMan listunspent()
676                 entry.pushKV("ancestorfees", uint64_t(ancestor_fees));
677             }
678         }
679-        entry.pushKV("spendable", out.spendable);
680+        entry.pushKV("spendable", true); // Any coins we list are always spendable
    w0xlt commented at 9:39 pm on August 8, 2025:

    The doc needs to be updated as well. This would close #33110.

    0- {RPCResult::Type::BOOL, "spendable", "Whether we have the private keys to spend this output"},
1+ {RPCResult::Type::BOOL, "spendable", "deprecated (always true)"},
    pablomartin4btc commented at 5:11 pm on August 13, 2025:

    nit (not blocker, only if you have to retouch or could be done on a follow-up):

    0- {RPCResult::Type::BOOL, "spendable", "deprecated (always true)"},
1+ {RPCResult::Type::BOOL, "spendable", "(DEPRECATED) Always true for UTXOs belonging to a descriptor wallet, including ones without private keys"},
    achow101 commented at 10:39 pm on August 14, 2025:
    Done
  70. w0xlt commented at 3:38 pm on August 11, 2025: contributor
    ACK https://github.com/bitcoin/bitcoin/pull/32523/commits/d9ca23fb6f3ce6d4863583226f5ae967a7c49497 (with the above observation)
  71. in src/wallet/transaction.h:157 in ac7c6a5b09 outdated 
    157+    }
158+    CAmount Get(bool avoid_reuse)
159+    {
160+        Assert(m_avoid_reuse_value.has_value() || m_all_value.has_value());
161+        if (avoid_reuse) return m_avoid_reuse_value.value();
162+        return m_all_value.value();
    fjahr commented at 10:06 pm on August 14, 2025:

    This seems unsafe. If we pass avoid_reuse as true but only m_all_value is set, then m_avoid_reuse_value.value() should throw.

    Should probably be something like

    0    if (avoid_reuse) {
1        Assert(m_avoid_reuse_value.has_value());
2        return m_avoid_reuse_value.value();
3    }
4    Assert(m_all_value.has_value());
5    return m_all_value.value();
    achow101 commented at 10:39 pm on August 14, 2025:
    Good point, done.
  72. fjahr commented at 10:23 pm on August 14, 2025: contributor
    Code looks mostly good to me aside from my one comment. I also found a few left-over mentions of isminetype(s) in comments: https://github.com/fjahr/bitcoin/commit/12a1b67460183556c2b9fac427efdef25c3c3a46 (could be fixed in a follow-up though if I am mistaken in my comment).
  73. achow101 force-pushed on Aug 14, 2025
  74. achow101 commented at 10:40 pm on August 14, 2025: member

    I also found a few left-over mentions of isminetype(s) in comments: fjahr@12a1b67 (could be fixed in a follow-up though if I am mistaken in my comment).

    I’m choosing to leave those comments in since they are specifically related to legacy wallet behavior

  75. fjahr commented at 10:48 pm on August 14, 2025: contributor
    utACK 8fba2fafbd561d4a2b743bac0237e1260074cb1b
  76. DrahtBot requested review from w0xlt on Aug 14, 2025
  77. DrahtBot requested review from rkrux on Aug 14, 2025
  78. rkrux approved
  79. rkrux commented at 10:25 am on August 15, 2025: contributor

    re-ACK 8fba2fa

    0git range-diff d9ca23f...8fba2fa

    The CI error seems unrelated to me.

  80. DrahtBot added the label Needs rebase on Aug 19, 2025
  81. interfaces, gui: Remove is_mine output parameter from getAddress 
    The is_mine output parameter is never used by any callers.
    620abe985e
  82. achow101 force-pushed on Aug 19, 2025
  83. enirox001 commented at 6:08 pm on August 19, 2025: contributor

    ACK 620abe9 — built & tested locally; wallet tests passed. Behavior-preserving refactor; removing isminetype in favor of explicit booleans improves clarity.

    Non-blocking nit: wallet_migration.py still references ISMINE_SPENDABLE / ISMINE_NO in comments. Functionally the tests use the boolean ismine; could you briefly explain why you kept the enum names in the comments, or consider changing them to ismine == True / ismine == False for consistency?

  84. achow101 commented at 6:11 pm on August 19, 2025: member

    Non-blocking nit: wallet_migration.py still references ISMINE_SPENDABLE / ISMINE_NO in comments. Functionally the tests use the boolean ismine; could you briefly explain why you kept the enum names in the comments, or consider changing them to ismine == True / ismine == False for consistency?

    #32523 (comment)

  85. DrahtBot removed the label Needs rebase on Aug 19, 2025
  86. in src/wallet/spend.cpp:558 in 1f085fa90a outdated 
    551@@ -563,11 +552,10 @@ std::map<CTxDestination, std::vector<COutput>> ListCoins(const CWallet& wallet)
552 
553     CCoinControl coin_control;
554     CoinFilterParams coins_params;
555-    coins_params.only_spendable = false;
556     coins_params.skip_locked = false;
557     for (const COutput& coin : AvailableCoins(wallet, &coin_control, /*feerate=*/std::nullopt, coins_params).All()) {
558         CTxDestination address;
559-        if ((coin.spendable || (wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && coin.solvable))) {
560+        if (!wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) || coin.solvable) {
    murchandamus commented at 7:03 pm on August 19, 2025:

    In “wallet: Remove COutput::spendable and AvailableCoinsListUnspent” (1f085fa90ae92d1fda437066f802c1fe9c1dfe1c), I was a bit surprised to find this change in the logic in the commit as it does not seem to be explicitly mentioned in the commit message.

    Prior to this change, we would proceed on UTXOs that are spendable or on UTXOs that are solvable while keys are disabled. After this change, we proceed either if keys are enabled OR the UTXO is solvable.

    If we consider all UTXOs in descriptor wallets to be spendable, shouldn’t this check just be obsolete here now?

    achow101 commented at 9:54 pm on August 19, 2025:
    Yes, I believe it is obsolete. Removed.
  87. fjahr commented at 8:25 pm on August 19, 2025: contributor
    re-ACK 620abe985e5150c3151192d08746b7845a69dbbf
  88. murchandamus commented at 9:29 pm on August 19, 2025: contributor
    Concept ACK
  89. 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.
    6a7aa01574
  90. 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.
    009a69a616
  91. 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.
    be776a1443
  92. achow101 force-pushed on Aug 19, 2025
  93. murchandamus commented at 10:16 pm on August 19, 2025: contributor
    ACK be776a1443fdf1a72e0d363c1566d71cb0cda8b5
  94. DrahtBot requested review from fjahr on Aug 19, 2025
  95. DrahtBot requested review from rkrux on Aug 19, 2025
  96. DrahtBot requested review from enirox001 on Aug 19, 2025
  97. enirox001 commented at 1:55 pm on August 20, 2025: contributor
    re-ACK be776a1
  99. fjahr commented at 10:13 pm on August 20, 2025: contributor
    reACK be776a1443fdf1a72e0d363c1566d71cb0cda8b5
  101. fanquake merged this on Aug 21, 2025
  102. fanquake closed this on Aug 21, 2025


achow101 DrahtBot fanquake rkrux davidgumberg maflcko w0xlt pablomartin4btc fjahr enirox001 murchandamus jlest01


fjahr rkrux 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-10-24 12:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me