Skip to content
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.

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

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

Merged
merged 4 commits into from Apr 4, 2019

Conversation

Projects
None yet
7 participants
@MarcoFalke
Copy link
Member

MarcoFalke commented Mar 13, 2019

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

@promag

This comment has been minimized.

Copy link
Member

promag commented Mar 14, 2019

Should minconf be CoinEligibilityFilter::conf_theirs?

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

MarcoFalke commented Mar 14, 2019

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

This comment has been minimized.

Copy link
Member

practicalswift commented Mar 14, 2019

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 :-)

MarcoFalke added some commits Mar 10, 2019

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-

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:1903-rpcWalletDummySendmany_2 branch from ad4a5e0 to fac1a0f Mar 14, 2019

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Mar 14, 2019

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

This comment has been minimized.

Copy link
Member Author

MarcoFalke commented Mar 18, 2019

Removed all now unused methods

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

MarcoFalke commented Mar 18, 2019

@ryanofsky
Copy link
Contributor

ryanofsky left a comment

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

@@ -929,11 +926,6 @@ static UniValue sendmany(const JSONRPCRequest& request)

EnsureWalletIsUnlocked(pwallet);

// Check funds
if (totalAmount > pwallet->GetLegacyBalance(ISMINE_SPENDABLE, nMinDepth)) {

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 20, 2019

Contributor

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

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."

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Mar 20, 2019

Author Member

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

This comment has been minimized.

Copy link
@promag

promag Apr 2, 2019

Member

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.

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Apr 2, 2019

Author Member

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

This comment has been minimized.

Copy link
@promag

promag Apr 2, 2019

Member

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

This comment has been minimized.

Copy link
@promag

promag Apr 2, 2019

Member

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?

This comment has been minimized.

Copy link
@jnewbery

jnewbery Apr 2, 2019

Member

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.

This comment has been minimized.

Copy link
@promag

promag Apr 2, 2019

Member

The help text is worthless for existing software.

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Apr 2, 2019

Author Member

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?

This comment has been minimized.

Copy link
@jnewbery

jnewbery Apr 2, 2019

Member

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.

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

MarcoFalke commented Mar 20, 2019

Added a writeup as requested by @ryanofsky

@ryanofsky
Copy link
Contributor

ryanofsky left a comment

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

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Mar 21, 2019

Concept ACK

@laanwj laanwj added this to Blockers in High-priority for review Mar 21, 2019

@promag

This comment has been minimized.

Copy link
Member

promag commented Mar 22, 2019

ACK behavior change, and no change in tests 😕

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Apr 1, 2019

utACK fabfb79

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

promag left a comment

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

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

MarcoFalke commented Apr 3, 2019

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

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Apr 4, 2019

Merge bitcoin#15596: rpc: Ignore sendmany::minconf as dummy value
fabfb79 doc: Add release notes for 15596 (MarcoFalke)
fac1a0f wallet: Remove unused GetLegacyBalance (MarcoFalke)
faa3a24 scripted-diff: wallet: Rename pcoin to wtx (MarcoFalke)
fae5f87 rpc: Document that minconf is an ignored dummy value (MarcoFalke)

Pull request description:

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

ACKs for commit fabfb7:
  jnewbery:
    utACK fabfb79
  ryanofsky:
    utACK fabfb79. Nice writeup! Release notes are only change since previous review.

Tree-SHA512: 2526ead2330be7c2beb78b96bc5e55440566c4a3a809bbbd66f5c9fc517f6890affa5d14005dc102644d49679a374510f9507255e870cf88aaa63e429beef658

@MarcoFalke MarcoFalke merged commit fabfb79 into bitcoin:master Apr 4, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@laanwj laanwj removed this from Blockers in High-priority for review Apr 4, 2019


* The `sendmany` RPC had an argument `minconf` that was not well specified and
would lead to RPC errors even when the wallet's coin selection would succeed.
The `sendtoaddress` RPC never had this check, so to normalize the behavior,

This comment has been minimized.

Copy link
@luke-jr

luke-jr Apr 8, 2019

Member

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

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Apr 8, 2019

Author Member

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 MarcoFalke deleted the MarcoFalke:1903-rpcWalletDummySendmany_2 branch Apr 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.