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

MarcoFalke commented at 6:30 PM on November 17, 2020: member

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

MarcoFalke added the label Wallet on Nov 17, 2020

MarcoFalke added the label RPC/REST/ZMQ on Nov 17, 2020

MarcoFalke added the label Needs backport (0.21) on Nov 17, 2020

in src/wallet/rpcwallet.cpp:213 in fa0bce7b01 outdated

 212 | @@ -213,10 +213,10 @@ static std::string LabelFromValue(const UniValue& value)
 213 |  static void SetFeeEstimateMode(const CWallet* pwallet, CCoinControl& cc, const UniValue& conf_target, const UniValue& estimate_mode, const UniValue& fee_rate, bool override_min_fee)

jonatack commented at 6:48 PM on November 17, 2020:

Would need to update the SetFeeEstimateMode Doxygen documentation, e.g.:

- * [@param](/bitcoin-bitcoin/contributor/param/)[in]     conf_target       UniValue integer; confirmation target in blocks, values between 1 and 1008 are valid per policy/fees.h;
- *                                      if a fee_rate is present, 0 is allowed here as a no-op positional placeholder
- * [@param](/bitcoin-bitcoin/contributor/param/)[in]     estimate_mode     UniValue string; fee estimation mode, valid values are "unset", "economical" or "conservative";
- *                                      if a fee_rate is present, "" is allowed here as a no-op positional placeholder
- * [@param](/bitcoin-bitcoin/contributor/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
+ * [@param](/bitcoin-bitcoin/contributor/param/)[in]     conf_target       UniValue integer; confirmation target in blocks, values between 1 and 1008 are valid per policy/fees.h
+ * [@param](/bitcoin-bitcoin/contributor/param/)[in]     estimate_mode     UniValue string; fee estimation mode, valid values are "unset", "economical" or "conservative"
+ * [@param](/bitcoin-bitcoin/contributor/param/)[in]     fee_rate          UniValue real; fee rate in sat/vB; if present, both conf_target and estimate_mode must be null
  * [@param](/bitcoin-bitcoin/contributor/param/)[in]     override_min_fee  bool; whether to set fOverrideFeeRate to true to disable minimum fee rate checks and instead
- *                                      verify only that fee_rate is greater than 0
+ *                                  verify only that fee_rate is greater than 0
  * [@throws](/bitcoin-bitcoin/contributor/throws/) a JSONRPCError if conf_target, estimate_mode, or fee_rate contain invalid values or are in conflict
  */
 static void SetFeeEstimateMode(const CWallet* pwallet, CCoinControl& cc, const UniValue& conf_target, const UniValue& estimate_mode, const UniValue& fee_rate, bool override_min_fee)

MarcoFalke commented at 7:05 PM on November 17, 2020:

Thanks, done

MarcoFalke force-pushed on Nov 17, 2020

jonatack commented at 7:46 PM on November 17, 2020: member

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.

MarcoFalke commented at 7:50 PM on November 17, 2020: member

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

DrahtBot commented at 8:33 PM on November 17, 2020: 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:

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

MarcoFalke commented at 9:06 PM on November 17, 2020: member

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

achow101 commented at 9:12 PM on November 17, 2020: member

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 commented at 9:17 PM on November 17, 2020: member

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

MarcoFalke commented at 6:35 AM on November 18, 2020: member

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

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

MarcoFalke commented at 7:34 AM on November 18, 2020: member

The RPCExamples for sendtoaddress and send would need to be updated

Thanks, done.

MarcoFalke force-pushed on Nov 18, 2020

jonatack commented at 10:36 AM on November 18, 2020: member

Building and testing.

MarcoFalke added this to the milestone 0.21.0 on Nov 18, 2020

in src/wallet/rpcwallet.cpp:205 in fa31347cb1 outdated

 202 | + * @param[in]     wallet            Wallet reference
 203 |   * @param[in,out] cc                Coin control to be updated
 204 |   * @param[in]     conf_target       UniValue integer; confirmation target in blocks, values between 1 and 1008 are valid per policy/fees.h;
 205 | - *                                      if a fee_rate is present, 0 is allowed here as a no-op positional placeholder
 206 |   * @param[in]     estimate_mode     UniValue string; fee estimation mode, valid values are "unset", "economical" or "conservative";
 207 |   *                                      if a fee_rate is present, "" is allowed here as a no-op positional placeholder

jonatack commented at 10:52 AM on November 18, 2020:

Maybe we should also allow an estimate mode of "unset" with a fee rate?

I wonder if the helps should state that "" is an allowed no-op value in addition to unset, economical and conservative.

MarcoFalke commented at 10:38 AM on November 19, 2020:

Thanks, done.

in test/functional/wallet_basic.py:238 in fa31347cb1 outdated

 234 | @@ -235,8 +235,7 @@ def run_test(self):
 235 |          fee_rate_btc_kvb = fee_rate_sat_vb * 1e3 / 1e8
 236 |          explicit_fee_rate_btc_kvb = Decimal(fee_rate_btc_kvb) / 1000
 237 |  
 238 | -        # Passing conf_target 0, estimate_mode "" as placeholder arguments should allow fee_rate to apply.
 239 | -        txid = self.nodes[2].sendmany(amounts={address: 10}, conf_target=0, estimate_mode="", fee_rate=fee_rate_sat_vb)
 240 | +        txid = self.nodes[2].sendmany(amounts={address: 10}, fee_rate=fee_rate_sat_vb)

jonatack commented at 10:57 AM on November 18, 2020:

re-enable previous coverage?

# Passing conf_target null, estimate_mode "" as placeholder arguments should allow fee_rate to apply.
txid = self.nodes[2].sendmany(amounts={address: 10}, conf_target=None, estimate_mode="", fee_rate=fee_rate_sat_vb)

MarcoFalke commented at 10:39 AM on November 19, 2020:

Added one test for estimate_mode="unset"

in test/functional/wallet_basic.py:409 in fa31347cb1 outdated

 405 | @@ -407,8 +406,7 @@ def run_test(self):
 406 |              fee_rate_sat_vb = 2
 407 |              fee_rate_btc_kvb = fee_rate_sat_vb * 1e3 / 1e8
 408 |  
 409 | -            # Passing conf_target 0, estimate_mode "" as placeholder arguments should allow fee_rate to apply.
 410 | -            txid = self.nodes[2].sendtoaddress(address=address, amount=amount, conf_target=0, estimate_mode="", fee_rate=fee_rate_sat_vb)
 411 | +            txid = self.nodes[2].sendtoaddress(address=address, amount=amount, fee_rate=fee_rate_sat_vb)

jonatack commented at 10:59 AM on November 18, 2020:

# Passing conf_target null, estimate_mode "" as placeholder arguments should allow fee_rate to apply.
txid = self.nodes[2].sendtoaddress(address=address, amount=amount, conf_target=None, estimate_mode="", fee_rate=fee_rate_sat_vb)

MarcoFalke commented at 10:39 AM on November 19, 2020:

Added one test for estimate_mode="unset"

in test/functional/wallet_send.py:263 in fa31347cb1 outdated

 259 | @@ -260,17 +260,15 @@ def run_test(self):
 260 |          res2 = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, fee_rate=1, add_to_wallet=False)
 261 |          assert_equal(self.nodes[1].decodepsbt(res1["psbt"])["fee"], self.nodes[1].decodepsbt(res2["psbt"])["fee"])
 262 |  
 263 | -        # Passing conf_target 0, estimate_mode "" as placeholder arguments should allow fee_rate to apply.
 264 | -        res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=0, estimate_mode="", fee_rate=7, add_to_wallet=False)
 265 | +        res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, fee_rate=7, add_to_wallet=False)

