Skip to content

wallet: GetEffectiveBalance #26365

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: GetEffectiveBalance #26365

wants to merge 3 commits into from

Conversation

willcl-ark
Copy link
Member

@willcl-ark willcl-ark commented Oct 21, 2022 •
edited
Loading

This is a rough draft of what a GetEffectiveBalance function on the wallet might look like, along with RPC call and wallet interface.
Related to #15767

Still todo and open questions:

  • RPCHelp wording is just a rough draft and needs amending
  • Should the result contain additional info such as: fee rate used, num utxos, num excluded utxos etc. (this would better satisfy Display sendable (effective) balance #15767 too)
  • Should confirm target be configurable? Currently fixed to a 3 blocks.
    • In this case the response should likely include the confirm target used to generate the effective value.
  • Should this be added to QT wallet? Interface is present. I think it would be nice but don't know much about QT.
  • Required adding a lot of headers to receive.cpp which seems sub-optimal. Perhaps the function belongs somewhere else...
  • Doesn't add a dummy recipient to the tx, so fee calculation is slightly off
  • Doesn't respect avoid_reuse
  • Doesn't respect watch_only
  • RBF just uses wallet default

@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 22, 2022 •
edited
Loading

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #26756 (wallet: Replace GetBalance() logic with AvailableCoins() by w0xlt)
  • #26699 (wallet, gui: bugfix, getAvailableBalance skips selected coins by furszy)
  • #25659 (wallet: simplify ListCoins implementation by furszy)

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.

@DrahtBot DrahtBot mentioned this pull request Oct 22, 2022
Closed
luke-jr
luke-jr reviewed Oct 27, 2022
View reviewed changes
src/wallet/rpc/coins.cpp
return RPCHelpMan{"geteffectivebalance",
"\nReturns the total effective balance.\n"
"The effective balance is what the wallet considers currently spendable at a\n"
"confirmation target of 3 blocks at current feerate.\n",
Copy link
Member

Choose a reason for hiding this comment

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

Why? This seems quite arbitrary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct, it's totally arbitrary for now but I think making it configurable woudl make it more interesting/useful.

The idea (from #15767 ) is that Bitcoin-qt (primarily) could display some estimate of the actual value that can be sent, after fees. In this case, we could either present the GUI with this "estimate" (3 confirmations seems estimate-ish to me), or there could perhaps be an option/drop-down for user to select which fee style they would like the estimate to use.

For an RPC, user could specify the fees they wanted used.

Anyway, for now I am just working out whether it makes much sense at all, and the best way to architect it (e.g. I started with it in spend.cpp as this made logical sense, but ended up introducing circular dependencies so moved into receive.cpp ...)

@murchandamus
Copy link
Contributor

Concept ACK

I think it's useful to be able to tell users how much they could actually spend. A three-block target or six-block target seems like a reasonable default, but I'd expect this RPC call to be more useful if it could also take a custom feerate.

@aureleoules
Copy link
Contributor

Concept ACK

I agree that there should be a configurable fee rate and/or configurable block target.
How about adding an RPC arg effective to getbalance instead of creating a separate RPC?

@willcl-ark willcl-ark mentioned this pull request Nov 15, 2022
Open
@DrahtBot DrahtBot mentioned this pull request Nov 29, 2022
Merged
@DrahtBot DrahtBot mentioned this pull request Dec 15, 2022
Merged
@DrahtBot DrahtBot mentioned this pull request Dec 28, 2022
Closed
@DrahtBot
Copy link
Contributor

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

@willcl-ark
Copy link
Member Author

Going to close this as there are other, cleaner and better-maintained version(s) of it being worked on now.

@willcl-ark willcl-ark closed this Mar 14, 2023
@bitcoin bitcoin locked and limited conversation to collaborators Mar 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
1 more reviewer

@luke-jr luke-jr luke-jr left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Needs rebase Wallet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@willcl-ark @DrahtBot @murchandamus @aureleoules @luke-jr