Skip to content
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

[rpc] Remove the addresses field from the getaddressinfo return object #15750

Merged

Conversation

Projects
None yet
10 participants
@jnewbery
Copy link
Member

commented Apr 4, 2019

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.

@promag

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

utACK bf58f83.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15639 (bitcoin-wallet tool: Drop libbitcoin_server.a dependency by ryanofsky)
  • #15452 (Replace CScriptID and CKeyID in CTxDestination with dedicated types by instagibbs)

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.

@promag

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

@leto care to elaborate on the down vote?

@laanwj

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

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.

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2019

This was effectively deprecated here: b98bfc5#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: 3eaa003#diff-ad6efdc354b57bd1fa29fc3abb6e2872R43

@sipa

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

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?

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2019

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: b98bfc5#diff-ad6efdc354b57bd1fa29fc3abb6e2872R45, which was included in V0.17.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

Could update the release notes just to be safe? No strong opinion, though.

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2019

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.

@leto

This comment has been minimized.

Copy link

commented Apr 10, 2019

@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 👎 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.

@jnewbery jnewbery force-pushed the jnewbery:2019_04_remove_address_from_getaddressinfo branch from bf58f83 to ff3f152 Apr 10, 2019

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2019

@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:

  • Don't upgrade and continue using the V0.17.x version
  • Maintain a fork of the software that meets your particular needs
  • Write a shim that sits in front of the RPC server to maintain compatibility.
[rpc] Remove the addresses field from the getaddressinfo return object
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.

@laanwj laanwj added this to the 0.18.0 milestone Apr 11, 2019

@jnewbery jnewbery force-pushed the jnewbery:2019_04_remove_address_from_getaddressinfo branch from ff3f152 to b4338c1 Apr 11, 2019

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2019

Rebased on master

@jonatack
Copy link
Contributor

left a comment

ACK b4338c1. Tests gist.

jonatack added a commit to jonatack/bitcoin that referenced this pull request Apr 14, 2019

test: Add RPC getaddressinfo functional test
as a regression test for bitcoin#15750.

This file can serve as a base for adding further getaddressinfo tests/sanity checks that are not tested elsewhere.
@jonatack

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2019

If helpful, here is a functional test for this change. Feel free to use/adapt/ignore.

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2019

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)

@MarcoFalke MarcoFalke merged commit b4338c1 into bitcoin:master Apr 15, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Apr 15, 2019

Merge bitcoin#15750: [rpc] Remove the addresses field from the getadd…
…ressinfo return object

b4338c1 [rpc] Remove the addresses field from the getaddressinfo return object (John Newbery)

Pull request description:

  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.

ACKs for commit b4338c:
  jonatack:
    ACK bitcoin@b4338c1. Tests [gist](https://gist.github.com/jonatack/31915e290bb1be39b9769dc9357385ca).

Tree-SHA512: 2c207510e565df600428838bfc6db5211fa06aaace365e31cbd74f1d2376b598675cb90df2fc1440858d49b22095aaa9d6b9ce3de0aff22417fe72cc6a6a321f

@jnewbery jnewbery deleted the jnewbery:2019_04_remove_address_from_getaddressinfo branch Apr 15, 2019

@jonatack

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

Thanks @jnewbery. I agree the test case does not need to be merged in. It was a sanity check to support my ACK and (maybe) help merge the PR, which I'm glad is in!

MarcoFalke added a commit that referenced this pull request Apr 15, 2019

Merge #15800: Backport: [rpc] Remove the addresses field from the get…
…addressinfo return object

b3a04c9 [rpc] Remove the addresses field from the getaddressinfo return object (John Newbery)

Pull request description:

  Backport #15750

ACKs for commit b3a04c:
  MarcoFalke:
    ACK b3a04c9 (checked cherry-pick)

Tree-SHA512: d6e586fb6e22b9825267d6c45dd3090b7f8dc1a06804ca238103d1c665b178f8c2b3004f67aae67ea51c2c3df97561894f3f2e85d45400c760c3b7f057ff4bb8
@fanquake

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

Backported in #15800.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.