Skip to content

wallet: GetEffectiveBalance#26365

Closed
willcl-ark wants to merge 3 commits intobitcoin:masterfrom
willcl-ark:effective_balance
Closed

wallet: GetEffectiveBalance#26365
willcl-ark wants to merge 3 commits intobitcoin:masterfrom
willcl-ark:effective_balance

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 Outdated
Copy link
Member

@luke-jr luke-jr Oct 27, 2022

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

@willcl-ark willcl-ark Oct 27, 2022

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
Member

murchandamus commented Nov 1, 2022

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

aureleoules commented Nov 2, 2022

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

DrahtBot commented Jan 18, 2023

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

@willcl-ark
Copy link
Member Author

willcl-ark commented Mar 14, 2023

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