jonatack commented at 11:01 AM on November 18, 2020:

# Passing conf_target null, estimate_mode "" as placeholder arguments should allow fee_rate to apply.
res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=None, estimate_mode="", fee_rate=7, add_to_wallet=False)

MarcoFalke commented at 10:39 AM on November 19, 2020:

Added one test for estimate_mode="unset"

in test/functional/wallet_send.py:272 in fa31347cb1 outdated

 270 |          fee = self.nodes[1].decodepsbt(res["psbt"])["fee"]
 271 |          assert_fee_amount(fee, Decimal(len(res["hex"]) / 2), Decimal("0.00002"))
 272 |  
 273 | -        # Passing conf_target 0, estimate_mode "" as placeholder arguments should allow fee_rate to apply.
 274 | -        res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_conf_target=0, arg_estimate_mode="", arg_fee_rate=4.531, add_to_wallet=False)
 275 | +        res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_fee_rate=4.531, add_to_wallet=False)

jonatack commented at 11:01 AM on November 18, 2020:

# Passing conf_target null, estimate_mode "" as placeholder arguments should allow fee_rate to apply.
res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_conf_target=None, arg_estimate_mode="", arg_fee_rate=4.531, add_to_wallet=False)

MarcoFalke commented at 10:39 AM on November 19, 2020:

