Caller-initiated use of deprecatedrpc methods #14394

As a bit of a follow-up to #14363 I think the deprecation of rpc functionality can be improved significantly.

One of the big problems with the way it is now, is that it's often undesirable to update client-software and bitcoin-core at the same time. Especially so when they're often maintained by different people/groups.

The deprecatedrpc is in a way a pretty intrusive way of just making sure the node operator knows that deprecated functionality is being used. I would suggest that this is generally the wrong "party" to be notifying anyway.

My suggestion is that in addition the rpc-client is the one that acknowledges it's using deprecated functionality. I'm not sure exactly how the implementation should work. But it could be done either connection-wide (an rpc-call to say which deprecated functionality it's using?), or as a flag to that specific call (acknowledge_deprecated=true).

Now as a bitcoin-client-software-author this makes a much smoother work flow. Let's say core 0.17 comes out and deprecates signrawtransaction in favor of signrawtransactionwithwallet. I can immediately update my software to acknowledge signrawtransaction is deprecated. Now it's immediately compatible with both 0.16 and 0.17. Once enough of my users (or my only user in a more corporate setting) has fully upgraded to 0.17 and I no longer care about supporting 0.16, I can simply replace the call with the new signrawtransactionwithwallet.

A change like this should be pretty uninvasive, and significantly make upgrades smoother for software using the rpc functionality.

Yeah, one of the big nuisances too on this is when you also support altcoins; so many are based on old codebases you end up having to use a bunch of different APIs to do the same thing.

This seems like a good idea. Maybe it could be tagged Up for grabs and good first issue.

I think all it would require is adding a JSONRPCRequest argument to the IsDeprecatedRPCEnabled function and having it return true if a ?deprecatedrpc=<method> query string parameter or Deprecated-Rpc: <method> bitcoin header is present in the request. We could also update bitcoin-cli to read -deprecatedrpc=<method> command line and configuration options, and use them in requests.

Hello! I would like to take a stab at this unless someone else is working on it already..?

Funny I've prototyped exactly what @ryanofsky said. Considering JSON-RPC specification and easy to use for the client I think the query string is the best option. And also for REST interface(?)

The problem is that IsDeprecatedRPCEnabled can needed deep in stack like https://github.com/bitcoin/bitcoin/blob/be992701b018f256db6d64786624be4cb60d8975/src/wallet/rpcwallet.cpp#L3469 Maybe the simplest solution is to pass JSONRPCRequest down?

I'd like to drop here the idea of a enablealldeprecatedrpc config flag instead, or in addition, since the above idea would very often result in that behavior anyway: public RPCClient(bool enableDeprecatedCalls){

But, to be fair, if we would really want to do a great job, someone would have to somehow create

1. A well maintained RPC documentation and make sure RPC won't ever get changed without it.
2. Introduce API modification policies, those can be relied upon by the user developer.

As for part 1, a good way could be to create a website, that extracts RPC help from GitHub, shows the proper documentation for every version, can compare the differences from version to version and would show the most recent changes, those not yet have been tagged, so to see what to expect in the future.

I'd like to drop here the idea of a enablealldeprecatedrpc config flag instead @nopara73 the explicit flag exists because it's important the user acknowledges that the feature will be gone. With that flag (especially if it's in the config file) the user will have surprises when the features are removed.

But, to be fair, if we would really want to do a great job, someone would have to somehow create

1. A well maintained RPC documentation and make sure RPC won't ever get changed without it.
2. Introduce API modification policies, those can be relied upon by the user developer.

I think we have both, not perfect, also we have the test suite.

But if you don't agree, suppose that what you suggest exists, it won't help — truth is clients lag — and some still rush in upgrading without caution.

What would help is to not have breaking changes but, like @jnewbery said, there are moments that, for the sake of the implementation, we must do that.

I think we have both

Are you referring to Bitcoin.org's documentation?

But if you don't agree, suppose that what you suggest exists, it won't help — truth is clients lag — and some still rush in upgrading without caution.

Assuming in your first sentence that you are referring to something I am not aware of and my suggestion indeed exist, I must say you are right. This suggestion is indeed my plan B. As I previously suggested (https://github.com/bitcoin/bitcoin/issues/14363) a "Never break the user space!" policy would solve the issue once and for all. Which is still my default suggestion.

What would help is to not have breaking changes but, like @jnewbery said, there are moments that, for the sake of the implementation, we must do that.

I should've read this last sentence of yours before answering the previous one.

Regarding the topic, I feel that if this is done it's going to make it worse, like clients/libraries will start to add that option to the request and only delay the issue for one release. I think this will bring too much noise to the client land.

Let's say core 0.17 comes out and deprecates signrawtransaction in favor of signrawtransactionwithwallet. I can immediately update my software to acknowledge signrawtransaction is deprecated. Now it's immediately compatible with both 0.16 and 0.17.

Why would you do that? Why not just acknowledge_deprecated=true all the time?

If I'd be a developer, I would prefer releasing software that doesn't break user software, even when if the user updates its underlying Core instance.

Thus, I'd keep sending acknowledge_deprecated=true flag preemptively, even for calls I think would never be changed, just to be sure, because what is the downside? Nothing really.

It must also be noted that the difference between acknowledge_deprecated=true and -deprecatedrpc=<methodname> is that, the developer may not want to (or can) mess up with -deprecatedrpc=<methodname> lines the config file for all rpc command it uses before the RPC would break. That just smells like a very dirty solution, compared to setting a simple flag true.

Anyhow, I'm not arguing for or against here, just giving some feedback on how the change would be used.

I still think the current approach is the best.

Upon new release the user must chose one of these:

• A) on the server — when upgrading enable specific deprecated features because client needs them, otherwise upgrade should not be done
• B) on the client — actually add support for new features and drop deprecated features, otherwise don't touch it
• C) keep calm, don't break, wait for client support.

