rpc: Ignore sendmany::minconf as dummy value #15596

MarcoFalke commented at 10:26 PM on March 13, 2019: member

Other RPCs such as sendtoaddress don't have this option at all and sendmany should by default spend from (lets say) our change.

rpc: Document that minconf is an ignored dummy value fae5f874d5

MarcoFalke added this to the milestone 0.19.0 on Mar 13, 2019

MarcoFalke added the label Wallet on Mar 13, 2019

MarcoFalke added the label RPC/REST/ZMQ on Mar 13, 2019

promag commented at 2:23 PM on March 14, 2019: member

Should minconf be CoinEligibilityFilter::conf_theirs?

MarcoFalke commented at 2:37 PM on March 14, 2019: member

That is not clear from the description in the rpc help. And I'd rather not change the behaviour of coinselection for this rpc, since that will change how it behaved previously. (See also #15595 (comment), where I tried to pass it down to coinselection)

practicalswift commented at 7:52 PM on March 14, 2019: contributor

GetLegacyBalance is unused and should be removed, no? . TBH this kind of stuff should really be identified automatically and fixed before human review takes place -- we really need to use better tooling in Travis :-)

scripted-diff: wallet: Rename pcoin to wtx

-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended -e 's/const CWalletTx ?\* ?pcoin = &/const CWalletTx\& wtx = /g' src/wallet/wallet.cpp
sed -i -e 's/\<pcoin->/wtx./g' src/wallet/wallet.cpp
sed -i -e 's/\<pcoin\>/\&wtx/g' src/wallet/wallet.cpp
-END VERIFY SCRIPT-

faa3a246e8

wallet: Remove unused GetLegacyBalance fac1a0fe54

MarcoFalke force-pushed on Mar 14, 2019

DrahtBot commented at 10:14 PM on March 14, 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:

#15729 (rpc: Ignore minconf parameter in getbalance by promag)
#13756 (wallet: "avoid_reuse" wallet flag for improved privacy by kallewoof)

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.

MarcoFalke commented at 3:30 PM on March 18, 2019: member

Removed all now unused methods

MarcoFalke commented at 7:52 PM on March 18, 2019: member

@jnewbery

