Get rid of ambiguous OutputType::NONE value by ryanofsky · Pull Request #12729 · bitcoin/bitcoin

ryanofsky · 2018-03-19T20:40:20Z

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: Get rid of ambiguous OutputType::NONE value #12729 (comment) and Get rid of ambiguous OutputType::NONE value #12729 (comment)
Add wallet AddressChangeType method to complement TransactionChangeType: Get rid of ambiguous OutputType::NONE value #12729 (comment).

maflcko · 2018-03-19T20:55:09Z

src/wallet/rpcwallet.cpp

-        output_type = ParseOutputType(request.params[1].get_str(), pwallet->m_default_address_type);
-        if (output_type == OutputType::NONE) {
+        if (!ParseOutputType(request.params[1].get_str(), output_type)) {
            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Unknown address type '%s'", request.params[1].get_str()));


Mind adding a test for an empty string?

Mind adding a test for an empty string?

Added tests in 3454f7b

ryanofsky

Added 1 commits 3be62ae -> 3454f7b (pr/nonone.1 -> pr/nonone.2, compare)
Squashed 3454f7b -> 305b6bc (pr/nonone.2 -> pr/nonone.3)

ryanofsky · 2018-03-20T18:39:45Z

src/wallet/rpcwallet.cpp

-        output_type = ParseOutputType(request.params[1].get_str(), pwallet->m_default_address_type);
-        if (output_type == OutputType::NONE) {
+        if (!ParseOutputType(request.params[1].get_str(), output_type)) {
            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Unknown address type '%s'", request.params[1].get_str()));


Mind adding a test for an empty string?

Added tests in 3454f7b

promag · 2018-03-20T22:21:27Z

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 · 2018-03-20T22:32:21Z

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 · 2018-03-21T00:09:47Z

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

sipa · 2018-03-21T01:37:29Z

src/wallet/wallet.cpp

-        return default_type;
-    } else if (type == OUTPUT_TYPE_STRING_LEGACY) {
-        return OutputType::LEGACY;
+    if (type == OUTPUT_TYPE_STRING_LEGACY) {


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.

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.

Oh, I missed that. Sounds reasonable.

sipa · 2018-03-21T01:38:45Z

src/qt/paymentserver.cpp

        // it's the output type we use subject to privacy issues, but not restricted by what
        // other software supports.
-        const OutputType change_type = wallet->m_default_change_type != OutputType::NONE ? wallet->m_default_change_type : wallet->m_default_address_type;
+        const OutputType change_type = wallet->m_default_change_type != OutputType::CHANGE_AUTO ? wallet->m_default_change_type : wallet->m_default_address_type;


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?

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

Added 1 commits 305b6bc -> 30a961b (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 · 2018-03-21T18:38:33Z

src/qt/paymentserver.cpp

        // it's the output type we use subject to privacy issues, but not restricted by what
        // other software supports.
-        const OutputType change_type = wallet->m_default_change_type != OutputType::NONE ? wallet->m_default_change_type : wallet->m_default_address_type;
+        const OutputType change_type = wallet->m_default_change_type != OutputType::CHANGE_AUTO ? wallet->m_default_change_type : wallet->m_default_address_type;


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

Suggested bitcoin#12729 (comment)

sipa · 2018-03-21T22:50:57Z

utACK 30a961b

maflcko

utACK, some questions about the code

maflcko · 2018-03-22T18:06:43Z

doc/release-notes.md

  name of any wallet is just its `<path>` string.
+- Passing an empty string (`""`) as the `address_type` parameter to
+  `getnewaddress`, `getrawchangeaddress`, `addmultisigaddress`,
+  `addmultisigaddress`, `fundrawtransaction` RPCs is now an error. Previously,


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

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

Typo, fixed in 9141e95

maflcko · 2018-03-22T18:21:04Z

src/wallet/rpcwallet.cpp

            }
-            coinControl.m_change_type = ParseOutputType(options["change_type"].get_str(), pwallet->m_default_change_type);
-            if (coinControl.m_change_type == OutputType::NONE) {
+            coinControl.m_change_type = pwallet->m_default_change_type;


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.

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

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

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.

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

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

Based on suggestion by Pieter Wuille <[email protected]> at bitcoin#12119 (comment) After bitcoin#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.

maflcko · 2018-04-06T17:01:14Z

@ryanofsky Are you still working on this?

Suggested bitcoin#12729 (comment)

ryanofsky

@ryanofsky Are you still working on this?

Should be ready now, sorry for the delay.

Rebased 30a961b -> 38c2647 (pr/nonone.4 -> pr/nonone.5) due to conflicts with #10244
Added 1 commits 38c2647 -> 9141e95 (pr/nonone.5 -> pr/nonone.6, compare)
Squashed 9141e95 -> eaaf9b4 (pr/nonone.6 -> pr/nonone.7)

ryanofsky · 2018-04-05T17:16:48Z

doc/release-notes.md

  name of any wallet is just its `<path>` string.
+- Passing an empty string (`""`) as the `address_type` parameter to
+  `getnewaddress`, `getrawchangeaddress`, `addmultisigaddress`,
+  `addmultisigaddress`, `fundrawtransaction` RPCs is now an error. Previously,


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

Typo, fixed in 9141e95

ryanofsky · 2018-04-05T17:17:06Z

src/wallet/rpcwallet.cpp

            }
-            coinControl.m_change_type = ParseOutputType(options["change_type"].get_str(), pwallet->m_default_change_type);
-            if (coinControl.m_change_type == OutputType::NONE) {
+            coinControl.m_change_type = pwallet->m_default_change_type;


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

maflcko · 2018-04-07T15:46:46Z

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

maflcko · 2018-04-07T19:25:59Z

Needs rebase

ryanofsky · 2018-04-10T10:33:22Z

Needs rebase

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

promag

Tested ACK 1e46d8a.

promag · 2018-05-02T21:55:13Z

src/wallet/rpcwallet.cpp

            }
-            coinControl.m_change_type = ParseOutputType(options["change_type"].get_str(), pwallet->m_default_change_type);
-            if (coinControl.m_change_type == OutputType::NONE) {
+            coinControl.m_change_type = pwallet->m_default_change_type;


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 · 2018-05-03T00:01:02Z

reutACK 1e46d8a

@sipa

1e46d8a Get rid of ambiguous OutputType::NONE value (Russell Yanofsky) Pull request description: 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 (comment) - [ ] Add wallet `AddressChangeType` method to complement `TransactionChangeType`: #12729 (comment). Tree-SHA512: 8b08b272bcb177a0a9e556dcd965840a7fe601ef83ca97938b879c9b1a33b5b3f96939e1bceef11ba7c644ac21bfd6c1dbc6ca715cd1da4ace50475240e4ee48

@sipa

1e46d8a Get rid of ambiguous OutputType::NONE value (Russell Yanofsky) Pull request description: Based on suggestion by @sipa bitcoin#12119 (comment) After bitcoin#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 bitcoin#12408 by @MarcoFalke Followups for future PRs: - [ ] Add explicit support for specifying "auto" in `ParseOutputType` as suggested by promag and sipa: bitcoin#12729 (comment) and bitcoin#12729 (comment) - [ ] Add wallet `AddressChangeType` method to complement `TransactionChangeType`: bitcoin#12729 (comment). Tree-SHA512: 8b08b272bcb177a0a9e556dcd965840a7fe601ef83ca97938b879c9b1a33b5b3f96939e1bceef11ba7c644ac21bfd6c1dbc6ca715cd1da4ace50475240e4ee48

maflcko reviewed Mar 19, 2018

View reviewed changes

fanquake added the Wallet label Mar 19, 2018

ryanofsky force-pushed the pr/nonone branch from 3be62ae to 305b6bc Compare March 20, 2018 18:39

ryanofsky commented Mar 20, 2018

View reviewed changes

sipa reviewed Mar 21, 2018

View reviewed changes

ryanofsky commented Mar 21, 2018

View reviewed changes

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Mar 21, 2018

Add ChangeAddressType method

30a961b

Suggested bitcoin#12729 (comment)

maflcko reviewed Mar 22, 2018

View reviewed changes

ryanofsky force-pushed the pr/nonone branch from 30a961b to eaaf9b4 Compare April 6, 2018 18:53

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Apr 6, 2018

Add ChangeAddressType method

eaaf9b4

Suggested bitcoin#12729 (comment)

ryanofsky commented Apr 6, 2018

View reviewed changes

ryanofsky force-pushed the pr/nonone branch from eaaf9b4 to 1e46d8a Compare April 9, 2018 14:27

promag reviewed May 2, 2018

View reviewed changes

laanwj merged commit 1e46d8a into bitcoin:master May 3, 2018

Bushstar mentioned this pull request May 4, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#320

Merged

Sjors mentioned this pull request Mar 9, 2019

Make OutputType consistent with Descriptor and return it #15567

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

ryanofsky commented Mar 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Mar 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Mar 20, 2018

Uh oh!

promag commented Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Mar 21, 2018 • 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!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Mar 21, 2018 • 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!

sipa commented Mar 21, 2018

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa May 2, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Apr 6, 2018

Uh oh!

ryanofsky 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Apr 7, 2018

Uh oh!

maflcko commented Apr 7, 2018

Uh oh!

ryanofsky commented Apr 10, 2018

Uh oh!

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

ryanofsky commented Mar 19, 2018 •

edited

Loading

promag commented Mar 20, 2018 •

edited

Loading

promag commented Mar 21, 2018 •

edited

Loading

ryanofsky Mar 21, 2018 •

edited

Loading

ryanofsky Mar 21, 2018 •

edited

Loading

sipa May 2, 2018 •

edited

Loading

ryanofsky left a comment •

edited

Loading

promag left a comment •

edited

Loading