Page MenuHomePhabricator

[Wallet] CheckBalance instead of GetBalance for 13% faster sendtoaddress
Needs RevisionPublic

Authored by jtoomim on May 11 2019, 06:41.

Details

Reviewers
deadalnix
markblundeberg
Group Reviewers
Restricted Project
Summary

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.

Test Plan

test_runner.py

Diff Detail

Repository
rABC Bitcoin ABC
Branch
stresstest
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 5865
Build 9790: Bitcoin ABC Buildbot (legacy)
Build 9789: arc lint + arc unit

Event Timeline

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.

Adding tests to test/functional/wallet_basic.py for insufficient balance

deadalnix requested changes to this revision.May 12 2019, 23:30
deadalnix added inline comments.
src/wallet/wallet.cpp
2193 ↗(On Diff #8611)

You need to unit test that guy.

Please add a new line before the function definition.

test/functional/wallet_basic.py
114 ↗(On Diff #8611)

My understanding is that this patch do not change any behavior, so how is this testing the new code ?

This revision now requires changes to proceed.May 12 2019, 23:30
src/wallet/wallet.cpp
2193 ↗(On Diff #8611)

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 ↗(On Diff #8611)

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.

Newline after function definition

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

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.

This revision now requires changes to proceed.May 28 2019, 17:58