wallet: Do not treat default constructed types as None-type by maflcko · Pull Request #20410 · bitcoin/bitcoin

maflcko · 2020-11-17T18:30:43Z

Equating 0==None and ""==None is confusing, unneeded and undocumented

src/wallet/rpcwallet.cpp

jonatack · 2020-11-17T19:46:24Z

Would this change not disable using fee_rate as a positional argument?

The RPCExamples for sendtoaddress and send would need to be updated if this change is made, as the positional fee_rate usage is documented in them.

maflcko · 2020-11-17T19:50:23Z

You can pass JSON-None if you'd like to use positional args

DrahtBot · 2020-11-17T20:33:34Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

test: Implicitly sync after generate* to preempt races and intermittent test failures #20362 (test: Implicitly sync after generate* to preempt races and intermittent test failures by MarcoFalke)

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.

maflcko · 2020-11-17T21:06:12Z

Oh, it looks like bitcoin-cli can not properly pass JSON-None? Sounds like a bug

achow101 · 2020-11-17T21:12:13Z

Oh, it looks like bitcoin-cli can not properly pass JSON-None? Sounds like a bug

You can just use null. It's worked for me before.

jonatack · 2020-11-17T21:17:27Z

An example? I haven't managed to make it work via the CLI.

maflcko · 2020-11-18T06:35:19Z

It doesn't work for strings because cli will convert null to "null" (string), not Null (null-type)

maflcko · 2020-11-18T07:34:06Z

The RPCExamples for sendtoaddress and send would need to be updated

Thanks, done.

jonatack · 2020-11-18T10:36:27Z

Building and testing.

jonatack

Tested and the code and CLI examples look good. Perhaps keep the placeholder test coverage but with conf_target=None.

src/wallet/rpcwallet.cpp

test/functional/wallet_basic.py

test/functional/wallet_send.py

laanwj · 2020-11-19T08:44:28Z

It doesn't work for strings because cli will convert null to "null" (string), not Null (null-type)

Yes, in that case you cannot pass any other kind of JSON type except strings. It's kind of annoying, but there is no non-ambigious way of doing so because every argument is a valid string. Special-casing the string "null" in bitcoin-cli would be awful. At least named arguments can achieve this.

jonatack · 2020-11-19T11:22:37Z

Tested ACK fa7df9be0d1034d2bbbde19a45b1e706368b4c1b

jonatack · 2020-11-19T11:31:40Z

A separate issue I found while testing this: in the as-yet unreleased send RPC, it seems impossible to use "options" as a positional arg, as there is no no-op value for the "fee_rate" arg. So the third example should either be removed, or updated if we fix this via either allowing "fee_rate" to have a no-op value or, more likely, by moving "fee_rate" to after "options" in the args order. I can do this in a fee_rate follow-up PR if not done here.

maflcko · 2020-11-19T12:16:53Z

Do you have steps to reproduce? This works fine locally:

$ ./src/bitcoin-cli  send '{"bcrt1q0pqmg4cvjf80x7l7xsrhp65fchg2fw9zk39m74":0.1}'  null "unset" null '{"fee_rate":1.234}'
{
  "txid": "3bda2cb650a3937b676782214837fea6313158a156113e059bd9cad118717ecb",
  "complete": true
}

jonatack · 2020-11-19T12:34:13Z

Oh good, thanks, sorry for noise. One of my "null"s was spelled "nul". I think the third week of full lockdown and lack of my usual balance from outdoor sport are getting to me...

$ ./src/bitcoin-cli -signet -rpcwallet="" send '{"tb1qn84cdagw67kpqaz4ugmw76tlhqgdp2jk9zz94j": 0.05}' null "unset" null '{"fee_rate": 1}'
{
  "txid": "25d0924beac73778566909c0658698a4602d2f9444895542f85726e7b5f22465",
  "complete": true
}

jonatack · 2020-11-19T12:37:49Z

This send example was incorrect and needs to be updated:

Send 0.2 BTC with a fee rate of 1 sat/vB using the options argument
> bitcoin-cli send '{"bc1q09vm5lfy0j5reeulh4x5752q25uqqvz34hufdl": 0.2}' '{"fee_rate": 1}'

maflcko · 2020-11-19T12:54:52Z

Good catch. Didn't realize the example was wrong, too. Now fixed.

jonatack · 2020-11-19T14:25:49Z

ACK fa69c2c

Sjors

tACK fa69c2c modulo unset

I don't think treating ""==None is particularly ambiguous, but null is not too bad.

Sjors · 2020-11-23T08:36:11Z

src/wallet/rpcwallet.cpp

- *                                      if a fee_rate is present, "" is allowed here as a no-op positional placeholder
 * @param[in]     fee_rate          UniValue real; fee rate in sat/vB;
- *                                      if a fee_rate is present, both conf_target and estimate_mode must either be null, or no-op
+ *                                      if present, both conf_target and estimate_mode must either be null, or "unset"


estimate_mode can't be null for send and sendtoaddress. It will throw Cannot specify both estimate_mode and fee_rate. It does if you add it to client.cpp, but then unset and "unset" throw error: Error parsing JSON: unset.

Yes, that comment is a bit ambiguous; IIRC null is only for conf_target and unset only for estimate_mode.

estimate_mode can't be null for send and sendtoaddress. It will throw Cannot specify both estimate_mode and fee_rate.

This comment is in the server code, where it can be null, not in the client code. You can trivially check with this diff, so I think the comment is correct:

diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py index ac4a6e4948..17c013d26a 100755 --- a/test/functional/wallet_basic.py +++ b/test/functional/wallet_basic.py @@ -235,7 +235,7 @@ class WalletTest(BitcoinTestFramework): fee_rate_btc_kvb = fee_rate_sat_vb * 1e3 / 1e8 explicit_fee_rate_btc_kvb = Decimal(fee_rate_btc_kvb) / 1000 - txid = self.nodes[2].sendmany(amounts={address: 10}, fee_rate=fee_rate_sat_vb) + txid = self.nodes[2].sendmany(amounts={address: 10}, fee_rate=fee_rate_sat_vb, estimate_mode=None) self.nodes[2].generate(1) self.sync_all(self.nodes[0:3]) balance = self.nodes[2].getbalance() @@ -406,7 +406,7 @@ class WalletTest(BitcoinTestFramework): fee_rate_sat_vb = 2 fee_rate_btc_kvb = fee_rate_sat_vb * 1e3 / 1e8 - txid = self.nodes[2].sendtoaddress(address=address, amount=amount, fee_rate=fee_rate_sat_vb) + txid = self.nodes[2].sendtoaddress(address=address, amount=amount, fee_rate=fee_rate_sat_vb, estimate_mode=None) tx_size = self.get_vsize(self.nodes[2].gettransaction(txid)['hex']) self.nodes[0].generate(1) self.sync_all(self.nodes[0:3])

maflcko · 2020-11-23T13:17:22Z

I don't think treating ""==None is particularly ambiguous

Fair enough, but it wasn't documented in the RPCArg help. I think keeping the already existing "unset" is sufficient and adding more alternative strings to do the same thing isn't going to help.

achow101 · 2020-11-24T18:42:59Z

ACK fa69c2c

maflcko · 2020-11-25T07:09:36Z

Backported in #20485

…es as None-type fa69c2c wallet: Do not treat default constructed types as None-type (MarcoFalke) fac4e13 refactor: Change pointer to reference because it can not be null (MarcoFalke) Pull request description: Github-Pull: #20410 Rebased-From: fac4e13 Github-Pull: #20410 Rebased-From: fa69c2c Top commit has no ACKs. Tree-SHA512: 05c3fe29677710b57dcc482fd529b0ab79475519f60f9cfde19f956c4e2212d09b042af458ec4f1272c581360ce841b735dca4df144e0798b3ccf16547de9cd0

…s None-…

maflcko added Wallet RPC/REST/ZMQ Needs backport (0.21) labels Nov 17, 2020

jonatack reviewed Nov 17, 2020

View reviewed changes

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved

maflcko mentioned this pull request Nov 17, 2020

wallet: introduce fee_rate sat/vB param/option #20305

Merged

maflcko force-pushed the 2011-rpcWalletNoneType branch from fa0bce7 to fa4120f Compare November 17, 2020 19:06

DrahtBot mentioned this pull request Nov 17, 2020

test: Implicitly sync after generate* to preempt races and intermittent test failures #20362

Closed

refactor: Change pointer to reference because it can not be null

fac4e13

maflcko force-pushed the 2011-rpcWalletNoneType branch from fa4120f to fa31347 Compare November 18, 2020 07:34

maflcko added this to the 0.21.0 milestone Nov 18, 2020

jonatack reviewed Nov 18, 2020

View reviewed changes

maflcko force-pushed the 2011-rpcWalletNoneType branch from fa31347 to fa7df9b Compare November 19, 2020 10:37

wallet: Do not treat default constructed types as None-type

fa69c2c

maflcko force-pushed the 2011-rpcWalletNoneType branch from fa7df9b to fa69c2c Compare November 19, 2020 12:53

Sjors reviewed Nov 23, 2020

View reviewed changes

maflcko merged commit ca4a784 into bitcoin:master Nov 25, 2020

maflcko mentioned this pull request Nov 25, 2020

[backport] wallet: Do not treat default constructed types as None-type #20485

Merged

maflcko removed the Needs backport (0.21) label Nov 25, 2020

maflcko deleted the 2011-rpcWalletNoneType branch November 25, 2020 10:28

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 25, 2020

Merge bitcoin#20410: wallet: Do not treat default constructed types a…

ec85366

…s None-…

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Conversation

maflcko commented Nov 17, 2020

Uh oh!

Uh oh!

jonatack commented Nov 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Nov 17, 2020

Uh oh!

DrahtBot commented Nov 17, 2020

Conflicts

Uh oh!

maflcko commented Nov 17, 2020

Uh oh!

achow101 commented Nov 17, 2020

Uh oh!

jonatack commented Nov 17, 2020

Uh oh!

maflcko commented Nov 18, 2020

Uh oh!

maflcko commented Nov 18, 2020

Uh oh!

jonatack commented Nov 18, 2020

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

laanwj commented Nov 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Nov 19, 2020

Uh oh!

jonatack commented Nov 19, 2020

Uh oh!

maflcko commented Nov 19, 2020

Uh oh!

jonatack commented Nov 19, 2020

Uh oh!

jonatack commented Nov 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Nov 19, 2020

Uh oh!

jonatack commented Nov 19, 2020

Uh oh!

Sjors left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors Nov 23, 2020

Choose a reason for hiding this comment

Uh oh!

jonatack Nov 23, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko Nov 23, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko commented Nov 23, 2020

Uh oh!

achow101 commented Nov 24, 2020

Uh oh!

maflcko commented Nov 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jonatack commented Nov 17, 2020 •

edited

Loading

jonatack left a comment •

edited

Loading

laanwj commented Nov 19, 2020 •

edited

Loading

jonatack commented Nov 19, 2020 •

edited

Loading

Sjors left a comment •

edited

Loading