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:

#32607 (rpc: Note in fundrawtransaction doc, fee rate is for package by benthecarman)
#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)

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

wallet: Remove watchonly balances

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

91f7daf586

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

1ec04d4c4e

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.

8ebf126295

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.

884c6329e3

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.

ccf618bf3b

interfaces, gui: Remove is_mine output parameter from getAddress

The is_mine output parameter is never used by any callers.

b1ee6a287d

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

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.

e6fd003729

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.

6e5f730c81

achow101 force-pushed on May 22, 2025

DrahtBot removed the label CI failed on May 22, 2025

wallet: Remove watchonly behavior and isminetypes #32523

Code Coverage & Benchmarks

Reviews

Conflicts