wallet: Remove watchonly behavior and isminetypes

achow101 commented at 1:53 am on May 16, 2025: member

Descriptor wallets do not use the watchonly behavior as it is not possible to mix watchonly and non-watchonly in a descriptor wallet. With legacy wallets now removed, all of the watchonly handling and reporting code is no longer needed. This PR removes watchonly options and results from the RPCs and the handling of watchonly things from the wallet’s internals.

Additionally, ISMINE_USED is removed as its use is only as a filter for cached balances. This PR changes the caching to utilize bools and explicit member variables to account for the avoid_reuse case so that ISMINE_USED is no longer necessary.

With both ISMINE_USED and ISMINE_WATCHONLY being no longer necessary, the entire isminetype enum can be deleted as well, with the various IsMine() functions now returning bools.

This builds upon #32459 which removes most of the watchonly things from the GUI.

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. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31664 (Fees: add Fee rate Forecaster Manager by ismaelsadeeq)
#30157 (Fee Estimation via Fee rate Forecasters by ismaelsadeeq)
#29062 (Wallet: (Refactor) GetBalance to calculate used balance by BrandonOdiwuor)
#27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
#27286 (wallet: Keep track of the wallet’s own transaction outputs in memory by achow101)
#25269 (wallet: re-activate “AmountWithFeeExceedsBalance” error by furszy)

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.

DrahtBot added the label Wallet on May 16, 2025

DrahtBot added the label CI failed on May 16, 2025

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.

maflcko added this to the milestone 30.0 on May 16, 2025

DrahtBot added the label Needs rebase on May 16, 2025

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.

achow101 force-pushed on May 16, 2025

DrahtBot removed the label Needs rebase on May 16, 2025

DrahtBot removed the label CI failed on May 16, 2025

DrahtBot added the label Needs rebase on May 19, 2025

achow101 force-pushed on May 20, 2025

DrahtBot removed the label Needs rebase on May 20, 2025

DrahtBot added the label Needs rebase on May 21, 2025

achow101 force-pushed on May 21, 2025

achow101 commented at 5:51 pm on May 21, 2025: member

Rebased, ready for review.

achow101 marked this as ready for review on May 21, 2025

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

DrahtBot removed the label Needs rebase on May 21, 2025

DrahtBot added the label CI failed on May 21, 2025

in src/wallet/rpc/addresses.cpp:404 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.

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.

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.

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

in src/wallet/rpc/coins.cpp:702 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.

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.

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.

achow101 force-pushed on May 22, 2025

DrahtBot removed the label CI failed on May 22, 2025

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

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

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

in src/wallet/rpc/addresses.cpp:382 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

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.

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

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

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

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

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

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.

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

achow101 marked this as a draft on May 26, 2025

achow101 force-pushed on May 26, 2025

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

achow101 force-pushed on May 27, 2025

DrahtBot added the label Needs rebase on Jun 4, 2025

achow101 force-pushed on Jun 4, 2025

DrahtBot removed the label Needs rebase on Jun 5, 2025

test: Watchonly wallets should estimate larger size

Test that when a watchonly wallet and the wallet with private keys fund
the same tx, the watchonly wallet should use a higher fee since it
should be estimating the size to be larger as it assumes the signer
cannot grind the R value.

9dc47f9955

wallet: Wallets without private keys cannot grind R 4422f90467

wallet: Remove watchonly balances

Descriptor wallets do not have mixed mine and watchonly, so there is no
need to report a watchonly balance.

645f1d5d86

wallet, rpc: Remove watchonly from RPCs

Descriptor wallets don't have a conception of watchonly within a wallet,
so remove all of these options and results from the RPCs

cf34cb9922

wallet, spend: Remove fWatchOnly from CCoinControl

Descriptor wallets do not have a concept of watchonly within a wallet,
so remove the watchonly option from coin control.

e277f4fbb0

wallet: Remove ISMINE_WATCH_ONLY

ISMINE_WATCH_ONLY has been removed from all places it was being used,
and migration does not need ISMINE_WATCH_ONLY, so remove the enum.

81d496bdef

doc: Release note for removed watchonly parameters and results 2aa5c97ba7

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.

92eb7fffee

interfaces, gui: Remove is_mine output parameter from getAddress

The is_mine output parameter is never used by any callers.

badb482305

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.

3b2fc693ec

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.

4217eebbd3

achow101 force-pushed on Jun 14, 2025

wallet: Remove watchonly behavior and isminetypes #32523

Code Coverage & Benchmarks

Reviews

Conflicts