Skip to content

wallet: replace raw pointer with const reference in AddrToPubKey #17584

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.

Sign up for GitHub

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

Jump to bottom
Merged
meshcollider merged 1 commit into from
Nov 26, 2019
Merged

wallet: replace raw pointer with const reference in AddrToPubKey #17584

meshcollider merged 1 commit into from
Nov 26, 2019

Conversation

@brakmic
Copy link
Contributor

@brakmic brakmic commented Nov 24, 2019

This PR replaces a redundant reference-to-pointer conversion in addmultisigaddress from wallet/rpcwallet.cpp. It also makes the API from rpc/util.h look more straightforward as AddrToPubKey now uses const references like other functions from there.

I am not sure why there is a ref-to-ptr conversion in addmultisignatures, so I can only speculate that this is because of "historical reasons".

The ref-to-ptr conversion happens here: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/rpcwallet.cpp#L1001

There, the address of LegacyScriptPubKeyMan& is given to AddrToPubKey.

Later, in AddrToPubKey, it gets converted back to a reference, because GetKeyForDestination in rpc/util.cpp expects a const ref: https://github.com/bitcoin/bitcoin/blob/master/src/rpc/util.cpp#L140

Regards,

@practicalswift
Copy link
Contributor

practicalswift commented Nov 25, 2019

Concept ACK

@instagibbs
Copy link
Member

instagibbs commented Nov 25, 2019

Any idea why it was a pointer in the first place? Always happy to get rid of pointers.

concept ACK

@vasild
Copy link
Contributor

vasild commented Nov 25, 2019

The pointer argument was introduced in 1df206f. Maybe @achow101 remembers?

At that time the keystore argument was used in two expressions inside AddrToPubKey(): GetKeyForDestination(*keystore, dest) and keystore->GetPubKey(). The first takes a const reference arg and in the second GetPubKey() method is marked as const (even back then). So it cannot have been that keystore was a pointer (to non-const!) only to be able to use it inside AddrToPubKey().

However at that time all callers of GetKeyForDestination() used a dereferenced pointer:

1df206f854:src/rpc/misc.cpp:229:            CKeyID key_id = GetKeyForDestination(*pwallet, dest);
1df206f854:src/rpc/util.cpp:33:    CKeyID key = GetKeyForDestination(*keystore, dest);
1df206f854:src/wallet/rpcdump.cpp:606:    auto keyid = GetKeyForDestination(*pwallet, dest);

So maybe it was due to some code mimicing?

Other callers of GetKeyForDestination() have switched to using a const reference, so this change should be welcome.

Concept ACK

@achow101
Copy link
Member

achow101 commented Nov 25, 2019 •
edited
Loading

So maybe it was due to some code mimicing?

More like code copying.

Concept ACK

promag
promag reviewed Nov 25, 2019
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK 1a3a256.

hebasto
hebasto approved these changes Nov 25, 2019
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 1a3a256, I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged.

@achow101
Copy link
Member

achow101 commented Nov 25, 2019

ACK 1a3a256

@meshcollider
Copy link
Contributor

meshcollider commented Nov 26, 2019

utACK 1a3a256

meshcollider added a commit that referenced this pull request Nov 26, 2019
@meshcollider
Merge #17584: wallet: replace raw pointer with const reference in Add…
0ee914b 
…rToPubKey

1a3a256 wallet: replace raw pointer with const reference in AddrToPubKey (Harris)

Pull request description:

  This PR replaces a redundant reference-to-pointer conversion in **addmultisigaddress** from *wallet/rpcwallet.cpp*. It also makes the API from *rpc/util.h* look more straightforward as **AddrToPubKey** now uses const references like other functions from there.

  I am not sure why there is a ref-to-ptr conversion in addmultisignatures, so I can only speculate that this is because of "historical reasons".

  The ref-to-ptr conversion happens here: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/rpcwallet.cpp#L1001

  There, the address of LegacyScriptPubKeyMan& is given to AddrToPubKey.

  Later, in AddrToPubKey, it gets converted back to a reference, because GetKeyForDestination in rpc/util.cpp expects a const ref: https://github.com/bitcoin/bitcoin/blob/master/src/rpc/util.cpp#L140

  Regards,

ACKs for top commit:
  achow101:
    ACK 1a3a256
  meshcollider:
    utACK 1a3a256
  promag:
    Code review ACK 1a3a256.
  hebasto:
    ACK 1a3a256, I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged.

Tree-SHA512: 1a2b8ddab5694ef4c65fac69f011e38dd03a634e84a35857e13bd05ad99fe42af22ee0af6230865e3d2c725693512f3336acb055ede19c958424283e7a3856c4
@meshcollider meshcollider merged commit 1a3a256 into bitcoin:master Nov 26, 2019
@brakmic brakmic deleted the rpcwallet-redundant-pointer-conversion branch November 26, 2019 08:28
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 27, 2019
@meshcollider
Merge bitcoin#17584: wallet: replace raw pointer with const reference…
3815259 
… in AddrToPubKey