Added one test for estimate_mode="unset"

jonatack commented at 11:03 AM on November 18, 2020: member

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

laanwj commented at 8:44 AM on November 19, 2020: member

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.

MarcoFalke force-pushed on Nov 19, 2020

jonatack commented at 11:22 AM on November 19, 2020: member

Tested ACK fa7df9be0d1034d2bbbde19a45b1e706368b4c1b

jonatack commented at 11:31 AM on November 19, 2020: member

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.

MarcoFalke commented at 12:16 PM on November 19, 2020: member

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 commented at 12:34 PM on November 19, 2020: member

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 commented at 12:37 PM on November 19, 2020: member

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}'

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

MarcoFalke force-pushed on Nov 19, 2020

MarcoFalke commented at 12:54 PM on November 19, 2020: member

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

jonatack commented at 2:25 PM on November 19, 2020: member

ACK fa69c2c78455fd0dc436018fece9ff7fc83a180d

in src/wallet/rpcwallet.cpp:206 in fa69c2c784

 200 | @@ -201,22 +201,20 @@ static std::string LabelFromValue(const UniValue& value)
 201 |   * @param[in]     wallet            Wallet reference
 202 |   * @param[in,out] cc                Coin control to be updated
 203 |   * @param[in]     conf_target       UniValue integer; confirmation target in blocks, values between 1 and 1008 are valid per policy/fees.h;
 204 | - *                                      if a fee_rate is present, 0 is allowed here as a no-op positional placeholder
 205 |   * @param[in]     estimate_mode     UniValue string; fee estimation mode, valid values are "unset", "economical" or "conservative";
 206 | - *                                      if a fee_rate is present, "" is allowed here as a no-op positional placeholder
 207 |   * @param[in]     fee_rate          UniValue real; fee rate in sat/vB;
 208 | - *                                      if a fee_rate is present, both conf_target and estimate_mode must either be null, or no-op
 209 | + *                                      if present, both conf_target and estimate_mode must either be null, or "unset"

Sjors commented at 8:36 AM on November 23, 2020:

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.

jonatack commented at 8:43 AM on November 23, 2020:

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

MarcoFalke commented at 12:47 PM on November 23, 2020:

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

Sjors commented at 8:37 AM on November 23, 2020: member

tACK fa69c2c78455fd0dc436018fece9ff7fc83a180d modulo unset

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

MarcoFalke commented at 1:17 PM on November 23, 2020: member

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 commented at 6:42 PM on November 24, 2020: member

ACK fa69c2c78455fd0dc436018fece9ff7fc83a180d

MarcoFalke merged this on Nov 25, 2020

MarcoFalke closed this on Nov 25, 2020

MarcoFalke commented at 7:09 AM on November 25, 2020: member

Backported in #20485

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

MarcoFalke referenced this in commit d47d16025e on Nov 25, 2020

MarcoFalke deleted the branch on Nov 25, 2020

sidhujag referenced this in commit ec85366aab on Nov 25, 2020

DrahtBot locked this on Feb 15, 2022