-
Notifications
You must be signed in to change notification settings - Fork 38.8k
Remove createwitnessaddress RPC command #8699
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
Conversation
src/rpc/misc.cpp
Outdated
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.
(typo) "Semantics"
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.
Fixed, thanks
3339721 to
656c2ad
Compare
|
utACK 656c2ad |
src/rpc/misc.cpp
Outdated
| UniValue createwitnessaddress(const UniValue& params, bool fHelp) | ||
| { | ||
| if (fHelp || params.size() < 1 || params.size() > 1) | ||
| if (fHelp || params.size() < 1 || params.size() > 1 || Params().NetworkIDString() == "main") |
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.
Please don't do a string test on the chain params. If you need a specific condition, add a ChainParams::AllowCreateWitness method or so.
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.
Also, is there any need to not just remove the RPC entirely?
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.
Just not sure if that's useful for testing. Should we keep it for test only or remove it entirely? I don't have strong opinion.
This RPC command is unsafe as it will return an address even if the script is invalid.
656c2ad to
86c3f8d
Compare
|
Revised to simply remove the command |
|
I'm a bit confused by the description. Is this only for 0.13.1? Should this be merged into master/0.14 at all?
Does it have value on testnet/regtest? |
|
@laanwj If this is merged, it is meant for all branches. |
|
So we want that RPC call to disappear forever, not just disable it on mainnet pending segwit rollout? |
|
I think it should never be allowed on mainnet. In principle we should not generate any address that may likely be unspendable. Also, I don't think it's very useful for testing. It is not used by any RPC test, and a witness address could be generated trivially with python. |
|
If it is never useful, sorry to ask, but why did this command make it into a release in the first place? Edit: obviously ACK on removing it |
|
It was part of the segwit pull request, and just added to mimick
createmultisig/addmultisigaddress. However, we only now realized that it's
pretty dangerous as it can't guarantee the returned address is valid. Maybe
it's still useful, but it at least needs a big warning.
|
86c3f8d Remove createwitnessaddress (Johnson Lau)
This RPC command is unsafe as it will return an address even if the script is invalid. Github-Pull: bitcoin#8699 Rebased-From: 86c3f8d
|
This is backported in #8815, removing tag |
For 0.13.1
createwitnessaddressdoes not examine script validity in any way and may return unspendable addresses. It should be disabled on mainnet.In longer term (maybe 0.14), we should disable all the sensitive commands (e.g. dumpprivkey, importprivkey) by default, and only be enabled with a command-line argument