wallet, follow-up: Refactor IsSpent to use HowSpent #35151

musaHaruna commented at 12:53 PM on April 24, 2026: contributor

Follow-up to #33671

This PR refactors CWallet::IsSpent to reuse the logic already implemented in CWallet::HowSpent.

Instead of manually iterating over mapTxSpends and checking transaction states, IsSpent now simply returns:

return HowSpent(outpoint) != SpendType::UNSPENT;

Also move CWallet::IsSpent from wallet.cpp to wallet.h as a small inline helper, since it is a one-line wrapper around HowSpent.

DrahtBot commented at 12:53 PM on April 24, 2026: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept NACK	luke-jr
Concept ACK	polespinasa

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#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.

musaHaruna force-pushed on Apr 24, 2026

DrahtBot added the label CI failed on Apr 24, 2026

DrahtBot commented at 1:02 PM on April 24, 2026: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/actions/runs/24890514354/job/72881342200 LLM reason (✨ experimental): CI failed the trailing_whitespace lint check due to trailing whitespace in src/wallet/wallet.cpp.

<details><summary>Hints</summary>

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.

</details>

in src/wallet/wallet.cpp:782 in ba4f82874e

 790 | -            if (!wtx.isAbandoned() && !wtx.isBlockConflicted() && !wtx.isMempoolConflicted())
 791 | -                return true; // Spent
 792 | -        }
 793 | -    }
 794 | -    return false;
 795 | +    return HowSpent(outpoint) != SpendType::UNSPENT;

polespinasa commented at 1:46 PM on April 24, 2026:

Given that it is a one-line function I think is fine to remove it from wallet.cpp and add the logic in wallet.h

polespinasa commented at 1:50 PM on April 24, 2026: member

concept ACK

The refactor is nice and clean.

I think it is worth adding in the commit message that this refactor has a super super small performance downgrade, but we can take it as it is negligible. Now we don't early exit if we found any condition that tells us that a coin is spent.

DrahtBot removed the label CI failed on Apr 24, 2026

in src/wallet/wallet.cpp:788 in ba4f82874e

 796 |  }
 797 |  
 798 |  CWallet::SpendType CWallet::HowSpent(const COutPoint& outpoint) const
 799 |  {
 800 |      SpendType st{SpendType::UNSPENT};
 801 | +    const auto range = mapTxSpends.equal_range(outpoint);

polespinasa commented at 1:57 PM on April 24, 2026:

I don't think the changes inside HowSpent are necessary. It can distract from what is really being changed.

musaHaruna commented at 2:00 PM on April 24, 2026:

What if I move the change to a separate commit and update everything appropriately? Would that work?

polespinasa commented at 2:04 PM on April 24, 2026:

It's just the changes are not necessary for the PR. To simplify IsSpent() you don't need to touch HowSpent().

I don't agree with them anyway. I am not a super fan of using auto it makes code less readable as you don't know what you are using. But that's a personal opinion.

musaHaruna commented at 2:08 PM on April 24, 2026:

Yeah. Also not a big fan of using auto. I will address all the suggestion in the next push. Thank you for the review.

musaHaruna force-pushed on Apr 24, 2026

musaHaruna commented at 3:09 PM on April 24, 2026: contributor

Force pushed 8722fad

I think it is worth adding in the commit message that this refactor has a super super small performance downgrade, but we can take it as it is negligible. Now we don't early exit if we found any condition that tells us that a coin is spent.

Updated the commit message as suggested.

Given that it is a one-line function I think is fine to remove it from wallet.cpp and add the logic in wallet.h

Move CWallet::IsSpent from wallet.cpp to wallet.h

in src/wallet/wallet.h:569 in 8722fad461

 565 | +    /**
 566 | +     * Outpoint is spent if any non-conflicted transaction
 567 | +     * spends it:
 568 | +     */
 569 | +    bool IsSpent(const COutPoint& outpoint) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet)
 570 | +    {

polespinasa commented at 3:42 PM on April 24, 2026:

There's an extra space here, that breaks the lint. I think you can do a one line to occupy less space, similar to SteadyClock::duration ScanningDuration():

    /** Outpoint is spent if any non-conflicted transaction spends it */
    bool IsSpent(const COutPoint& outpoint) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { return HowSpent(outpoint) != SpendType::UNSPENT; }

musaHaruna commented at 10:00 AM on April 25, 2026:

Fixed. Thank you!

musaHaruna force-pushed on Apr 24, 2026

DrahtBot added the label CI failed on Apr 24, 2026

DrahtBot commented at 3:56 PM on April 24, 2026: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/actions/runs/24896525104/job/72902929770 LLM reason (✨ experimental): CI failed because the trailing_whitespace lint check detected trailing whitespace in src/wallet/wallet.h (line 569).

<details><summary>Hints</summary>

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.

</details>

musaHaruna force-pushed on Apr 25, 2026

wallet: Refactor IsSpent to use HowSpent

Simplify CWallet::IsSpent by delegating to CWallet::HowSpent.

Previously, IsSpent manually iterated over mapTxSpends and applied
logic to determine whether an output was spent. This logic is now
centralized in HowSpent, which classifies spends into CONFIRMED,
MEMPOOL, NONMEMPOOL, or UNSPENT.

There is a very small potential performance regression because HowSpent
fully evaluates all relevant entries instead of early-exiting on first
match. However, this is considered negligible in practice given the small
size of mapTxSpends per outpoint in typical wallet usage.

Move CWallet::IsSpent from wallet.cpp to wallet.h as a small inline helper,
since it is a one-line wrapper around HowSpent.

Co-authored-by: w0xlt <94266259+w0xlt@users.noreply.github.com>

9775318519

musaHaruna force-pushed on Apr 25, 2026

DrahtBot removed the label CI failed on Apr 25, 2026

luke-jr commented at 6:54 PM on April 25, 2026: member

Concept NACK. You've made it much less efficient. Not only does it perform more checks, it also doesn't exit the loop early.

polespinasa commented at 9:45 AM on April 27, 2026: member

Not only does it perform more checks

I don't think the extra checks is a performance decrease. If I am not mistaken isConfirmed() and InMempool() only read a value already loaded in CWalletTx.

it also doesn't exit the loop early.

I am not sure how bad it would be to ignore the early exit. But anyway it's easily solvable by adding a bool arg saying if we are checking for the exact HowSpent or only if IsSpent.

    SpendType HowSpent(const COutPoint& outpoint, const bool isSpent = false) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);

    /** Outpoint is spent if any non-conflicted transaction spends it */
    bool IsSpent(const COutPoint& outpoint) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { return HowSpent(outpoint, /*isSpent=*/true) != SpendType::UNSPENT; }

