rpc: Raise error in getbalance if minconf is not zero

promag commented at 3:31 pm on April 3, 2019: member

Closes #15702.

promag commented at 3:31 pm on April 3, 2019: member

Similar to #15596.

promag commented at 3:45 pm on April 3, 2019: member

@MarcoFalke is this what you mean in #15702?

in src/wallet/wallet.cpp:2166 in 7b126469de outdated

2162@@ -2163,7 +2163,7 @@ CAmount CWallet::GetBalance(const isminefilter& filter, const int min_depth) con
2163         for (const auto& entry : mapWallet)
2164         {
2165             const CWalletTx* pcoin = &entry.second;
2166-            if (pcoin->IsTrusted(*locked_chain) && pcoin->GetDepthInMainChain(*locked_chain) >= min_depth) {
2167+            if (pcoin->IsTrusted(*locked_chain)) {

MarcoFalke commented at 4:08 pm on April 3, 2019:

This shouldn’t count conflicts

promag commented at 7:06 am on April 4, 2019:

IsTrusted already checks GetDepthInMainChain > 0, isn’t it enough?

DrahtBot commented at 5:07 pm on April 3, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18592 (rpc: replace raw pointers with shared_ptrs by brakmic)
#18502 (doc: Update docs for getbalance (default minconf should be 0) by uzyn)
#18202 (refactor: consolidate sendmany and sendtoaddress code by Sjors)

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.

fanquake added the label RPC/REST/ZMQ on Apr 3, 2019

DrahtBot added the label Needs rebase on Apr 4, 2019

promag marked this as ready for review on Apr 7, 2019

luke-jr commented at 11:38 am on April 8, 2019: member

This should throw an exception, not simply be ignored…

promag force-pushed on Apr 8, 2019

promag commented at 12:45 pm on April 8, 2019: member

@luke-jr I’m doing the same as #15596. Why throw an error?

luke-jr commented at 1:03 pm on April 8, 2019: member

Because if I specify minconf=10, I expect the value with received transactions less than 10 blocks deep to be excluded. Returning a wrong answer is not a reasonable API change.

#15596 should probably be throwing an error too.

fanquake removed the label Needs rebase on Apr 8, 2019

promag commented at 1:39 pm on April 8, 2019: member

@luke-jr how about renaming to min_conf_theirs=10? That wouldn’t cause confusion and allows to take into account change (or send to self) outputs with 1 confirmation.

MarcoFalke commented at 2:26 pm on April 8, 2019: member

@luke-jr I’m doing the same as #15596. Why throw an error?

#15596 has always ignored the minconf parameter, getbalance did not. So I think an error is fine here.

DrahtBot added the label Needs rebase on Apr 9, 2019

laanwj commented at 3:20 pm on April 10, 2019: member

This should throw an exception, not simply be ignored…

Yes, error is definitely better. Programs ignoring arguments can be incredibly frustrating.

promag commented at 3:40 pm on April 10, 2019: member

Then when should it throw an error? minconf > 0?

luke-jr commented at 6:18 am on April 18, 2019: member

minconf != 0

promag force-pushed on Apr 21, 2019

promag force-pushed on Apr 22, 2019

promag renamed this:
~~rpc: Ignore minconf parameter in getbalance~~
rpc: Raise error in getbalance if minconf is not zero
on Apr 22, 2019

promag commented at 0:13 am on April 22, 2019: member

Now raising error.

DrahtBot removed the label Needs rebase on Apr 22, 2019

DrahtBot added the label Needs rebase on May 1, 2019

promag force-pushed on May 5, 2019

DrahtBot removed the label Needs rebase on May 6, 2019

promag commented at 11:53 am on May 25, 2019: member

This is ready?

DrahtBot added the label Needs rebase on Jun 18, 2019

jnewbery commented at 1:48 pm on July 2, 2019: member

Is this still needed now that #15596 is merged?

MarcoFalke commented at 1:55 pm on July 2, 2019: member

On pull request is removing min conf, the other is adding it. Would be nice if there was a uniform general direction.

rpc: Add min_conf option to fund transaction calls #14641
rpc: Raise error in getbalance if minconf is not zero #15729

promag commented at 2:12 pm on July 2, 2019: member

@jnewbery that one is unrelated? @MarcoFalke is #15702 still a concern?

in src/wallet/wallet.cpp:1999 in f907ed27d3 outdated

2145@@ -2146,7 +2146,7 @@ CWallet::Balance CWallet::GetBalance(const int min_depth) const
2146             const int tx_depth{wtx.GetDepthInMainChain(*locked_chain)};
2147             const CAmount tx_credit_mine{wtx.GetAvailableCredit(*locked_chain, /* fUseCache */ true, ISMINE_SPENDABLE)};
2148             const CAmount tx_credit_watchonly{wtx.GetAvailableCredit(*locked_chain, /* fUseCache */ true, ISMINE_WATCH_ONLY)};
2149-            if (is_trusted && tx_depth >= min_depth) {
2150+            if (is_trusted && tx_depth >= 0) {

jnewbery commented at 3:06 pm on July 2, 2019:

nit: strictly, you can remove the tx_depth check here, since is_trusted() will do the tx_depth check for you:

https://github.com/bitcoin/bitcoin/blob/2f717fb5cdfc312784f9c1539fc41cdfcfbde452/src/wallet/wallet.cpp#L2168

Regardless of whether you make that change, I think a comment here would be helpful. Something like “Only count coins which are confirmed or are unconfirmed and trusted”.

promag commented at 3:18 pm on July 2, 2019:

~~I’ll take your suggestion.~~

I’ve reverted since the condition is more strong than a comment and IMO self explanatory.

in src/wallet/rpcwallet.cpp:762 in f907ed27d3 outdated

757@@ -758,17 +758,17 @@ static UniValue getbalance(const JSONRPCRequest& request)
758         throw JSONRPCError(RPC_METHOD_DEPRECATED, "dummy first argument must be excluded or set to \"*\".");
759     }
760 
761-    int min_depth = 0;
762     if (!request.params[1].isNull()) {
763-        min_depth = request.params[1].get_int();
764+        int min_depth = request.params[1].get_int();

jnewbery commented at 3:07 pm on July 2, 2019:

nit: seems strange to define a variable in this scope which is just going to get used once. How about:

0if (!request.params[1].isNull() && request.params[1].get_int() != 0) {
1    throw ...

promag commented at 3:17 pm on July 2, 2019:

Sure.

jnewbery commented at 3:11 pm on July 2, 2019: member

that one is unrelated?

Sorry, you’re correct. That PR isn’t relevant here.

I think the current logic for minconf is wrong, so I’m a cautious concept ACK, although I think we’d want to check that no-one is relying on the current, broken behaviour.

I’ve added a couple of nits inline.

promag commented at 3:18 pm on July 2, 2019: member

although I think we’d want to check that no-one is relying on the current, broken behaviour.

How?

jnewbery commented at 3:20 pm on July 2, 2019: member

How?

Ask in #bitcoin or #bitcoin-core-dev? Deprecate this first before removing in the next release?

luke-jr commented at 3:45 pm on July 2, 2019: member

It’s only been broken for a few releases. Much more likely that someone is relying on the correct behaviour, and hasn’t noticed it got broken.

jnewbery commented at 6:04 pm on July 2, 2019: member

It’s only been broken for a few releases

No - it’s always been inconsistent/broken

luke-jr commented at 6:24 pm on July 2, 2019: member

No, it hasn’t. It worked fine until v0.17.0

jnewbery commented at 7:26 pm on July 2, 2019: member

Please see #14602#pullrequestreview-179051754 for some of the historic problems with getbalance minconf. No-one has been able to specify what the behaviour even should be, so it’s not correct to say that this worked fine - there wasn’t even a specified of documented behaviour for this.

luke-jr commented at 7:32 pm on July 2, 2019: member

The behaviour it has historically had (and IMO fairly reasonable too) is to add all N-block confirmed incoming, and subtract all (ignoring confirmation) outgoing.

promag force-pushed on Jul 2, 2019

DrahtBot removed the label Needs rebase on Jul 3, 2019

MarcoFalke commented at 5:04 pm on July 8, 2019: member

The behaviour it has historically had (and IMO fairly reasonable too) is to add all N-block confirmed incoming, and subtract all (ignoring confirmation) outgoing.

Whatever the behaviour is, it should be clearly documented. Especially since the behaviour is not obvious from just the one line argument description.

luke-jr commented at 5:37 pm on July 8, 2019: member

Core no longer has code to correctly calculate the balance in this case anymore, so removal via error (ie, this PR) seems like the correct solution.

MarcoFalke commented at 5:59 pm on July 8, 2019: member

@promag This does not compile

promag commented at 6:02 pm on July 8, 2019: member

I know, I’m waiting :popcorn:

DrahtBot added the label Needs rebase on Jul 9, 2019

promag force-pushed on Aug 2, 2019

promag commented at 5:22 pm on August 2, 2019: member

Rebased, updated and added release notes.

MarcoFalke commented at 5:37 pm on August 2, 2019: member

I believe my comment #15729 (comment) has not been addressed

MarcoFalke commented at 5:47 pm on August 2, 2019: member

On a second though, I think this is fine to remove. The behavior seems confusing and I can’t really come up with a use case.

ACK 2a774cf7542d2e321823a1c79c66017fca4993af

DrahtBot removed the label Needs rebase on Aug 2, 2019

DrahtBot added the label Needs rebase on Aug 16, 2019

promag force-pushed on Nov 3, 2019

DrahtBot removed the label Needs rebase on Nov 3, 2019

DrahtBot added the label Needs rebase on Nov 5, 2019

qa: Update usages of getbalance to use minconf=0 2bc97a46eb

rpc: Raise error in getbalance if minconf is not zero a6f3e2f3af

wallet: Remove min_depth argument from CWallet::GetBalance 841f01d653

doc: Add release notes for minconf change in getbalance 019872a5ee

promag force-pushed on Nov 17, 2019

DrahtBot removed the label Needs rebase on Nov 17, 2019

promag commented at 10:21 pm on November 24, 2019: member

Also wallet label?

Ping @meshcollider.

fanquake added the label Wallet on Nov 24, 2019

meshcollider commented at 9:35 am on November 25, 2019: contributor

With the many minconf PRs and arguments we’ve had, I think we should at least briefly discuss this in the meeting to make sure people that haven’t looked here are ok with the change

ryanofsky commented at 2:07 pm on November 25, 2019: member

Conditional NACK until this has good release notes.

I don’t like this change, but without a better alternative I think it might be an improvement over the status quo. The release notes here are bad though. If you’re going to remove a feature (“tell me my balance ignoring incoming payments with less than min_conf confirmations”) that once worked well enough and was more recently broken you should say why you are removing it and what possible alternatives are. I can try to write up some better release notes for this change today, and if anybody else has more information about past or present problems with this feature, it would be useful to post here. (Last discussions I remember having about this were about theoretical problems in potential future implementations of this feature.)

promag commented at 3:18 pm on November 25, 2019: member

until this has good release notes.

Agree, these are poor, don’t explain the motivation.

I don’t like this change, but without a better alternative I think it might be an improvement over the status quo.

I was under the impression there was no intention in fixing the feature. If that’s not the case then this should be closed.

Last discussions I remember having about this were about theoretical problems in potential future implementations of this feature

Yes, there’s #14602 and also related discussion in #15596.

in src/wallet/rpcwallet.cpp:735 in 019872a5ee

731@@ -732,7 +732,7 @@ static UniValue getbalance(const JSONRPCRequest& request)
732                 "thus affected by options which limit spendability such as -spendzeroconfchange.\n",
733                 {
734                     {"dummy", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "Remains for backward compatibility. Must be excluded or set to \"*\"."},
735-                    {"minconf", RPCArg::Type::NUM, /* default */ "0", "Only include transactions confirmed at least this many times."},
736+                    {"minconf", RPCArg::Type::NUM, /* default */ "0", "Must be zero"},

Sjors commented at 12:52 pm on February 25, 2020:

It’s very confusing for this to be 0, because it won’t actually show unconfirmed incoming transactions, only unconfirmed change. It’s not even really 0 by default, but 1, see a few lines below. Since we can’t drop it and can’t rename it to dummy2, let’s add a comment: This is ignored and only remains for backward compatibility. Must be excluded or set to zero.

DrahtBot added the label Needs rebase on Apr 13, 2020

DrahtBot commented at 1:19 pm on April 13, 2020: member

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

DrahtBot commented at 11:22 am on December 15, 2021: member

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

DrahtBot commented at 1:07 pm on March 21, 2022: member

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

MarcoFalke added the label Up for grabs on Mar 21, 2022

MarcoFalke commented at 1:48 pm on March 21, 2022: member

Marked “up for grabs”

fanquake closed this on Apr 26, 2022

promag deleted the branch on Apr 26, 2022

promag restored the branch on Apr 26, 2022

promag deleted the branch on Apr 26, 2022

promag restored the branch on Apr 26, 2022

promag deleted the branch on Apr 26, 2022

DrahtBot locked this on Apr 26, 2023

rpc: Raise error in getbalance if minconf is not zero #15729

Conflicts