Have the wallet give out destinations instead of keys #16237
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, 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. |
src/wallet/wallet.h
Outdated
There was a problem hiding this comment.
2010b1a13e6b7eaa681cc62ae58079ccbec72816
This should be atomic or add EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) annotation?
Also, could make GetKeyFromPool private?
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
Concept ACK. Will review more closely, but this seems like a nice, straightforward change.
achow101
force-pushed
the
cwallet-getnewaddr
branch
from
7d014e4 to
835ae12
Compare
src/interfaces/wallet.cpp
Outdated
There was a problem hiding this comment.
interfaces/wallet.cpp:146:16: error: calling function 'GetNewAddress' requires holding mutex 'm_wallet->cs_wallet' exclusively [-Werror,-Wthread-safety-analysis]
835ae12 to
89df834
Compare
src/wallet/wallet.cpp
Outdated
There was a problem hiding this comment.
nit: the message is translated here, but not in the new occurences. As this looks like a message that will be returned over the RPC interface (referring to a RPC command, at least). Maybe better to not translate it here either?
There was a problem hiding this comment.
I've made it untranslated.
|
Concept ACK |
|
Concept ACK. In line with @laanwj's comment, maybe rename
Travis seems unhappy about a locking issue. On macOS I also get some warnings: |
achow101
force-pushed
the
cwallet-getnewaddr
branch
2 times, most recently
from
47a7c3f to
5b771f3
Compare
I've renamed it.
It is?
Should be fixed now. |
achow101
force-pushed
the
cwallet-getnewaddr
branch
from
5b771f3 to
3a58f6d
Compare
achow101
force-pushed
the
cwallet-getnewaddr
branch
from
3a58f6d to
c84cbd5
Compare
achow101
changed the title
|
utACK c84cbd5 |
Instead of having the same multiple lines of code everywhere that new destinations are fetched, introduce GetNewDestination as a member function of CWallet which does the key fetching, label setting, script generation, and destination generation.
Instead of reserving keys, reserve destinations which are backed by keys
Adds a GetNewChangeDestination that has the same objective as GetNewDestination
achow101
force-pushed
the
cwallet-getnewaddr
branch
from
c84cbd5 to
8e7f930
Compare
|
re-utACK 8e7f930 only changes are clearing error messages at beginning of |
|
ACK 8e7f930. Concept ACK as this gives a much cleaner abstraction to work with, and light code review ACK. It's bit confusing that constructing a (filled) ReserveDestination does not actually require a destination type, preventing the interface from being able to select where to draw change from based on the address type desired. Perhaps something for a follow-up? |
|
ACK 8e7f930 |
8e7f930 Add GetNewChangeDestination for getting new change Destinations (Andrew Chow) 33d13ed Replace CReserveKey with ReserveDestinatoin (Andrew Chow) 172213b Add GetNewDestination to CWallet to fetch new destinations (Andrew Chow) Pull request description: The wallet should give out destinations instead of keys. It should be the one that handles the conversion from key to destination and the setting of the label, not the caller. In order to do this, two new member functions are introduced `GetNewDestination()` and `GetNewChangeDestination()`. Additionally, `CReserveKey` is changed to be `ReserveDestination` and represents destinations whose keys can be returned to the keypool. ACKs for top commit: instagibbs: re-utACK 8e7f930 sipa: ACK 8e7f930. Concept ACK as this gives a much cleaner abstraction to work with, and light code review ACK. laanwj: ACK 8e7f930 Tree-SHA512: 5be7051409232b71e0ef2c1fd1a3e76964ed2f5b14d47d06edc2ad3b3687abd0be2803a1adc45c0433aa2c3bed172e14f8a7e9f4a23bff70f86260b5a0497500
|
The macOS warnings I saw before are indeed gone now. |
… keys 8e7f930 Add GetNewChangeDestination for getting new change Destinations (Andrew Chow) 33d13ed Replace CReserveKey with ReserveDestinatoin (Andrew Chow) 172213b Add GetNewDestination to CWallet to fetch new destinations (Andrew Chow) Pull request description: The wallet should give out destinations instead of keys. It should be the one that handles the conversion from key to destination and the setting of the label, not the caller. In order to do this, two new member functions are introduced `GetNewDestination()` and `GetNewChangeDestination()`. Additionally, `CReserveKey` is changed to be `ReserveDestination` and represents destinations whose keys can be returned to the keypool. ACKs for top commit: instagibbs: re-utACK bitcoin@8e7f930 sipa: ACK 8e7f930. Concept ACK as this gives a much cleaner abstraction to work with, and light code review ACK. laanwj: ACK 8e7f930 Tree-SHA512: 5be7051409232b71e0ef2c1fd1a3e76964ed2f5b14d47d06edc2ad3b3687abd0be2803a1adc45c0433aa2c3bed172e14f8a7e9f4a23bff70f86260b5a0497500
…stinations Summary: Instead of having the same multiple lines of code everywhere that new destinations are fetched, introduce GetNewDestination as a member function of CWallet which does the key fetching, label setting, script generation, and destination generation. bitcoin/bitcoin@172213b --- Depends on D6714 Partial backport of Core [[bitcoin/bitcoin#16237 | PR16237]] Test Plan: ninja check-all Reviewers: #bitcoin_abc, jasonbcox, deadalnix Reviewed By: #bitcoin_abc, jasonbcox, deadalnix Subscribers: deadalnix, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D6716
Summary: Instead of reserving keys, reserve destinations which are backed by keys bitcoin/bitcoin@33d13ed --- Depends on D6716 Partial backport of Core [[bitcoin/bitcoin#16237 | PR16237]] Test Plan: ninja check-all Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6717
…ge Destinations Summary: Adds a GetNewChangeDestination that has the same objective as GetNewDestination bitcoin/bitcoin@8e7f930 --- Depends on D6717 Concludes backport of Core [[bitcoin/bitcoin#16237 | PR16237]] Test Plan: ninja check-all Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6721
10 participants
The wallet should give out destinations instead of keys. It should be the one that handles the conversion from key to destination and the setting of the label, not the caller. In order to do this, two new member functions are introduced
GetNewDestination()andGetNewChangeDestination(). Additionally,CReserveKeyis changed to beReserveDestinationand represents destinations whose keys can be returned to the keypool.