1a3a256 wallet: replace raw pointer with const reference in AddrToPubKey (Harris)

Pull request description:

  This PR replaces a redundant reference-to-pointer conversion in **addmultisigaddress** from *wallet/rpcwallet.cpp*. It also makes the API from *rpc/util.h* look more straightforward as **AddrToPubKey** now uses const references like other functions from there.

  I am not sure why there is a ref-to-ptr conversion in addmultisignatures, so I can only speculate that this is because of "historical reasons".

  The ref-to-ptr conversion happens here: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/rpcwallet.cpp#L1001

  There, the address of LegacyScriptPubKeyMan& is given to AddrToPubKey.

  Later, in AddrToPubKey, it gets converted back to a reference, because GetKeyForDestination in rpc/util.cpp expects a const ref: https://github.com/bitcoin/bitcoin/blob/master/src/rpc/util.cpp#L140

  Regards,

ACKs for top commit:
  achow101:
    ACK 1a3a256
  meshcollider:
    utACK 1a3a256
  promag:
    Code review ACK 1a3a256.
  hebasto:
    ACK 1a3a256, I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged.

Tree-SHA512: 1a2b8ddab5694ef4c65fac69f011e38dd03a634e84a35857e13bd05ad99fe42af22ee0af6230865e3d2c725693512f3336acb055ede19c958424283e7a3856c4
@MarkLTZ MarkLTZ mentioned this pull request Apr 4, 2020
Closed
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 1, 2020
@meshcollider @majcosta
[backport#17584] wallet: replace raw pointer with const reference in …
be62d94 
…AddrToPubKey

Summary:
1a3a256d5e0443d19757c1f1fceb9c9ede17758a wallet: replace raw pointer with const reference in AddrToPubKey (Harris)

Pull request description:

  This PR replaces a redundant reference-to-pointer conversion in **addmultisigaddress** from *wallet/rpcwallet.cpp*. It also makes the API from *rpc/util.h* look more straightforward as **AddrToPubKey** now uses const references like other functions from there.

  I am not sure why there is a ref-to-ptr conversion in addmultisignatures, so I can only speculate that this is because of "historical reasons".

  The ref-to-ptr conversion happens here: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/rpcwallet.cpp#L1001

  There, the address of LegacyScriptPubKeyMan& is given to AddrToPubKey.

  Later, in AddrToPubKey, it gets converted back to a reference, because GetKeyForDestination in rpc/util.cpp expects a const ref: https://github.com/bitcoin/bitcoin/blob/master/src/rpc/util.cpp#L140

  Regards,

---

Backport of Core [[bitcoin/bitcoin#17584 | PR17584]]

Test Plan:
  ninja all check check-functional

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D7715
sidhujag pushed a commit to syscoin-core/syscoin that referenced this pull request Nov 10, 2020
@meshcollider
Merge bitcoin#17584: wallet: replace raw pointer with const reference…
f9fcb62 
… in AddrToPubKey

1a3a256 wallet: replace raw pointer with const reference in AddrToPubKey (Harris)

Pull request description:

  This PR replaces a redundant reference-to-pointer conversion in **addmultisigaddress** from *wallet/rpcwallet.cpp*. It also makes the API from *rpc/util.h* look more straightforward as **AddrToPubKey** now uses const references like other functions from there.

  I am not sure why there is a ref-to-ptr conversion in addmultisignatures, so I can only speculate that this is because of "historical reasons".

  The ref-to-ptr conversion happens here: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/rpcwallet.cpp#L1001

  There, the address of LegacyScriptPubKeyMan& is given to AddrToPubKey.

  Later, in AddrToPubKey, it gets converted back to a reference, because GetKeyForDestination in rpc/util.cpp expects a const ref: https://github.com/bitcoin/bitcoin/blob/master/src/rpc/util.cpp#L140

  Regards,

ACKs for top commit:
  achow101:
    ACK 1a3a256
  meshcollider:
    utACK 1a3a256
  promag:
    Code review ACK 1a3a256.
  hebasto:
    ACK 1a3a256, I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged.

Tree-SHA512: 1a2b8ddab5694ef4c65fac69f011e38dd03a634e84a35857e13bd05ad99fe42af22ee0af6230865e3d2c725693512f3336acb055ede19c958424283e7a3856c4
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@hebasto hebasto hebasto approved these changes

+1 more reviewer

@promag promag promag left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Refactoring Wallet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@brakmic @practicalswift @instagibbs @vasild @achow101 @meshcollider @promag @hebasto @fanquake @maflcko