Make the dummy argument to getaddednodeinfo optional #8272

pull sipa wants to merge 1 commits into bitcoin:master from sipa:optionaladdnodedummy changing 3 files +7 −7

sipa commented at 6:33 PM on June 26, 2016: member

As suggested in #8113.
in src/rpc/net.cpp:None in 0c1ed2a869 outdated
```
 298 | @@ -299,10 +299,10 @@ UniValue getaddednodeinfo(const UniValue& params, bool fHelp)
 299 |  
 300 |      std::vector<AddedNodeInfo> vInfo = GetAddedNodeInfo();
 301 |  
 302 | -    if (params.size() == 2) {
 303 | +    if (params.size() == 2 || params.size() == 1 && params[0].type() == UniValue::VSTR) {
```
jonasschnelli commented at 8:07 PM on June 26, 2016:

use params[0].isStr() instead of params[0].type() == UniValue::VSTR?

laanwj commented at 11:16 AM on June 27, 2016:

Was first a bit doubtful about this, and going to suggest to only allow skipping the dummy argument when the second argument is not provided. But all in all I like this trick. This allows a future path to getting rid of the argument completely.

laanwj commented at 11:17 AM on June 27, 2016:

Although I'm not sure how this interacts with bitcoin-cli's conversion table.

sipa commented at 11:17 AM on June 27, 2016:

One downside I just realized: this likely does not work in bitcoin-cli because it will try to interpret the first argument as JSON.

We could remove the parsing, though, which would make old-style calls fail.

laanwj commented at 11:25 AM on June 27, 2016:

Yes, that'd be a possible solution, it would break backwards compat for bitcoin-cli but not for direct JSON-RPC users. Or we could go all the way, remove backwards compatibility completely and just mention it in the release notes. After all this is a rarely-used call.
jonasschnelli commented at 8:07 PM on June 26, 2016: contributor

utACK 0c1ed2a86928cb1b086b8f7ae9abeee09643a05f (once nit is solved)
jonasschnelli added the label RPC/REST/ZMQ on Jun 26, 2016
laanwj commented at 11:06 AM on August 3, 2016: member

I think we should simply remove the dummy argument for 0.14, and change the API, and mention that in the release notes. To try to accommodate the old usage is unnecessary complexity, especially with regard to bitcoin-cli parsing.
theuni commented at 7:50 PM on August 3, 2016: member

Agree with @laanwj
sipa force-pushed on Aug 3, 2016
sipa commented at 8:05 PM on August 3, 2016: member

Updated to just remove it.
laanwj commented at 1:04 PM on August 4, 2016: member
Also need to get rid of this line in src/rpc/client.cpp:
```
{ "getaddednodeinfo", 0 }
```
sipa force-pushed on Sep 1, 2016
sipa commented at 1:52 PM on September 1, 2016: member

Rebased, added to release nodes, and removed the rpccient.cpp line.
laanwj commented at 2:23 PM on September 1, 2016: member

This still shows as conflicted. Did we merge something that conflicted while you were rebasing?
Make the dummy argument to getaddednodeinfo optional 854f1af22e
sipa force-pushed on Sep 1, 2016
sipa commented at 2:38 PM on September 1, 2016: member

@laanwj yes... #8638 conflicted in the release notes.
laanwj merged this on Sep 2, 2016
laanwj closed this on Sep 2, 2016
laanwj referenced this in commit 91990ee01d on Sep 2, 2016
codablock referenced this in commit 5d3ff475b6 on Sep 19, 2017
codablock referenced this in commit dac3f4074e on Jan 9, 2018
codablock referenced this in commit 4d0be42c6e on Jan 9, 2018
andvgal referenced this in commit f04becccc9 on Jan 6, 2019
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#8113 Rework addnode behaviour #8455 TODO for release notes 0.14.0