The sendtoaddress RPC currently calculates the entire wallet balance
to ensure that the wallet has enough funds to handle a sendtoaddress/
SendMoney() request. For large wallets, this can take quite a while and is
unnecessary. Instead, we can just check that we have more money than we are
trying to send (plus fees). This improves sendtoaddress speed by 13% in
my regtest spam tests.
Details
Details
- Reviewers
deadalnix markblundeberg - Group Reviewers
Restricted Project
test_runner.py
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- stresstest
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 5814 Build 9690: Bitcoin ABC Buildbot (legacy) Build 9689: arc lint + arc unit
Event Timeline
Comment Actions
Note: FlameGraph performance testing shows that this code changes CPU usage by GetBalance at 7.88% of total before this change, and CheckBalance at 0.10% after this change. (Only 45% of CPU was used by bitcoind/bitcoin-cli in my test; most of the rest is idle.) This is for a wallet with many thousands of UTXOs.
| src/wallet/wallet.cpp | ||
|---|---|---|
| 2193 | There are no unit tests in src/wallet/tests/ that create transactions for which the IsTrusted() check can be passed. There are no unit tests in src/tests/ that have any wallet code. I spent a couple hours trying to hack something together on Monday with no success yet. Core has some new wallet unit tests that look like they will be easier to add a CheckBalance test to, but they have several other commits as dependencies and are not conducive to cherry picking or copy-pasting. I'll take a look at the unit tests again later, but I won't have time for this for a while, maybe a week. :/ | |
| test/functional/wallet_basic.py | ||
| 114 | The check on the new code is done by the sendtoaddress lines. sendtoaddress is the only RPC call that runs the new code (CheckBalance), and no longer runs the old code (GetBalance). If ::CheckBalance(..) returns the wrong results (and disagrees with ::GetBalance()), then the sendtoaddress calls will either succeed when they should fail (lines 100 and 107) or fail when they should succeed (line 113). The nodes[1].getbalance asserts are just there to sanity check the python code itself, and to make sure that the wallet contains as much as it was supposed to contain at that point in execution according to when the RPC test was written. | |
Comment Actions
I'm seeing that Core did a big refactor of GetBalance in https://github.com/bitcoin/bitcoin/pull/15747 .
Presumably we'll want to catch up to that at some point, so will this code end up discarded? If you can make the requisite backports to catch up this part of the code and then apply your changes it would be much more convenient for ABC. :-)
Comment Actions
I think given the current backporting efforts we should put this on back burner for now. I'll put 'request changes' just to get it out of review queue.