Skip to content

wallet: Replace GetBalance() logic with AvailableCoins() #26756

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
Closed
wants to merge 3 commits into from
Closed

wallet: Replace GetBalance() logic with AvailableCoins() #26756

wants to merge 3 commits into from

Conversation

w0xlt
Copy link
Contributor

@w0xlt w0xlt commented Dec 27, 2022 •
edited
Loading

This PR proposes a solution for #26500 by changing the AvailableCoins() function to calculate values by status (TRUSTED, UNTRUSTED_PENDING and IMMATURE) and ownership (MINE, WATCH_ONLY), eliminating the GetBalance() logic.

The downside is that the cache is no longer used. Not sure about the performance implication, but if the approach is OK, caching can also be used with this solution.

This approach also fixes the bug mentioned at the end of the wallet_abandonconflict.py file.

@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 27, 2022 •
edited
Loading

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

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #26951 (wallet: improve rescan performance 640% by pstratem)
  • #26573 (Wallet: don't underestimate the fees when spending a Taproot output by darosior)
  • #26567 (Wallet: estimate the size of signed inputs using descriptors by darosior)
  • #26152 (Bump unconfirmed ancestor transactions to target feerate by Xekyo)
  • #25979 ([WIP] wallet: standardize change output detection process by furszy)
  • #25620 (wallet: Introduce AddressBookManager by furszy)
  • #24897 ([Draft / POC] Silent Payments by w0xlt)
  • #24149 (Signing support for Miniscript Descriptors by darosior)

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.

@w0xlt w0xlt force-pushed the balance_coin_selection branch 2 times, most recently from ca8dbf6 to 7215a05 Compare December 27, 2022 06:00
This was referenced Dec 28, 2022
Closed
Closed
Merged
Merged
Closed
Merged
w0xlt added 3 commits January 4, 2023 23:26
@w0xlt
wallet: Separate the AvailableCoins() function into a new file
0cc7da8 
This allows `AvailableCoins()` to be used in multiple files
without creating a circular dependency
@w0xlt
wallet: Replace GetBalance() logic with modified AvailableCoins()
e57dfaa 
Add to the `AvailableCoins()` function the ability to separate
values by status and ownership of each coin.
@w0xlt
wallet: Remove unused CachedTxGetAvailableCredit() function
3a2bc11
@w0xlt w0xlt force-pushed the balance_coin_selection branch from 7215a05 to 3a2bc11 Compare January 5, 2023 03:02
@w0xlt
Copy link
Contributor Author

w0xlt commented Jan 5, 2023

Rebased.

This was referenced Jan 7, 2023
Closed
Closed
@DrahtBot DrahtBot mentioned this pull request Jan 23, 2023
Closed
This was referenced Feb 1, 2023
Merged
Closed
Merged
@DrahtBot
Copy link
Contributor

🐙 This pull request conflicts with the target branch and needs rebase.

@achow101 achow101 requested review from achow101 and josibake April 25, 2023 15:48
@achow101
Copy link
Member

achow101 commented May 1, 2023

I think this will end up making GetBalance really slow. We should really be tracking our UTXOs and computing the balance (and available coins) from that instead of iterating the entire wallet to figure out the UTXOs. #27286 implements the first steps to doing that, and I think we should prefer moving in that direction.

@josibake
Copy link
Member

josibake commented May 1, 2023

I'd also prefer we stopped using AvailableCoins as a general wallet traversal tool in favor of something like #27286

A lot of bugs/strangeness in the wallet seems to come from the fact that we always need to iterate over a bunch of transactions to learn the state of our wallet. If we did track wallet state in one place, we could have well-scoped individual functions for specific tasks.

@achow101
Copy link
Member

achow101 commented May 3, 2023

Closing due to lack of interest in this approach.

@achow101 achow101 closed this May 3, 2023
@bitcoin bitcoin locked and limited conversation to collaborators May 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@achow101 achow101 Awaiting requested review from achow101

@josibake josibake Awaiting requested review from josibake

Assignees
No one assigned
Labels
Needs rebase Wallet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@w0xlt @DrahtBot @achow101 @josibake