wallet: Remove isminetypes #32523

achow101 · 2025-05-16T01:53:31Z

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.

DrahtBot · 2025-05-16T01:53:34Z

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.

DrahtBot · 2025-05-16T02:28:17Z

🚧 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.}

Hints

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.

fanquake · 2025-05-16T10:56:41Z

 /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]
  474 |     return AvailableCoins(wallet, coinControl, /*feerate=*/ std::nullopt, params);
      |            ^
1 error generated.

achow101 · 2025-05-21T17:51:43Z

Rebased, ready for review.

rkrux · 2025-05-21T18:13:49Z

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.

d637af8#diff-d41d68c5a65d67956c91b33ca86da7df1981d84a0b15b4a186deea566563fed5R372

achow101 · 2025-08-14T22:40:03Z

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

fjahr · 2025-08-14T22:48:12Z

utACK 8fba2fa

rkrux

re-ACK 8fba2fa

git range-diff d9ca23f...8fba2fa

The CI error seems unrelated to me.

The is_mine output parameter is never used by any callers.

enirox001

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?

achow101 · 2025-08-19T18:11:29Z

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)

fjahr · 2025-08-19T20:25:35Z

re-ACK 620abe9

murchandamus

Concept ACK

src/wallet/spend.cpp

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.

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.

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.

murchandamus · 2025-08-19T22:16:36Z

ACK be776a1

enirox001

re-ACK be776a1

jlest01

reACK be776a1

fjahr · 2025-08-20T22:13:42Z

reACK be776a1

davidgumberg · 2025-08-21T00:03:11Z

crACK be776a1

Updated Namecoin-specific wallet code for the upstream removal of isminetype (bitcoin/bitcoin#32523).

DrahtBot added Wallet CI failed labels May 16, 2025

maflcko added this to the 30.0 milestone May 16, 2025

DrahtBot added the Needs rebase label May 16, 2025

DrahtBot mentioned this pull request May 16, 2025

qt: drop unused watch-only functionality #32459

Merged

achow101 force-pushed the delete-isminetypes branch from 717afa2 to 75729d4 Compare May 16, 2025 18:14

DrahtBot added Needs rebase and removed Needs rebase CI failed labels May 16, 2025

achow101 force-pushed the delete-isminetypes branch from 75729d4 to 6a77821 Compare May 20, 2025 00:55

DrahtBot added Needs rebase and removed Needs rebase labels May 20, 2025

achow101 force-pushed the delete-isminetypes branch from 6a77821 to f3ab751 Compare May 21, 2025 17:51

achow101 marked this pull request as ready for review May 21, 2025 17:51

achow101 force-pushed the delete-isminetypes branch from d9ca23f to 8fba2fa Compare August 14, 2025 22:39

DrahtBot requested review from w0xlt and rkrux August 14, 2025 22:48

rkrux approved these changes Aug 15, 2025

View reviewed changes

DrahtBot added the Needs rebase label Aug 19, 2025

interfaces, gui: Remove is_mine output parameter from getAddress

620abe9

The is_mine output parameter is never used by any callers.

achow101 force-pushed the delete-isminetypes branch from 8fba2fa to 65f1089 Compare August 19, 2025 17:30

enirox001 reviewed Aug 19, 2025

View reviewed changes

DrahtBot removed the Needs rebase label Aug 19, 2025

murchandamus reviewed Aug 19, 2025

View reviewed changes

src/wallet/spend.cpp Outdated Show resolved Hide resolved

achow101 added 3 commits August 19, 2025 14:49

wallet: Remove ISMINE_USED

009a69a

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.

wallet: Remove isminetype

be776a1

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.

achow101 force-pushed the delete-isminetypes branch from 65f1089 to be776a1 Compare August 19, 2025 21:52

DrahtBot requested review from fjahr, rkrux and enirox001 August 19, 2025 22:16

DrahtBot mentioned this pull request Aug 20, 2025

refactor: rename fees.{h, cpp} to fees/block_policy_estimator{h, cpp} #33218

Open

enirox001 reviewed Aug 20, 2025

View reviewed changes

jlest01 reviewed Aug 20, 2025

View reviewed changes

fanquake merged commit 8333aa5 into bitcoin:master Aug 21, 2025
19 checks passed

kevkevinpal mentioned this pull request Aug 21, 2025

wallet: Remove isminetypes kevkevinpal/bitcoin#134

Open

domob1812 added a commit to domob1812/namecoin-core that referenced this pull request Aug 29, 2025

Merge branch 'auxpow'

724bdfc

Updated Namecoin-specific wallet code for the upstream removal of isminetype (bitcoin/bitcoin#32523).

wallet: Remove isminetypes #32523

wallet: Remove isminetypes #32523

Uh oh!

Conversation

achow101 commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

DrahtBot commented May 16, 2025

Uh oh!

fanquake commented May 16, 2025

Uh oh!

achow101 commented May 21, 2025

Uh oh!

rkrux commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Aug 14, 2025

Uh oh!

fjahr commented Aug 14, 2025

Uh oh!

rkrux left a comment

Choose a reason for hiding this comment

Uh oh!

enirox001 left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Aug 19, 2025

Uh oh!

fjahr commented Aug 19, 2025

Uh oh!

murchandamus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

murchandamus commented Aug 19, 2025

Uh oh!

enirox001 left a comment

Choose a reason for hiding this comment

Uh oh!

jlest01 left a comment

Choose a reason for hiding this comment

Uh oh!

fjahr commented Aug 20, 2025

Uh oh!

davidgumberg commented Aug 21, 2025

Uh oh!

Uh oh!

Uh oh!

achow101 commented May 16, 2025 •

edited

Loading

DrahtBot commented May 16, 2025 •

edited

Loading

rkrux commented May 21, 2025 •

edited

Loading