The "addresses" field was confusing because it refered to public keys using their P2PKH address. It was included in the return object when needed for backward compatibility. Remove that compatibility now that the -deprecatedrpc=validateaddress option has been removed.
New applications should use the 'embedded'->'address' field for P2SH or P2WSH wrapped addresses, and 'pubkeys' for inspecting multisig participants.
utACK bf58f83.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
3439 | // TODO: abstract out the common functionality between this logic and ExtractDestinations.
3440 | obj.pushKV("sigsrequired", solutions_data[0][0]);
3441 | UniValue pubkeys(UniValue::VARR);
3442 | for (size_t i = 1; i < solutions_data.size() - 1; ++i) {
3443 | CPubKey key(solutions_data[i].begin(), solutions_data[i].end());
3444 | - if (include_addresses) a.push_back(EncodeDestination(key.GetID()));
UniValue a(UniValue::VARR); on L3423 is no longer use after this removal? Could be dropped :-)
done! Thanks
Might want to add a deprecation notice—as well as what to use for replacement—to the documentation of getaddressinfo, I expect this to be more often read than a code comment.
This was effectively deprecated here: https://github.com/bitcoin/bitcoin/commit/b98bfc5ed0da1efef1eff552a7e1a7ce9caf130f#diff-df7d84ff2f53fcb2a0dc15a3a51e55ceR3587 and since that commit, the addresses field has only been shown when -deprecatedrpc=validateaddress was set.
I believe this functionality should have been removed with #12490
Would appreciate input from @leto if he thinks there is a good reason not to remove this, and @sipa since he added the include_addresses parameter here: https://github.com/bitcoin/bitcoin/commit/3eaa003c888f80207e8ff132f78417ff373ddfa3#diff-ad6efdc354b57bd1fa29fc3abb6e2872R43
I will be very happy to see the addresses field disappear (it's a semi regular question on bitcoin.se asking how an output can have multiple addresses...).
I don't have an opinion on deprecation schedule, so we can keep it around if there are uses for it, but it's been explained in the RPC help and deprecated for a major release already, right?
I don't have an opinion on deprecation schedule, so we can keep it around if there are uses for it, but it's been explained in the RPC help and deprecated for a major release already, right?
Yes, deprecation warning was given here: https://github.com/bitcoin/bitcoin/commit/b98bfc5ed0da1efef1eff552a7e1a7ce9caf130f#diff-ad6efdc354b57bd1fa29fc3abb6e2872R45, which was included in V0.17.
Could update the release notes just to be safe? No strong opinion, though.
Could update the release notes just to be safe? No strong opinion, though.
If there's going to be another v0.18 rc, we should include this PR and update https://github.com/bitcoin-core/bitcoin-devwiki/wiki/0.18.0-Release-Notes-Draft#deprecated-or-removed-rpcs. This functionality should have been removed at the same time as #12490.
@promag I help maintain software that heavily uses the BTC RPC interface and from the perspective of a 3rd party developer integrating with Bitcoin Core and wanting to support the latest versions, it's frustrating that RPCs keep getting removed and changed. validateaddress is used extensively and so breaking the backcompat on getaddressinfo because it's "confusing" does not seem like a great reason. In the Perl 5 Core world, we do anything we can to ensure backward compatibility, which means you can run 99.9% of programs from before Bitcoin was invented on the latest Perl 5 release.
My :-1: was to show that some "consistency" changes might look nice from inside the walls of Core, but they have many negative effects to 3rd parties that just want to have a stable RPC API.
I don't expect this to change minds, but hopefully it helps inform future decisions of supporting backward compatibility.
@leto - thanks for your input. I understand your point about maintaining compatibility for clients, and it's definitely something that the contributors take into consideration. That needs to be weighed against other considerations like making the RPCs consistent and not confusing for users. The addresses field in this RPC return object was confusing. How can an address have multiple addresses?
The reason these RPC methods have been modified in recent versions is justified in #10583. Originally, validateaddress accessed data and functionality in both the wallet and node. Splitting it into getaddressinfo and validateaddress allow us to move forward with properly modularising the codebase. Not making that change would preclude very useful changes such as #10973, which make the code more maintainable, allowing us to more quickly and safely make changes to the code that benefit all users.
We try to make the transition as painless as possible for clients by marking features as deprecated for at least one release before removing them. Anyone still using the addresses field in v0.17 would have to have set the deprecatedrpc=validateaddress option, and so would know that the field would be removed shortly.
Finally, Bitcoin Core is an open source project, so if you really do rely on the addresses field (I can't see how that's possible since the data is available in other fields), you have several options:
The "addresses" field was confusing because it refered to public keys
using their P2PKH address. It was included in the return object when
needed for backward compatibility. Remove that compatibility now that
the -deprecatedrpc=validateaddress option has been removed.
New applications should use the 'embedded'->'address' field for P2SH or
P2WSH wrapped addresses, and 'pubkeys' for inspecting multisig
participants.
Rebased on master
If helpful, here is a functional test for this change. Feel free to use/adapt/ignore.
Thanks @jonatack . Writing tests is a very helpful way of contributing. In this PR, I don't think a test case is necessary. This PR removes any code that would create an addresses field in the return object, and I think adding test code for code that doesn't exist isn't necessary.
(If this PR was putting code behind a deprecation flag, then it would definitely deserve a test case. See https://github.com/bitcoin/bitcoin/blob/2a191b48463adc64dfb6187fccb7742c795dee60/test/functional/rpc_deprecated.py for example)
