Skip to content

[wallet] [refactor] Simplify getbalance implementation #9614

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
laanwj merged 2 commits into from
Apr 26, 2017
Merged

[wallet] [refactor] Simplify getbalance implementation #9614

laanwj merged 2 commits into from
Apr 26, 2017

Conversation

@ryanofsky
Copy link
Contributor

@ryanofsky ryanofsky commented Jan 23, 2017

This is just code cleanup, no behavior is changing. It deduplicates getbalance code and makes it comprehsible so it is not doing bizarre things like subtracting negative fees (#9167).

For each wallet transaction, the balance is updated as follows:

+        CAmount debit = wtx.GetDebit(filter);
+        const bool outgoing = debit > 0;
+        for (const CTxOut& out : wtx.tx->vout) {
+            if (outgoing && IsChange(out)) {
+                debit -= out.nValue;
+            } else if (IsMine(out) & filter && depth >= minDepth && (!account || *account == GetAccountName(out.scriptPubKey))) {
+                balance += out.nValue;
+            }
+        }
+
+        if (outgoing && (!account || *account == wtx.strFromAccount)) {
+            balance -= debit;
+        }

jonasschnelli
jonasschnelli reviewed Jan 24, 2017
View reviewed changes
src/wallet/rpcwallet.cpp Outdated
if (request.params.size() == 0)
return ValueFromAmount(pwalletMain->GetBalance());

const string* account = request.params[0].get_str() != "*" ? &request.params[0].get_str() : nullptr;
Copy link
Contributor

@jonasschnelli jonasschnelli Jan 24, 2017

Choose a reason for hiding this comment

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

I think this results in no longer throwing RPC_WALLET_INVALID_ACCOUNT_NAME when someone tries to get balance of "*", instead, it returns back the total balance. But I think this is what we want....

Copy link
Contributor Author

@ryanofsky ryanofsky Jan 24, 2017

Choose a reason for hiding this comment

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

Yes, this is just keeping the current behavior (line 698 before this commit, which is removed in the next commit).

jonasschnelli
jonasschnelli approved these changes Jan 24, 2017
View reviewed changes
Copy link
Contributor

@jonasschnelli jonasschnelli left a comment

Choose a reason for hiding this comment

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

Nice.
utACK 4b49ed6

I could imagine a follow-up refactoring that get rid of all the individual caches...

    mutable bool fDebitCached;
    mutable bool fCreditCached;
    mutable bool fImmatureCreditCached;
    mutable bool fAvailableCreditCached;
    mutable bool fWatchDebitCached;
    mutable bool fWatchCreditCached;
    mutable bool fImmatureWatchCreditCached;
    mutable bool fAvailableWatchCreditCached;
    mutable bool fChangeCached;
    mutable CAmount nDebitCached;
    mutable CAmount nCreditCached;
    mutable CAmount nImmatureCreditCached;
    mutable CAmount nAvailableCreditCached;
    mutable CAmount nWatchDebitCached;
    mutable CAmount nWatchCreditCached;
    mutable CAmount nImmatureWatchCreditCached;
    mutable CAmount nAvailableWatchCreditCached;
    mutable CAmount nChangeCached;

... and manages them in a std::map or similar.

ryanofsky
ryanofsky commented Jan 24, 2017
View reviewed changes
Copy link
Contributor Author

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

I could imagine a follow-up refactoring that get rid of all the individual caches...

I also think this could be good to unify/organize these. #9167 adds two more caches, so they could be integrated as well.

src/wallet/rpcwallet.cpp Outdated
if (request.params.size() == 0)
return ValueFromAmount(pwalletMain->GetBalance());

const string* account = request.params[0].get_str() != "*" ? &request.params[0].get_str() : nullptr;
Copy link
Contributor Author

@ryanofsky ryanofsky Jan 24, 2017

Choose a reason for hiding this comment

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

Yes, this is just keeping the current behavior (line 698 before this commit, which is removed in the next commit).

@ryanofsky ryanofsky force-pushed the pr/getbalance-cleanup branch from 4b49ed6 to e8ce6a4 Compare February 6, 2017 17:42
@ryanofsky
Copy link
Contributor Author

ryanofsky commented Feb 6, 2017

Rebased 4b49ed6 -> e8ce6a4 (getbalance-cleanup.0 -> getbalance-cleanup.1), fixing trivial merge conflict with #9613.

@ryanofsky ryanofsky mentioned this pull request Feb 28, 2017
Closed
@ryanofsky ryanofsky force-pushed the pr/getbalance-cleanup branch from e8ce6a4 to be74f34 Compare March 1, 2017 10:35
@ryanofsky
Copy link
Contributor Author

ryanofsky commented Mar 1, 2017

Rebased e8ce6a4 -> be74f34 (getbalance-cleanup.1 -> getbalance-cleanup.2) because of conflict with #9764.

@ryanofsky ryanofsky force-pushed the pr/getbalance-cleanup branch from be74f34 to 0b9520d Compare March 3, 2017 19:40
@ryanofsky
Copy link
Contributor Author

ryanofsky commented Mar 3, 2017

Rebased be74f34 -> 0b9520d (pr/getbalance-cleanup.2 -> pr/getbalance-cleanup.3) because of conflicts with pwalletMain renames in #9775.

@laanwj
Copy link
Member

laanwj commented Apr 26, 2017 •
edited
Loading

Needs rebase again (sorry!)

utACK otherwise.

@ryanofsky
[wallet] Add GetLegacyBalance method to simplify getbalance RPC
82b7dc3 
This adds a simpler new implementation of getbalance logic along with asserts
to confirm it behaves identically to the old logic. The old logic is removed in
the next commit.
@ryanofsky
[wallet] Remove unneeded legacy getbalance code
02d9f50
@ryanofsky ryanofsky force-pushed the pr/getbalance-cleanup branch from 0b9520d to 02d9f50 Compare April 26, 2017 10:42
@ryanofsky
Copy link
Contributor Author

ryanofsky commented Apr 26, 2017 •
edited
Loading

No problem, thanks for the review!

Rebased 0b9520d -> 02d9f50 (pr/getbalance-cleanup.3 -> pr/getbalance-cleanup.4). No changes except adding another std:: prefix and passing the right new argument to the CWalletDB constructor.

@laanwj laanwj merged commit 02d9f50 into bitcoin:master Apr 26, 2017
laanwj added a commit that referenced this pull request Apr 26, 2017
@laanwj
Merge #9614: [wallet] [refactor] Simplify getbalance implementation
cf57825 
02d9f50 [wallet] Remove unneeded legacy getbalance code (Russell Yanofsky)
82b7dc3 [wallet] Add GetLegacyBalance method to simplify getbalance RPC (Russell Yanofsky)

Tree-SHA512: c3b890ff1f5a3df7e886309bad94bdf5cc3c12b72983bb79c72f8655ce16edf581bff0faaade2f18c2cb723f50d516e53f87792f81c3f8143b0c4377c0d80e87
@jonasschnelli jonasschnelli mentioned this pull request Jun 13, 2018
Closed
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 24, 2019
@laanwj @PastaPastaPasta
Merge bitcoin#9614: [wallet] [refactor] Simplify getbalance implement…
de96295 
…ation

02d9f50 [wallet] Remove unneeded legacy getbalance code (Russell Yanofsky)
82b7dc3 [wallet] Add GetLegacyBalance method to simplify getbalance RPC (Russell Yanofsky)

Tree-SHA512: c3b890ff1f5a3df7e886309bad94bdf5cc3c12b72983bb79c72f8655ce16edf581bff0faaade2f18c2cb723f50d516e53f87792f81c3f8143b0c4377c0d80e87
@Fuzzbawls Fuzzbawls mentioned this pull request Jul 4, 2020
Closed
11 tasks
Fuzzbawls added a commit to Fuzzbawls/PIVX that referenced this pull request Jul 8, 2020
@Fuzzbawls
Refactor: Simplify getbalance implementation
57148f2 
backported from bitcoin#9614
@psolstice psolstice mentioned this pull request Feb 2, 2021
Merged
@nuttycom nuttycom mentioned this pull request Aug 11, 2021
Merged
zkbot added a commit to zcash/zcash that referenced this pull request Aug 12, 2021
@zkbot
Auto merge of #5271 - nuttycom:backport/13825_kill_accounts, r=str4d
5b19406 
[Backport] bitcoin/bitcoin#13825 kill accounts

This is a backport of the following commits:

- from bitcoin/bitcoin#13825
- pick bitcoin/bitcoin@c9c32e6
  - from bitcoin/bitcoin#14023
    - pick bitcoin/bitcoin@f0dc850
      - from bitcoin/bitcoin#13566
        - pick bitcoin/bitcoin@702ae1e
        - pick bitcoin/bitcoin@cf15761
        - pick bitcoin/bitcoin@0f3d6e9
        - pick bitcoin/bitcoin@7110c83
        - pick bitcoin/bitcoin@ef7bc88
        - pick bitcoin/bitcoin@4279da4
    - pick bitcoin/bitcoin@c410f41
    - from bitcoin/bitcoin#9614
      - pick bitcoin/bitcoin@02d9f50
      - pick bitcoin/bitcoin@82b7dc3
        - from bitcoin/bitcoin#8061
          - pick bitcoin/bitcoin@ecb9741
  - from bitcoin/bitcoin#6851
    - pick bitcoin/bitcoin@3e7c89196c
  - from bitcoin/bitcoin#8828
    - pick bitcoin/bitcoin@86029e7
      - from bitcoin/bitcoin#8696
        - just the first two commits, as AccountingEntry is going away
        - pick bitcoin/bitcoin@d2e678d
        - pick bitcoin/bitcoin@59adc86
    - from bitcoin/bitcoin#8028
      - pick bitcoin/bitcoin@0fd5997
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@laanwj laanwj laanwj approved these changes

+1 more reviewer

@jonasschnelli jonasschnelli jonasschnelli approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Refactoring Wallet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ryanofsky @laanwj @jonasschnelli