CWallet::SpendType CWallet::HowSpent(const COutPoint& outpoint, bool isSpent) const
{
    SpendType st{SpendType::UNSPENT};

    std::pair<TxSpends::const_iterator, TxSpends::const_iterator> range;
    range = mapTxSpends.equal_range(outpoint);

    for (TxSpends::const_iterator it = range.first; it != range.second; ++it) {
        const Txid& txid = it->second;
        const auto mit = mapWallet.find(txid);
        if (mit != mapWallet.end()) {
            const auto& wtx = mit->second;
            if (wtx.isConfirmed()) return SpendType::CONFIRMED;
            if (wtx.InMempool()) {
                st = SpendType::MEMPOOL;
            } else if (!wtx.isAbandoned() && !wtx.isBlockConflicted() && !wtx.isMempoolConflicted()) {
                if (st == SpendType::UNSPENT) st = SpendType::NONMEMPOOL;
            }
            // If we are only checking for spent condition and not how the coin is spent
            // early exit as fast as we know if it is spent.
            if (isSpent && st != SpendType::UNSPENT) return st;
        }
    }
    return st;
}

wallet: Add early-exit optimization to HowSpent for IsSpent

Introduce an optional `isSpent` flag to HowSpent to allow early
exit when only checking whether an outpoint is spent.

This restores the early-return behavior previously present in
IsSpent, avoiding unnecessary iteration when a spent condition
is already known.

775964ef02

musaHaruna force-pushed on Apr 30, 2026

DrahtBot added the label CI failed on Apr 30, 2026

DrahtBot commented at 3:41 PM on April 30, 2026: contributor

🚧 At least one of the CI tasks failed. Task tidy: https://github.com/bitcoin/bitcoin/actions/runs/25172360438/job/73795063472 LLM reason (✨ experimental): CI failed due to a clang-tidy error (readability-avoid-const-params-in-decls) in wallet/wallet.h where parameter isSpent is const-qualified in the declaration.

<details><summary>Hints</summary>

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.

</details>

DrahtBot removed the label CI failed on Apr 30, 2026

musaHaruna commented at 5:15 PM on April 30, 2026: contributor

Added commit 775964e as suggested by @polespinasa to address the early-exit concern raised in review.

HowSpent now takes an optional isSpent flag, allowing it to return early when used by IsSpent, restoring the previous early-exit behavior while still keeping the refactor.

Thank you so much @polespinasa for the reviews and suggestions. I will keep the changes like this to see what other reviewers thinks about it.

polespinasa commented at 6:15 PM on April 30, 2026: member

Great!

The commits can be squashed into one single commit. Each commit should be good for himself and the first one is a behavior change so it needs the changes in the second :)