Get rid of ambiguous OutputType::NONE value #12729

ryanofsky commented at 8:40 PM on March 19, 2018: member

Based on suggestion by @sipa #12119 (comment)

After #12119, the NONE output type was overloaded to refer to either an output type that couldn't be parsed, or to an automatic change output mode. This change drops the NONE enum and uses a simple bool to indicate parse failure, and a new CHANGE_AUTO enum to refer the change output type.

This change is almost a pure refactoring except it makes RPCs reject empty string ("") address types instead of treating them like they were unset. This simplifies the parsing code a little bit and could prevent RPC usage mistakes. It's noted in the release notes.

Follows up #12408 by @MarcoFalke

Followups for future PRs:

Add explicit support for specifying "auto" in ParseOutputType as suggested by promag and sipa: #12729 (comment) and #12729 (review)
Add wallet AddressChangeType method to complement TransactionChangeType: #12729 (review).

in src/wallet/rpcwallet.cpp:168 in 3be62aeb1c outdated

 163 | @@ -164,8 +164,7 @@ UniValue getnewaddress(const JSONRPCRequest& request)
 164 |  
 165 |      OutputType output_type = pwallet->m_default_address_type;
 166 |      if (!request.params[1].isNull()) {
 167 | -        output_type = ParseOutputType(request.params[1].get_str(), pwallet->m_default_address_type);
 168 | -        if (output_type == OutputType::NONE) {
 169 | +        if (!ParseOutputType(request.params[1].get_str(), output_type)) {
 170 |              throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Unknown address type '%s'", request.params[1].get_str()));

MarcoFalke commented at 8:55 PM on March 19, 2018:

Mind adding a test for an empty string?

ryanofsky commented at 6:39 PM on March 20, 2018:

Mind adding a test for an empty string?

Added tests in 3454f7bf3b4f1ae041c549dbe9e1e9fae3a8b4aa

fanquake added the label Wallet on Mar 19, 2018

ryanofsky force-pushed on Mar 20, 2018

ryanofsky commented at 6:40 PM on March 20, 2018: member

Added 1 commits 3be62aeb1ca1658dde30e2b29009894c3aa952f6 -> 3454f7bf3b4f1ae041c549dbe9e1e9fae3a8b4aa (pr/nonone.1 -> pr/nonone.2, compare) Squashed 3454f7bf3b4f1ae041c549dbe9e1e9fae3a8b4aa -> 305b6bc1d823d3200dc7d0d4501db3301cf0f6ee (pr/nonone.2 -> pr/nonone.3)

promag commented at 10:21 PM on March 20, 2018: member

It is still possible to pass null

Only with named parameters.

or leave the parameter unset to use the default address type

How about explicit setting like -changetype=auto and { "change_type": "auto" } or alike? It would allow a specific -changetype and auto change type for RPC.

ryanofsky commented at 10:32 PM on March 20, 2018: member

It is still possible to pass null

Only with named parameters.

Not sure what this is referring to. The checks are all for isNull(). Let me know if you would like me to rephrase the release notes in some way though.

How about explicit setting like -changetype=auto

Good idea for another PR, but for now I'm just trying to do a code cleanup.

promag commented at 12:09 AM on March 21, 2018: member

Good idea for another PR, but for now I'm just trying to do a code cleanup.

Sounds good.

Regarding the null/omit thing, and considering the following:

# bitcoin.conf
addresstype=bech32
changetype=bech32

IMO we could allow empty string as it enables to:

launch and use software default address type overriding bitcoin.conf:

bitcoind -addresstype=

(nit) pass empty string in bitcoin-cli without named parameters:

bitcoin-cli getnewaddress "" ""

(nit) set change type to default (same as not setting):

bitcoin-cli fundrawtransaction "hex" '{"change_type": ""}'

The first case is the most relevant.

See 242c8bd for the change (minus release notes update).

in src/wallet/wallet.cpp:4220 in 305b6bc1d8 outdated

4210 |  {
4211 | -    if (type.empty()) {
4212 | -        return default_type;
4213 | -    } else if (type == OUTPUT_TYPE_STRING_LEGACY) {
4214 | -        return OutputType::LEGACY;
4215 | +    if (type == OUTPUT_TYPE_STRING_LEGACY) {

sipa commented at 1:37 AM on March 21, 2018:

Perhaps it would be good to support explicitly selecting the auto behaviour (with a "auto" string, only available for change?)

Otherwise it's hard to override a config file setting that set it to something else e.g.

promag commented at 2:05 PM on March 21, 2018:

FYI #12729 (comment):

How about explicit setting like -changetype=auto

Good idea for another PR, but for now I'm just trying to do a code cleanup.

sipa commented at 8:30 PM on March 21, 2018:

Oh, I missed that. Sounds reasonable.

in src/qt/paymentserver.cpp:646 in 305b6bc1d8 outdated

 642 | @@ -643,7 +643,7 @@ void PaymentServer::fetchPaymentACK(CWallet* wallet, const SendCoinsRecipient& r
 643 |          // use for change. Despite an actual payment and not change, this is a close match:
 644 |          // it's the output type we use subject to privacy issues, but not restricted by what
 645 |          // other software supports.
 646 | -        const OutputType change_type = wallet->m_default_change_type != OutputType::NONE ? wallet->m_default_change_type : wallet->m_default_address_type;
 647 | +        const OutputType change_type = wallet->m_default_change_type != OutputType::CHANGE_AUTO ? wallet->m_default_change_type : wallet->m_default_address_type;

sipa commented at 1:38 AM on March 21, 2018:

This expression to determine the change type seems to be repeated pattern (at least twice, maybe that's all). Do you think it's worth it to abstract it out into a CWallet method?

ryanofsky commented at 6:38 PM on March 21, 2018:

This expression to determine the change type seems to be repeated pattern (at least twice, maybe that's all). Do you think it's worth it to abstract it out into a CWallet method?


EDIT: Dropped this for now to simplify the PR and avoid a conflict. Will include the change in followup PR (see description).

ryanofsky commented at 6:47 PM on March 21, 2018: member

Added 1 commits 305b6bc1d823d3200dc7d0d4501db3301cf0f6ee -> 30a961ba74abf1eaadbe887473493b2361170254 (pr/nonone.3 -> pr/nonone.4, compare)

bitcoind -addresstype=

Resetting config file options on the command line works on master, and this PR doesn't change that behavior. I agree it's useful.

bitcoin-cli getnewaddress "" "" bitcoin-cli fundrawtransaction "hex" '{"change_type": ""}'

Passing an empty string as a JSON-RPC output type value now triggers an error with this PR, instead of falling back to defaults like in your commit and current master. To me this change seems like a pure improvement. It removes a corner case from the ParseOutputType function, making the code more straightforward. It can prevent mistakes in JSON-RPC client code, like accidentally confusing account/label and address type parameters, or meaning to choose an address type but not properly initializing a variable and passing an empty string instead. There are already two ways in JSON to not specify an option and fall back to defaults: (1) you can not pass the option (2) you can pass the option but set it to null. Going of out of the way to accept empty strings as valid enum values doesn't seem helpful in light of the alternatives, need for more parsing logic, and possibilities of client side bugs.

ryanofsky referenced this in commit 30a961ba74 on Mar 21, 2018

sipa commented at 10:50 PM on March 21, 2018: member

utACK 30a961ba74abf1eaadbe887473493b2361170254

in doc/release-notes.md:98 in 30a961ba74 outdated

  92 | @@ -93,6 +93,11 @@ Low-level RPC changes
  93 |    now the empty string `""` instead of `"wallet.dat"`. If bitcoin is started
  94 |    with any `-wallet=<path>` options, there is no change in behavior, and the
  95 |    name of any wallet is just its `<path>` string.
  96 | +- Passing an empty string (`""`) as the `address_type` parameter to
  97 | +  `getnewaddress`, `getrawchangeaddress`, `addmultisigaddress`,
  98 | +  `addmultisigaddress`, `fundrawtransaction` RPCs is now an error. Previously,

MarcoFalke commented at 6:06 PM on March 22, 2018:

nit: Is there any difference between "addmultisigaddress" and "addmultisigaddress"?

ryanofsky commented at 5:16 PM on April 5, 2018:

nit: Is there any difference between "addmultisigaddress" and "addmultisigaddress"?

Typo, fixed in 9141e95de9f6c0a466f6ae13269d0acae764aadd

in src/wallet/rpcwallet.cpp:3180 in 30a961ba74 outdated

3179 | @@ -3183,8 +3180,8 @@ UniValue fundrawtransaction(const JSONRPCRequest& request)
3180 |              if (options.exists("changeAddress")) {
3181 |                  throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both changeAddress and address_type options");
3182 |              }
3183 | -            coinControl.m_change_type = ParseOutputType(options["change_type"].get_str(), pwallet->m_default_change_type);
3184 | -            if (coinControl.m_change_type == OutputType::NONE) {
3185 | +            coinControl.m_change_type = pwallet->m_default_change_type;

MarcoFalke commented at 6:21 PM on March 22, 2018:

Why is this assignment needed? Either ParseOutputType in the next line succeeds and overwrites the value or it fails and we get thrown out of this function.

I think you can safely remove this line.

ryanofsky commented at 5:17 PM on April 5, 2018:

Why is this assignment needed?

The optional value is dereferenced in the next line (*coinControl.m_change_type), and this only works if the option is set. Dereferencing an unset option is bad, similar to dereferencing a null pointer (http://en.cppreference.com/w/cpp/utility/optional/operator%2A).

MarcoFalke commented at 2:45 PM on April 7, 2018:

Ah, thanks. So the assignment is needed, but the value doesn't matter.

promag commented at 9:55 PM on May 2, 2018:

Right, also got confused because forgot that CCoinControl::m_change_type is boost::optional<OutputType>.

Nit, to avoid the assignment is needed, but the value doesn't matter detail:

OutputType change_type;
if (!ParseOutputType(options["change_type"].get_str(), change_type) {
    ...
}
coinControl.m_change_type = change_type;

Otherwise a comment above could help.

sipa commented at 11:51 PM on May 2, 2018:

@ryanofsky I think it's perfectly legal to dereference a pointer to an uninitialized value, as long as you don't read it? This is different from dereferencing a null pointer, which is always undefined.

No problem with initializing it, though.

promag commented at 12:21 AM on May 3, 2018:

@sipa the following

  boost::optional<std::string> s;
  *s = "hello";

gives

Assertion failed: (this->is_initialized()), function get, file /usr/local/include/boost/optional/optional.hpp, line 1191.

MarcoFalke commented at 6:21 PM on March 22, 2018: member

utACK, some questions about the code

Get rid of ambiguous OutputType::NONE value

Based on suggestion by Pieter Wuille <pieter.wuille@gmail.com> at
https://github.com/bitcoin/bitcoin/pull/12119#issuecomment-357982763

After #12119, the NONE output type was overloaded to refer to either an output
type that couldn't be parsed, or to an automatic change output mode.  This
change drops the NONE enum and uses a simple bool indicate parse failure, and a
new CHANGE_AUTO enum to refer the change output type.

This change is almost a pure refactoring except it makes RPCs reject empty
string ("") address types instead of treating them like they were unset. This
simplifies the parsing code a little bit and could prevent RPC usage mistakes.
It's noted in the release notes.

1e46d8ae89

MarcoFalke commented at 5:01 PM on April 6, 2018: member

@ryanofsky Are you still working on this?

ryanofsky force-pushed on Apr 6, 2018

ryanofsky referenced this in commit eaaf9b4676 on Apr 6, 2018

ryanofsky commented at 6:58 PM on April 6, 2018: member

@ryanofsky Are you still working on this?

Should be ready now, sorry for the delay.

Rebased 30a961ba74abf1eaadbe887473493b2361170254 -> 38c2647588d18cbb8d758335e01e48edbbd6f292 (pr/nonone.4 -> pr/nonone.5) due to conflicts with #10244 Added 1 commits 38c2647588d18cbb8d758335e01e48edbbd6f292 -> 9141e95de9f6c0a466f6ae13269d0acae764aadd (pr/nonone.5 -> pr/nonone.6, compare) Squashed 9141e95de9f6c0a466f6ae13269d0acae764aadd -> eaaf9b46768ad1cfcb445f9118e8bc701e73e427 (pr/nonone.6 -> pr/nonone.7)

MarcoFalke commented at 3:46 PM on April 7, 2018: member

Concept ACK eaaf9b4 (downgrading my utACK, since I am going to re-review from scratch)

MarcoFalke commented at 7:25 PM on April 7, 2018: member

Needs rebase

ryanofsky force-pushed on Apr 9, 2018

ryanofsky commented at 10:33 AM on April 10, 2018: member

Needs rebase

Dropped second commit to avoid need for rebase: eaaf9b46768ad1cfcb445f9118e8bc701e73e427 -> 1e46d8ae897aded3367a2dd63a76991882d170fa (pr/nonone.7 -> pr/nonone.8)

promag commented at 10:00 PM on May 2, 2018: member

Tested ACK 1e46d8a.

sipa commented at 12:01 AM on May 3, 2018: member

reutACK 1e46d8ae897aded3367a2dd63a76991882d170fa

laanwj merged this on May 3, 2018

laanwj closed this on May 3, 2018

laanwj referenced this in commit 979150bc23 on May 3, 2018

TheArbitrator referenced this in commit d0fefe7ec9 on Jun 21, 2021

MarcoFalke locked this on Sep 8, 2021