-
Notifications
You must be signed in to change notification settings - Fork 35.6k
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
Get rid of ambiguous OutputType::NONE value #12729
Conversation
@@ -164,8 +164,7 @@ UniValue getnewaddress(const JSONRPCRequest& request) | |||
|
|||
OutputType output_type = pwallet->m_default_address_type; | |||
if (!request.params[1].isNull()) { | |||
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())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mind adding a test for an empty string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mind adding a test for an empty string?
Added tests in 3454f7b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added 1 commits 3be62ae -> 3454f7b (pr/nonone.1 -> pr/nonone.2, compare)
Squashed 3454f7b -> 305b6bc (pr/nonone.2 -> pr/nonone.3)
@@ -164,8 +164,7 @@ UniValue getnewaddress(const JSONRPCRequest& request) | |||
|
|||
OutputType output_type = pwallet->m_default_address_type; | |||
if (!request.params[1].isNull()) { | |||
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())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mind adding a test for an empty string?
Added tests in 3454f7b
Only with named parameters.
How about explicit setting like |
Not sure what this is referring to. The checks are all for
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:
IMO we could allow empty string as it enables to:
bitcoind -addresstype=
bitcoin-cli getnewaddress "" ""
bitcoin-cli fundrawtransaction "hex" '{"change_type": ""}' The first case is the most relevant. See 242c8bd for the change (minus release notes update). |
return default_type; | ||
} else if (type == OUTPUT_TYPE_STRING_LEGACY) { | ||
return OutputType::LEGACY; | ||
if (type == OUTPUT_TYPE_STRING_LEGACY) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I missed that. Sounds reasonable.
src/qt/paymentserver.cpp
Outdated
@@ -643,7 +643,7 @@ void PaymentServer::fetchPaymentACK(CWallet* wallet, const SendCoinsRecipient& r | |||
// use for change. Despite an actual payment and not change, this is a close match: | |||
// 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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/qt/paymentserver.cpp
Outdated
@@ -643,7 +643,7 @@ void PaymentServer::fetchPaymentACK(CWallet* wallet, const SendCoinsRecipient& r | |||
// use for change. Despite an actual payment and not change, this is a close match: | |||
// 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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
utACK 30a961b |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK, some questions about the code
doc/release-notes.md
Outdated
@@ -93,6 +93,11 @@ Low-level RPC changes | |||
now the empty string `""` instead of `"wallet.dat"`. If bitcoin is started | |||
with any `-wallet=<path>` options, there is no change in behavior, and the | |||
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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Is there any difference between "addmultisigaddress" and "addmultisigaddress"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Is there any difference between "addmultisigaddress" and "addmultisigaddress"?
Typo, fixed in 9141e95
@@ -3183,8 +3180,8 @@ UniValue fundrawtransaction(const JSONRPCRequest& request) | |||
if (options.exists("changeAddress")) { | |||
throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both changeAddress and address_type options"); | |||
} | |||
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, thanks. So the assignment is needed, but the value doesn't matter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 <pieter.wuille@gmail.com> 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.
@ryanofsky Are you still working on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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)
doc/release-notes.md
Outdated
@@ -93,6 +93,11 @@ Low-level RPC changes | |||
now the empty string `""` instead of `"wallet.dat"`. If bitcoin is started | |||
with any `-wallet=<path>` options, there is no change in behavior, and the | |||
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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Is there any difference between "addmultisigaddress" and "addmultisigaddress"?
Typo, fixed in 9141e95
@@ -3183,8 +3180,8 @@ UniValue fundrawtransaction(const JSONRPCRequest& request) | |||
if (options.exists("changeAddress")) { | |||
throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both changeAddress and address_type options"); | |||
} | |||
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
Concept ACK eaaf9b4 (downgrading my utACK, since I am going to re-review from scratch) |
Needs rebase |
Dropped second commit to avoid need for rebase: eaaf9b4 -> 1e46d8a (pr/nonone.7 -> pr/nonone.8) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested ACK 1e46d8a.
@@ -3183,8 +3180,8 @@ UniValue fundrawtransaction(const JSONRPCRequest& request) | |||
if (options.exists("changeAddress")) { | |||
throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both changeAddress and address_type options"); | |||
} | |||
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
reutACK 1e46d8a |
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
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
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:
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)AddressChangeType
method to complementTransactionChangeType
: Get rid of ambiguous OutputType::NONE value #12729 (comment).