in src/wallet/rpcwallet.cpp:933 in fae5f874d5 outdated

 925 | @@ -929,11 +926,6 @@ static UniValue sendmany(const JSONRPCRequest& request)
 926 |  
 927 |      EnsureWalletIsUnlocked(pwallet);
 928 |  
 929 | -    // Check funds
 930 | -    if (totalAmount > pwallet->GetLegacyBalance(ISMINE_SPENDABLE, nMinDepth)) {

ryanofsky commented at 3:56 PM on March 20, 2019:

In commit "rpc: Document that minconf is an ignored dummy value" (fae5f874d51c770322fb18cc4050b3f14697de66)

Is this commit description actually accurate? Is this just documenting existing behavior or changing the behavior?

If this commit is changing the behavior, I think there should be release notes saying min_depth is now ignored. Also, maybe this should raise an error if min_depth is passed and is set to anything other than 1.

If this commit isn't changing behavior, and min depth was ignored all along, the commit message should directly say it was already ignored, and maybe explain how setting a value didn't have any effect. Also, there could still be release notes saying that sendmany will now throw "Insufficient funds" (from CreateTransaction) instead of "Wallet has insufficient funds."

MarcoFalke commented at 4:52 PM on March 20, 2019:

Strictly speaking it is changing behavior, since it might throw a different rpc error (or none at all and succeed) now. However, the previous behavior was not well specified by the documentation string of minconf. And my attempt at making sense of it failed:

rpc: Actually use sendmany::minconf #15595

promag commented at 2:27 PM on April 2, 2019:

By not throwing an error the client may think min_depth is still used and never update his code.

Also, maybe this should raise an error if min_depth is passed and is set to anything other than 1.

I think @ryanofsky suggestion should be considered.

MarcoFalke commented at 2:42 PM on April 2, 2019:

That is just going to open a can of worms. I think, either it should throw for any value (including 1, the default), since that was ignored before as well, or never throw at all.

If it throws for all values, you might as well just remove the parameter and break the interface. Though, then that'd have to go through a -deprecatedrpc cycle. I don't think this effort is justified. Please explain why ignoring this parameter could lead to any meaningful issues

promag commented at 2:48 PM on April 2, 2019:

I think ignoring doesn't raise awareness and clients remain deluded, as we were.

promag commented at 2:52 PM on April 2, 2019:

I also understand the effort concern, but we do have a clear way to deprecate things and break backward compatibility, so why deal with this as a special case, and leave the parameter sitting there?

jnewbery commented at 3:37 PM on April 2, 2019:

I can see both sides. In this case though, I think we should just silently ignore. I don't see any downside to clients continuing to specify this and having it ignored. The help text has been updated to "ignore dummy value", so I don't think there's any chance of users being confused by this.

Enforcing that this be set to 1 would give the impression that the passed value actually does something.

promag commented at 3:53 PM on April 2, 2019:

The help text is worthless for existing software.

MarcoFalke commented at 4:15 PM on April 2, 2019:

This will be mentioned in the release notes and RPC help, if people don't read those when upgrading their bitcoind backend, I don't think there is much we can do. Making it an error to pass something other than 1 is not going them to read them either, but just modify their code to pass 1, what is the point then?

jnewbery commented at 4:22 PM on April 2, 2019:

The help text is worthless for existing software.

For existing software, the only thing throwing an error would do is make the user change the client so it sets this to 1 or omit it. The end result will be the same as if the RPC doesn't throw an error, except there would be more disruption for users during transition.

If this PR were actually deprecating useful functionality, then I'd agree with you, but it's not. This option has never done what the help text implies that it should do.

ryanofsky approved

ryanofsky commented at 4:01 PM on March 20, 2019: member

utACK fac1a0fe54287d819cd0967ad6c75bbcb49b332b. This seems like a nice simplifying change, but I think it could definitely be documented better (better commit message and release notes).

MarcoFalke added the label Needs release note on Mar 20, 2019

MarcoFalke removed the label Needs release note on Mar 20, 2019

doc: Add release notes for 15596 fabfb79673

MarcoFalke commented at 4:49 PM on March 20, 2019: member

Added a writeup as requested by @ryanofsky

ryanofsky approved

ryanofsky commented at 4:52 PM on March 20, 2019: member

utACK fabfb79673d6bf9bff5258cd709d8294e77c1764. Nice writeup! Release notes are only change since previous review.

jnewbery commented at 2:47 PM on March 21, 2019: member

Concept ACK

laanwj added this to the "Blockers" column in a project

promag commented at 12:42 AM on March 22, 2019: member

ACK behavior change, and no change in tests 😕

jnewbery commented at 3:01 PM on April 1, 2019: member

utACK fabfb79673d6bf9bff5258cd709d8294e77c1764

The fact that this parameter was not covered by any test cases is indication that it was underspecified.

Nice incidental correction of coin -> wtx.

promag changes_requested

promag commented at 2:36 PM on April 2, 2019: member

NACK, I think ignoring is bad - clients should update to use a default value or omit. Discussion in #15596 (review)

MarcoFalke commented at 4:07 PM on April 3, 2019: member

I am going to merge this tomorrow unless there are further concerns

MarcoFalke referenced this in commit daef20fb50 on Apr 4, 2019

MarcoFalke merged this on Apr 4, 2019

MarcoFalke closed this on Apr 4, 2019

laanwj removed this from the "Blockers" column in a project

in doc/release-notes.md:77 in fabfb79673

  73 | +Note: some low-level RPC changes mainly useful for testing are described in the
  74 | +Low-level Changes section below.
  75 | +
  76 | +* The `sendmany` RPC had an argument `minconf` that was not well specified and
  77 | +  would lead to RPC errors even when the wallet's coin selection would succeed.
  78 | +  The `sendtoaddress` RPC never had this check, so to normalize the behavior,

luke-jr commented at 2:29 PM on April 8, 2019:

The reasoning here is flawed, BTW. sendtoaddress was kind of soft-deprecated by sendmany.

MarcoFalke commented at 2:35 PM on April 8, 2019:

It is just one of many reasons why the arg should be ignored. Feel free to remove that reasoning, the others should still hold.

MarcoFalke deleted the branch on Apr 8, 2019

gmaxwell commented at 6:07 PM on November 24, 2019: contributor

Since some people reading the release notes are probably going to end up here confused (I did), I thought it would be useful to write my understanding here.

The min_conf argument to sendmany was functionally part of the now gone "accounts" system. ... that's also why it was only an argument to sendmany and not sendtoaddress, since sendmany had an account parameter (and as luke-noted, sendtoaddress was soft-deprecated).

The accounts system was intended to provide the backing logic for a shared user wallet (in particular, mybitcoin which went away after "losing" half its users funds). Especially after the mental model of UTXO and such started becoming understood by users, accounts functions were commonly misunderstood to be coincontrol features, but they never were: The wallet was written almost entirely with the view that the underlying output model would be hidden from the user. And for a shared-wallet it would be generally undesirable to have multiple users mean separate wallets because of the loss of fee efficiency, in any case.

The purpose of min_conf was accurately documented in the help: Use the balance according to a specified number of confirmations. This did not mean use the coins of a specific depth: remember accounts were not a coin-control feature, and the abstraction of coins was originally intended to be completely hidden from the users.

The reason an accounts-based shared wallet would want to use this is to prevent an attack where a user deposits some coins, waits for 1 conf (so their balance goes up) then withdraw that amount (which they can because their balance is high enough) which then ends up selecting some different >6 conf coins (since that is coinselection's extremely strong preference) ... then reorg the chain 1 block to claw their payment back while leaving the withdraw in place. ... leaving the user with a negative balance which they never repay. By setting min_conf to 6, a permitted payment would not result in the user's balance becoming negative if there was a 6 or fewer block reorg + doublespend.

Though I'm not sure of it, I would be surprised if this option wasn't added in direct response to actual losses.

Since the accounts system is gone, this setting doesn't have a real use anymore AFAICT. It probably would have been better if the release notes had said that, rather than suggesting some coin-controlly things which this setting never accomplished, not intended to accomplish, nor was documented to accomplish.

It's possible someone was using it to make sure they didn't overspend relative to their well confirmed coins and end up insolvent and owing people for payments in the event of a reorg beyond what they can make good for-- but I doubt it. If so, I believe there is currently no alternative functionality available for them (certainly not any of the options mentioned in the release notes).

Nor could the setting really have been re-purposed to some coin control-ish purpose, even though filtering your coins by depth is a pretty reasonable ask-- because change and third party payments really need to be treated distinctly if you do that kind of filtering because otherwise any filtering means you can't use your own change which is extremely disruptive and surprising, particularly since the interface mostly hides the fact that transacting temporarily kicks an essentially random amount of your balance to unconfirmed status. Any kind of min_conf coin control setting would probably need to work like the coin-selection does and have a separate argument for change (which would almost always be kept at the default of 0). ... maybe such a feature would even need three categories (immature coinbases) or four (change from segwit-only txn should probably be handled differently!). yuck: coin control is hard.

MarcoFalke commented at 6:44 PM on November 24, 2019: member

Thanks for providing some more background on this. I read the release notes myself this morning and was confused by my own wording. Unfortunately it is now too late to change the release notes :(

jnewbery commented at 10:08 PM on November 24, 2019: member

What does soft-deprecated mean?

MarcoFalke commented at 10:40 PM on November 24, 2019: member

What does soft-deprecated mean?

I think it means that there is an similar call that should be preferred, but the original one will never be removed.

gmaxwell commented at 10:51 PM on November 24, 2019: contributor

And, in particular, new functionality would be added to the new call... and the old call just hangs around for compatibility reasons, without changing except for bug fixes and trivial improvements.

vansergen referenced this in commit 0c35cfe238 on Mar 26, 2020

deadalnix referenced this in commit 07a8d7d8c1 on May 22, 2020

deadalnix referenced this in commit 5f2c795c25 on May 22, 2020

deadalnix referenced this in commit 45c0bb5b90 on May 22, 2020

deadalnix referenced this in commit c72b2a706b on May 22, 2020

kittywhiskers referenced this in commit 3e789f3d30 on Dec 4, 2021

kittywhiskers referenced this in commit 469d6652f5 on Dec 8, 2021

kittywhiskers referenced this in commit 5b6b0fb4a5 on Dec 12, 2021

kittywhiskers referenced this in commit 2374cce8ba on Dec 13, 2021

kittywhiskers referenced this in commit 05c4627328 on Dec 13, 2021

kittywhiskers referenced this in commit 06cc4bbe5f on Dec 13, 2021

DrahtBot locked this on Dec 16, 2021