Skip to content

Conversation

@ajtowns
Copy link
Contributor

@ajtowns ajtowns commented Oct 21, 2025

Changes getbalances to report the sum of txos spent by transactions that aren't confirmed nor in the mempool (eg due to being part of too long a mempool chain, or spending non-standard outputs, or having a datacarrier output that exceeds -datacarriersize, etc). Those values are added to the trusted/untrusted_pending/immature/used fields as appropriate (where previously they were skipped), and subtracted from the new nonmempool field, so that the sum of all fields remains the same.

For example:

$ bitcoin-cli -regtest getbalances
{
  "mine": {
    "trusted": 6049.99999220,
    "untrusted_pending": 0.00000000,
    "immature": 3200.00000780,
    "nonmempool": -100.00000000
  },
  "lastprocessedblock": {
    "hash": "3ab4582226d5e8ad76438db48d76e822c31bce2cdbc7ba82a5d974a277515d0d",
    "height": 221
  }
}

Closes #11887

@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 21, 2025

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/33671.

Reviews

See the guideline for information on the review process.

Type Reviewers
Concept NACK achow101
Concept ACK vasild, RandyMcMillan

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #bitcoin-core/gui/911 (Adds non-mempool wallet balance to overview by ajtowns)
  • #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.

LLM Linter (✨ experimental)

Possible places where comparison-specific test macros should replace generic comparisons:

  • test/functional/wallet_send.py: assert from_wallet.getbalances()["mine"]["nonmempool"] < 0 -> recommendation: replace with a comparison macro, e.g. assert_greater_than(0, from_wallet.getbalances()["mine"]["nonmempool"])

2026-01-07

@ajtowns
Copy link
Contributor Author

ajtowns commented Oct 21, 2025

Relevant for #29415 -- private relay of our wallet transactions would create spends that weren't in the mempool and would thus disappear mysteriously from the wallet balance which seems unacceptable. I think I've seen similar behaviour when doing lots of consolidations of my signet wallet, which has also been disturbing.

Perhaps see #11020 for a similar previous attempt.

This PR currently doesn't include tests that this works sensibly for nonzero nonmempool balances, hence wip/draft.

Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

This extends the getbalances RPC. Would be good to also update the GUI (not necessary in this PR).

The new type of transactions would have somehow to be fed to the wallet so that they would be returned by wallet.GetTXOs(), called by GetBalance(). How would that be done?

@RandyMcMillan
Copy link
Contributor

Concept ACK

@ajtowns ajtowns force-pushed the 202510-wallet-unconf-bal branch from 9d9a0a6 to 5209273 Compare November 19, 2025 00:28
@ajtowns
Copy link
Contributor Author

ajtowns commented Nov 19, 2025

This extends the getbalances RPC. Would be good to also update the GUI (not necessary in this PR).

Okay, I have a patch for that opened as draft at bitcoin-core/gui#911

The new type of transactions would have somehow to be fed to the wallet so that they would be returned by wallet.GetTXOs(), called by GetBalance(). How would that be done?

I don't think there's a way to do this for arbitrary txs that originate outside of the wallet, even if they involve a wallet address. Maybe there should be? (ie, an RPC that lets you pass raw tx to AddToWalletIfInvolvingMe ?)

But for txs that are created by the wallet, eg send or sendall rpc, then I believe they'll be added to the wallet via CommitTransaction which will also invoke SubmitTxMemoryPoolAndRelay() which in turn invokes broadcastTransaction.