@promag I think you're missing what makes it painful at the moment.

B) on the client — actually add support for new features and drop deprecated features, otherwise don't touch it

As the author of the client, you know 100% of your users are using the old version of core (as it would break if they upgraded), making it painful to try switch to something you know literally no-one is using when it's not even backwards compatible (without special flags). It's also not easy to ask those users to upgrade, because when they upgrade it'll potentially break a bunch of other stuff they are doing.

Honestly, the more I think about it, the more I think the current setup is over-engineered. I really think the best solution would be a sort of WARN_ONCE for deprecated methods, write to a deprecated.log . Then if people don't read the release notes (and previous warnings, or search the deprecated log) and get burnt upgrading -- then it's their problem.

That said, I'm just going to close this issue. After thought I think "Caller-initiated use of deprecatedrpc methods" is just a hack, and the better solution is removing the whole deprecated flags entirely. But I don't really care enough to bikeshed over it, so I'll just accept doing version-detection in my client.

A decent client should state which versions are supported. For instance, who's the fault if bitcoind won't build with Qt7? Things can break in upgrades..

The current setup is engineered so that the client has a chance to enable deprecated features while they are there and meanwhile plan to update clients.

This is a shame. I thought the original request (allow deprecatedrpc to be passed as a JSONRPC argument) was a reasonable suggestion. I would suggest that if implemented, it should allow passing it as a JSONRPC argument as well as the option to specify it as a command-line argument. That way the deprecated RPC can be enabled either per-client or globally.

Sadly this thread has been hijacked for yet another "freeze the public API forever" rant.

Honestly, the more I think about it, the more I think the current setup is over-engineered. I really think the best solution would be a sort of WARN_ONCE for deprecated methods, write to a deprecated.log

deprecatedrpc is a way to allow users to use both the old API and the new API in a single bitcoin core release. It gives users at least 6 months to update the client software to support the new API. I'm very happy to have the conversation about whether there's a more user-friendly way to support the old API, but both of your suggestions (per-client JSON RPC option and stateful tracking of whether a client has called a deprecated API) both require more engineering than a simple global toggle.

the better solution is removing the whole deprecated flags entirely

If you don't like the deprecatedrpc flag, then don't use it! You're going to have to upgrade your client to use the new API at some point (unless you chose to stay on an old version of Bitcoin Core forever). deprecatedrpc simply gives you one additional warning release before you have to upgrade your client.

Sadly this thread has been hijacked for yet another "freeze the public API forever" rant.

If you want such a rant, you should read this from Linus Torvalds 😄