Remove deprecated rpc options

jnewbery commented at 7:45 pm on February 2, 2018: member

There were some RPC/RPC options deprecated in v0.16. Those can now be removed from master since v0.16 has been branched.

estimatefee RPC has been removed. The feature_fee_estimation.py test has been updated to remove the RPC, but doesn’t yet have good coverage of the replacement RPC estimatesmartfee. Improving the test coverage should be done in a new PR. (#11031)
the errors field returned by getmininginfo has been deprecated and replaced by a warning field. (#10858)
providing addresses as inputs to createmultisig has been deprecated. Users should use addmultisigaddress instead (#11415)
The return format from addmultisigaddress has changed (#11415)

getwitnessaddress was also deprecated in v0.16 and can be removed, but many tests are using that RPC, so it’s a larger job to remove. It should be removed in a separate PR (possibly after #11739 and #11398 have been merged and the segwit test code tidied up)

[tests] Fix style warnings in feature_fee_estimation.py d119f2ec1a

[tests] Remove tests for deprecated estimatefee RPC a5623b1615

[tests] Remove estimatefee from rpc_deprecated.py test a8e437a02f

in src/rpc/mining.cpp:770 in a5f1c8e177 outdated

802-    CFeeRate feeRate = ::feeEstimator.estimateFee(nBlocks);
803-    if (feeRate == CFeeRate(0))
804-        return -1.0;
805-
806-    return ValueFromAmount(feeRate.GetFeePerK());
807+    throw JSONRPCError(RPC_METHOD_DEPRECATED, "estimatefee was removed in v0.17.\n"

promag commented at 7:50 pm on February 2, 2018:

Why not remove?

jnewbery commented at 8:36 pm on February 2, 2018:

Because there’s no harm in leaving the warning in for one additional release. Output is now:

0→ bcli estimatefee 4
1error code: -32
2error message:
3estimatefee was removed in v0.17.
4Clients should use estimatesmartfee.

promag commented at 8:43 pm on February 2, 2018:

Alright, in the future we should do the same for others. Not sure if this is documented (or should be).

laanwj commented at 10:02 am on February 6, 2018:

~~Nit: No need for \n at the end of exception messages.~~ Edit: never mind, I didn’t see it was continued on the next line due to the comment interrupting it, sorry.

promag commented at 11:39 am on February 8, 2018:

I guess there is no need for \n. There are other multiple sentence errors without it.

jnewbery commented at 2:06 pm on February 8, 2018:

ugh. This error message was split over two lines before the commit. Please can we focus on reviewing the code changes and not how many new lines there should be in error messages?

in src/rpc/mining.cpp:221 in a5f1c8e177 outdated

217@@ -219,11 +218,7 @@ UniValue getmininginfo(const JSONRPCRequest& request)
218     obj.push_back(Pair("networkhashps",    getnetworkhashps(request)));
219     obj.push_back(Pair("pooledtx",         (uint64_t)mempool.size()));
220     obj.push_back(Pair("chain",            Params().NetworkIDString()));
221-    if (IsDeprecatedRPCEnabled("getmininginfo")) {
222-        obj.push_back(Pair("errors",       GetWarnings("statusbar")));
223-    } else {
224-        obj.push_back(Pair("warnings",     GetWarnings("statusbar")));
225-    }
226+    obj.push_back(Pair("warnings",     GetWarnings("statusbar")));

promag commented at 7:51 pm on February 2, 2018:

Nit, to align or not to align 👀 IMO one space.

jnewbery commented at 8:35 pm on February 2, 2018:

Fine. Done.

jnewbery force-pushed on Feb 2, 2018

fanquake added the label RPC/REST/ZMQ on Feb 2, 2018

[rpc] remove deprecated estimatefee RPC c6f09c2713

[rpc] Remove deprecated getmininginfo RPC option d066a1c069

in src/rpc/mining.cpp:949 in 7c5aede491 outdated

945@@ -986,7 +946,7 @@ static const CRPCCommand commands[] =
946 
947     { "generating",         "generatetoaddress",      &generatetoaddress,      {"nblocks","address","maxtries"} },
948 
949-    { "util",               "estimatefee",            &estimatefee,            {"nblocks"} },
950+    { "util",               "estimatefee",            &estimatefee,            {} },

MarcoFalke commented at 1:24 pm on February 3, 2018:

Should be hidden, I guess.

jnewbery commented at 8:19 pm on February 5, 2018:

Yes. Fixed.

jnewbery force-pushed on Feb 5, 2018

sipa commented at 9:00 pm on February 5, 2018: member

Concept ACK

in test/functional/rpc_deprecated.py:24 in ba8bac6236 outdated

21-        self.nodes[1].estimatefee(1)
22-
23-        self.log.info("Make sure that -deprecatedrpc=createmultisig allows it to take addresses")
24-        assert_raises_rpc_error(-5, "Invalid public key", self.nodes[0].createmultisig, 1, [self.nodes[0].getnewaddress()])
25-        self.nodes[1].createmultisig(1, [self.nodes[1].getnewaddress()])
26+        pass

laanwj commented at 9:41 am on February 6, 2018:

Please add a comment here that newly deprecated functionality should be tested here, otherwise this empty function looks really strange. Alternatively, maybe it’s better to remove this test entirely if it’s empty, It can always be brought back.

promag commented at 11:32 am on February 8, 2018:

IMO should stay otherwise it’s easy to add deprecated tests elsewhere. A comment would be nice.

jnewbery commented at 1:59 pm on February 8, 2018:

Comment added.

laanwj commented at 10:04 am on February 6, 2018: member

Looks good to me, a few small nits. Would make sense to mention final removal of these in doc/release-notes.md (the one on the master branch).

laanwj assigned laanwj on Feb 6, 2018

in src/rpc/misc.cpp:298 in 37562668c9 outdated

301-            } else
302-#endif
303             throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Invalid public key: %s\nNote that from v0.16, createmultisig no longer accepts addresses."
304-            " Clients must transition to using addmultisigaddress to create multisig addresses with addresses known to the wallet before upgrading to v0.17."
305-            " To use the deprecated functionality, start bitcoind with -deprecatedrpc=createmultisig", keys[i].get_str()));
306+            " Users must use addmultisigaddress to create multisig addresses with addresses known to the wallet.", keys[i].get_str()));

promag commented at 11:35 am on February 8, 2018:

I believe there is no test for this error?

jnewbery commented at 2:10 pm on February 8, 2018:

https://github.com/bitcoin/bitcoin/blob/b2645286745b77a8268d00497e0cbccecf42952c/test/functional/rpc_rawtransaction.py#L155

promag commented at 11:40 am on February 8, 2018: member

Tested ACK.

Would make sense to mention final removal of these in doc/release-notes.md

Or needs release notes tag.

I would reword commit “[tests] Remove tests for deprecated estimatefee RPC” to “[qa] Replace deprecated estimatefee with estimatesmartfee”.

Also remove:? https://github.com/bitcoin/bitcoin/blob/c9ca4f6024e01fdca509ae07887e6fe2157c6384/src/wallet/rpcwallet.cpp#L3140

[tests] Remove test for deprecated createmultsig option ed45c82019

[RPC] Remove deprecated createmultisig object cb28a0b07f

[RPC] Remove deprecated addmultisigaddress return format db1cbcc856

jnewbery commented at 2:12 pm on February 8, 2018: member

I would reword commit “[tests] Remove tests for deprecated estimatefee RPC” to “[qa] Replace deprecated estimatefee with estimatesmartfee”.

That’s not what the commit does. It removes the tests for estimatefee and leaves the (minimal) checking of estimatesmartfee in place.

Please see original comment:

The feature_fee_estimation.py test has been updated to remove the RPC, but doesn’t yet have good coverage of the replacement RPC estimatesmartfee. Improving the test coverage should be done in a new PR.

Feel free to contribute better testing for estimatesmartfee.

jnewbery force-pushed on Feb 8, 2018

in test/functional/rpc_deprecated.py:23 in db1cbcc856

29+        # self.log.info("Make sure that -deprecatedrpc=createmultisig allows it to take addresses")
30+        # assert_raises_rpc_error(-5, "Invalid public key", self.nodes[0].createmultisig, 1, [self.nodes[0].getnewaddress()])
31+        # self.nodes[1].createmultisig(1, [self.nodes[1].getnewaddress()])
32+        #
33+        # There are currently no deprecated RPC methods in master, so this
34+        # test is currently empty.

laanwj commented at 2:25 pm on February 8, 2018:

Thanks

laanwj merged this on Feb 8, 2018

laanwj closed this on Feb 8, 2018

laanwj referenced this in commit 3843780fd8 on Feb 8, 2018

laanwj unassigned laanwj on Feb 8, 2018

jnewbery deleted the branch on Feb 8, 2018

PastaPastaPasta referenced this in commit 168e93162f on Jun 9, 2020

PastaPastaPasta referenced this in commit 7e04a9a3b1 on Jun 9, 2020

PastaPastaPasta referenced this in commit 8254d30d21 on Jun 10, 2020

PastaPastaPasta referenced this in commit 55cc7d8f89 on Jun 10, 2020

PastaPastaPasta referenced this in commit da0042972b on Jun 11, 2020

PastaPastaPasta referenced this in commit 1b7e6b45af on Jun 11, 2020

DrahtBot locked this on Sep 8, 2021

Remove deprecated rpc options #12336