You can create non-mempool txs by limiting -datacarriersize on startup, then using bitcoin-cli send to create a tx with a {"data":"deadbeef..."} output that exceeds the datacarriersize you set. Those will not be rejected by the rpc, and will enter the wallet as described above, but not enter the mempool. If you restart with a sufficiently larger datacarriersize, the tx will enter the mempool (and be trusted, since it's from you, to you).

I've added a test to wallet_send.py that exercises this behaviour.

@ajtowns ajtowns marked this pull request as ready for review November 19, 2025 01:21
@ajtowns ajtowns changed the title [wip] wallet: Add separate balance info for non-mempool wallet txs wallet: Add separate balance info for non-mempool wallet txs Nov 19, 2025
@achow101
Copy link
Member

Concept NACK-ish

There's a lot of reasons that an unconfirmed transaction might be in the wallet but not the mempool, and I think giving an accurate balance of those transactions will be hard to get right. We absolutely do not want to mislead users into thinking they have more coins than they actually do, so I am hesitant to add something like this.

From a cursory glance at the code, I think 2 transactions that conflict with each other but aren't in the mempool would both get counted towards the nonmempool balance, and which I think is incorrect. But in that case, which one should be counted, if at all?

@ajtowns
Copy link
Contributor Author

ajtowns commented Nov 20, 2025

Concept NACK-ish

There's a lot of reasons that an unconfirmed transaction might be in the wallet but not the mempool, and I think giving an accurate balance of those transactions will be hard to get right. We absolutely do not want to mislead users into thinking they have more coins than they actually do, so I am hesitant to add something like this.

We currently mislead users into thinking they have less coins than they actually do (due to transactions in the wallet with long mempool chains, eg), which is very scary, and (IMO) very bad from an accounting POV. So I don't think the current state is acceptable either.

The underlying problem (IMO) is that the accounting is unbalanced: if we see two transactions (A,B) in the wallet, where B spends A (via IsSpent()) but B is not in the mempool, then because B spends A we will ignore our funds from A (believing it's spent by B) but we will also ignore our funds from B (believing it's not a real tx). There's two ways to fix that: we could not ignore the funds that are still in A, or we could not ignore the funds that are in B. Currently this PR tries not ignoring the funds in B.

From a cursory glance at the code, I think 2 transactions that conflict with each other but aren't in the mempool would both get counted towards the nonmempool balance, and which I think is incorrect. But in that case, which one should be counted, if at all?

That's accurate:

$ bitcoin-cli -regtest -named send outputs='{"bcrt1qperny2w6xvfkjvec44w4qtwwk3ykuw9py0ejrp": 1.0, "data": "54686520717569636b2062726f776e20666f78206a756d7073206f76657220746865206c617a7920646f67"}' fee_rate=1 inputs='[{"txid":"a6e88286d165dbe4f4aab7830f7b68931ef7f5b66d32f0ea573d906594ad6a0c","vout":0}]'
{
  "txid": "42ef1c7d7c2afee4bb2519fbfcf284ef40ae156ea193ce86a1fa670183a12322",
  "complete": true
}
$ bitcoin-cli -regtest -named send outputs='{"bcrt1qperny2w6xvfkjvec44w4qtwwk3ykuw9py0ejrp": 2.0, "data": "54686520717569636b2062726f776e20666f78206a756d7073206f76657220746865206c617a7920646f67"}' fee_rate=2 inputs='[{"txid":"a6e88286d165dbe4f4aab7830f7b68931ef7f5b66d32f0ea573d906594ad6a0c","vout":0}]'
{
  "txid": "ebbd3bf4807d8e5683d124cd726de982a4f36e2bae264ff5b91545dae41bacbe",
  "complete": true
}
$ bitcoin-cli -regtest getbalances
{
  "mine": {
    "trusted": 450.00000000,
    "untrusted_pending": 0.00000000,
    "immature": 5000.00000000,
    "nonmempool": 99.99999415
  },
  "lastprocessedblock": {
    "hash": "33e1b4932fb3a464aef972470ffed7650795a35f26214cfa58f76a787d548768",
    "height": 110
  }
}

ie spending a single 50 rBTC output twice suddenly gets reported as ~100 rBTC ex fees.

I don't think there's an easy way to avoid that with the "don't ignore B's funds" approach: if you have confirmed utxos A (1 BTC), B (2 BTC), C (3 BTC), you could have non-mempool txs D spending A,B (3 BTC out, 2.9 BTC change) and E spending A,C (4 BTC 3.8 BTC change). You subtract 6 BTC from your balance because each of A,B,C have non-abandoned, non-block/mempool-conflicted spends in the wallet, but if D were to confirm you should add 5.9 BTC (D+C), if E were to confirm you should add 5.8 BTC (E+B), and if you pretended both were to confirm you'd add 6.7 BTC (D+E).

The other approach, then, is to say "D and E aren't in the mempool, so A B and C aren't -really- spent", and not remove the 6 BTC from A,B,C from your balance; though IMO it still makes sense to separate those out into "possibly-spent-by-nonmempool-txs". I believe that easily avoids the double counting if you have conflicting possible spends.

With that approach, if I spend an immature coinbase, that changes my balances from:

$ bitcoin-cli -regtest getbalances
{
  "mine": {
    "trusted": 450.00000000,
    "untrusted_pending": 0.00000000,
    "immature": 5000.00000000,
    "nonmempool": 50.00000000
  },
  "lastprocessedblock": {
    "hash": "33e1b4932fb3a464aef972470ffed7650795a35f26214cfa58f76a787d548768",
    "height": 110
  }
}
$ bitcoin-cli -regtest getbalances
{
  "mine": {
    "trusted": 450.00000000,
    "untrusted_pending": 0.00000000,
    "immature": 4950.00000000,
    "nonmempool": 100.00000000
  },
  "lastprocessedblock": {
    "hash": "33e1b4932fb3a464aef972470ffed7650795a35f26214cfa58f76a787d548768",
    "height": 110
  }
}

Double spending the same tx at that point doesn't change the result. If I do the "A,B,C" (50 rBTC each) spent by conflicting "D,E", I get:

$ bitcoin-cli -regtest getbalances
{
  "mine": {
    "trusted": 5949.99999220,
    "untrusted_pending": 0.00000000,
    "immature": 3200.00000780,
    "nonmempool": 100.00000000
  },
  "lastprocessedblock": {
    "hash": "3ab4582226d5e8ad76438db48d76e822c31bce2cdbc7ba82a5d974a277515d0d",
    "height": 221
  }
}
...
$ bitcoin-cli -regtest getbalances
{
  "mine": {
    "trusted": 5899.99999220,
    "untrusted_pending": 0.00000000,
    "immature": 3200.00000780,
    "nonmempool": 150.00000000
  },
  "lastprocessedblock": {
    "hash": "3ab4582226d5e8ad76438db48d76e822c31bce2cdbc7ba82a5d974a277515d0d",
    "height": 221
  }
}

That approach is still "misleading" in the sense that those nonmempool txs are reported as how much money you're spending, rather than how much you're potentially keeping as change if those spends go through.

Another approach would be to report it as:

trusted: 6049.99
untrusted_pending: 0
immature: 3200
nonmempool-pending: -150

ie "your balance is this, but you have X funds that are currently not in your mempool that you might lose some/all of"

Could also perhaps do it more fine-grained as:

trusted: 6049.99 [-100 non-mempool spends]
untrusted_pending: 0
immature: 3200 [-50 non-mempool spends]

[EDIT: You could perhaps also account for locked coins similarly to non-mempool spends, which might be logical if you're locking coins because you're spending them elsewhere in ways you don't expect will make it to your mempool]

Happy to switch to one of those approaches, or something else, if that seems more acceptable.

@achow101
Copy link
Member

Another approach would be to report it as:

trusted: 6049.99
untrusted_pending: 0
immature: 3200
nonmempool-pending: -150

ie "your balance is this, but you have X funds that are currently not in your mempool that you might lose some/all of"

I would prefer this approach and docs just need to be clear that some of that amount may come back as change.

@ajtowns ajtowns force-pushed the 202510-wallet-unconf-bal branch from 7af1215 to e720037 Compare November 27, 2025 05:36
@ajtowns
Copy link
Contributor Author

ajtowns commented Nov 27, 2025

Okay, so when a "transfer to self" tx paying 1310 sats in fees from 100 BTC of inputs doesn't hit the mempool, it currently looks like:

{
  "mine": {
    "trusted": 5949.99999220,
    "untrusted_pending": 0.00000000,
    "immature": 3200.00000780
  }
}

vs when the tx does hit the mempool:

{
  "mine": {
    "trusted": 6049.99997910,
    "untrusted_pending": 0.00000000,
    "immature": 3200.00000780
  }
}

That is, there's 99.9999869 BTC missing from your balance while the transaction is in the wallet but not in the mempool.

With the current version of this PR, it now looks like this when the tx is not in the mempool:

{
  "mine": {
    "trusted": 6049.99999220,
    "untrusted_pending": 0.00000000,
    "immature": 3200.00000780,
    "nonmempool": -100.00000000
  }
}

So if you add those figures up, you're still 100 BTC down, but at least you have a clue as to what's going on.

When it does hit the mempool (or gets mined), it becomes:

{
  "mine": {
    "trusted": 6049.99997910,
    "untrusted_pending": 0.00000000,
    "immature": 3200.00000780,
    "nonmempool": 0.00000000
  }
}

which is the same as previously (with the addtion of nonmempool=0).

I also refactored it to calculate the "used" value directly (ie funds held in coins with addresses that are already spent from, for wallets that care about that), since I couldn't figure out how to get the nonmempool figure correct otherwise, but I think that's an improvement anyway.

I believe there was a "bug" in the cleanup decorator for wallet_v3_txs in that coins in a wallet spent by non-mempool wallet txs wouldn't be cleared out by the sendall calls. That wasn't showing up in the balance checks previously, but does show up when the nonmempool figures are also reported. Addressed that by abandoning any unconfirmed wallet txs prior to calling sendall.

@ajtowns ajtowns force-pushed the 202510-wallet-unconf-bal branch from e720037 to 815a1fc Compare November 27, 2025 06:41
@DrahtBot
Copy link
Contributor

🚧 At least one of the CI tasks failed.
Task test each commit: https://github.com/bitcoin/bitcoin/actions/runs/19727553059/job/56521733216
LLM reason (✨ experimental): Git rebase failed due to uncommitted changes in the index.

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.

}
if (bucket) {
// Get the amounts for mine
CAmount credit_mine = txo.GetTxOut().nValue;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 81e763f "wallet: Have GetBalance report used amount directly without two calls"

This could be brought inline with *bucket += credit_mine; below as credit_mine is only ever used there.

Copy link
Contributor Author

@ajtowns ajtowns Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value is used twice, once for *bucket += and once for ...nonmempool -= ? EDIT: ah, I guess that only happens in the following commit. Seems fine to leave though?

@ajtowns ajtowns force-pushed the 202510-wallet-unconf-bal branch from e720037 to 7a05458 Compare January 7, 2026 12:57
Uses send rpc to create a tx with oversized OP_RETURN output, verifies
that it doesn't enter the mempool, and that getbalance rpc returns a
nonmempool value.
@ajtowns ajtowns force-pushed the 202510-wallet-unconf-bal branch from 7a05458 to f77bf4c Compare January 7, 2026 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Could the wallet count unconfirmed non-mempool change?

6 participants