Skip to content

LegacyScriptPubKeyMan code cleanups #17300

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
merged 5 commits into from
Oct 31, 2019
Merged

LegacyScriptPubKeyMan code cleanups #17300

merged 5 commits into from
Oct 31, 2019

Conversation

ryanofsky
Copy link
Contributor

This PR implements suggested code cleanups from #17260 review comments

@ryanofsky
Refactor: Add GetLegacyScriptPubKeyMan helper
52cf68f 
Suggested by João Barbosa <joao.paulo.barbosa@gmail.com>
bitcoin#17260 (comment)
@ryanofsky
Add back mistakenly removed AssertLockHeld
628d11b 
Suggestion from MarcoFalke <falke.marco@gmail.com>
bitcoin#17260 (comment)
@ryanofsky
doc: Clarify WalletStorage / Wallet relation
2632b1f 
Suggested by MarcoFalke <falke.marco@gmail.com>
bitcoin#17260 (comment)
@ryanofsky
Fix misplaced AssertLockHeld
4b28a05 
Suggestion from MarcoFalke <falke.marco@gmail.com>
bitcoin#17260 (comment)
@ryanofsky
Fix missing strFailReason in CreateTransaction
53fe0b7 
Suggested by MarcoFalke <falke.marco@gmail.com>
bitcoin#17260 (comment)
@maflcko
Copy link
Member

maflcko commented Oct 29, 2019

In commit aa0e6d8:

My comment was just an example. It is generally not clear to me when this nullptr check should be done. Most of the other methods don't do it either. E.g. CWallet::LoadWallet, CWallet::GetNewChangeDestination, CWallet::CreateWalletFromFile, ...

@ryanofsky
Copy link
Contributor Author

It is generally not clear to me when this nullptr check should be done.

Checking for null key managers should always be done, and various commits in #17261 add more null checks. But I don't think it makes a big difference if we add null checks now (there should only be a small handful) or wait for changes from #17261. Just let me know what you prefer: dropping this commit, keeping it, or expanding it to cover LoadWallet, etc.

Note m_spk_man will never actually be null until commit "Refactor: Allow LegacyScriptPubKeyMan to be null" 66d2e22 from #17261

Later after that, m_spk_man is replaced with lists of key managers in commit "Box the wallet: Add multiple keyman maps and loops" 00b7802 from #17261 and all the if (auto spk_man = m_spk_man.get()) blocks turn into for loops.

@Sjors
Copy link
Member

Sjors commented Oct 29, 2019

It is generally not clear to me when this nullptr check should be done.

I like how GetLegacyScriptPubKeyMan helper returns a reference instead of a pointer.

Just let me know what you prefer: dropping this commit, keeping it, or expanding it to cover LoadWallet, etc.

I prefer expanding it.

@maflcko
Copy link
Member

maflcko commented Oct 29, 2019

Either drop it or expand it. It shouldn't matter

@ryanofsky
Copy link
Contributor Author

re: #17300 (comment) from Sjors

I prefer expanding it.

re: #17300 (comment) from MarcoFalke

Either drop it or expand it. It shouldn't matter

I looked into expanding this but some of the changes in places like CreateWallet and AddToWalletIfInvolvingMe are awkward and less-obvious than I'd want to make in a bulk commit. I think the existing individual commits in #17261 which are broken down by functionality add the null checking in a more understandable way than could be done here. So I just dropped commit aa0e6d8. Another advantage of not expanding this is it will keep #17300 from conflicting with #17261 too much.

@Sjors Sjors mentioned this pull request Oct 29, 2019
Merged
2 tasks
@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 29, 2019 •
edited
Loading

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #17261 (Make ScriptPubKeyMan an actual interface and the wallet to have multiple by achow101)
  • #16895 (External signer multisig support by Sjors)
  • #16549 ([WIP] UI external signer support (e.g. hardware wallet) by Sjors)
  • #16546 ([WIP] External signer support - Wallet Box edition by Sjors)

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.

@maflcko
Copy link
Member

maflcko commented Oct 30, 2019 •
edited
Loading

Approach ACK, haven't looked at the diff in-depth

@Sjors
Copy link
Member

Sjors commented Oct 31, 2019

ACK 53fe0b7

1 similar comment
@achow101
Copy link
Member

ACK 53fe0b7

@maflcko
Copy link
Member

maflcko commented Oct 31, 2019

ACK 53fe0b7

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 53fe0b70adeffe4cb94e6fa18a9abbdf674a2cd0
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUg83wwAgwAEPoOoUUuaq/slVcJkmBK2g/zAdTLvLCsBV0PNQiI5v9QN1DcAbFgW
vTWN0+u2VaehZGhJoqw5riAOdXrY+MQrcn01Ljx4b49550F1w0Dbf4BLu41yQ/qj
56sZzYxSyZ0x5E6sogazCOhwKWZ6KEyht3aZR2YPx7y8Gp1BNF53Whmb0LVSsm9C
xIKg3Z+/aP3FzBLL3VD39lhxSZjjw0yxTqlZPNgEcHK7xQvn7uf+VkhIVoXdNcSf
kPgljx5KscWrsMAP/i8NViZ8k86n5QRlcO/iQIZbIyWk2wywzQpq/SBf8uxF+V9Y
SNiEnTwSnvLl4IyLwLcHio58S7+PXgQtV3L1wgGDhsXD/ZNRuiwAnshrnlECPMe+
3ZgE82cPznKEj/b22lo/Jyt/XMlLkrcEiuFDw9Z+eqMO1c/+6UxGoPX/E/YFPv0M
GPK3fen95qdbmwknlSoALUUbnJQvS5QkAwWug0Bm6C+zMy5eBwp2l84bOT69CMa7
IYipWyK3
=8EPP
-----END PGP SIGNATURE-----

Timestamp of file with hash f41dcbac528b83e896b46a19b8757a3b59f8e6f343c9e7f4830f5b6cb65d028e -

maflcko pushed a commit that referenced this pull request Oct 31, 2019
Merge #17300: LegacyScriptPubKeyMan code cleanups
100fa0a 
53fe0b7 Fix missing strFailReason in CreateTransaction (Russell Yanofsky)
4b28a05 Fix misplaced AssertLockHeld (Russell Yanofsky)
2632b1f doc: Clarify WalletStorage / Wallet relation (Russell Yanofsky)
628d11b Add back mistakenly removed AssertLockHeld (Russell Yanofsky)
52cf68f Refactor: Add GetLegacyScriptPubKeyMan helper (Russell Yanofsky)

Pull request description:

  This PR implements suggested code cleanups from #17260 review comments

ACKs for top commit:
  Sjors:
    ACK 53fe0b7
  achow101:
    ACK 53fe0b7
  MarcoFalke:
    ACK 53fe0b7

Tree-SHA512: a577b96cb21a9aa7185d7d900e4db0665c302adcd12097957b9d8e838a8548c7de8f901bcb83e7c46d231b841221345c9264f5e29ed069f3d9236896430f959b
@maflcko maflcko merged commit 53fe0b7 into bitcoin:master Oct 31, 2019
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 31, 2019
Merge bitcoin#17300: LegacyScriptPubKeyMan code cleanups
5273f88 
53fe0b7 Fix missing strFailReason in CreateTransaction (Russell Yanofsky)
4b28a05 Fix misplaced AssertLockHeld (Russell Yanofsky)
2632b1f doc: Clarify WalletStorage / Wallet relation (Russell Yanofsky)
628d11b Add back mistakenly removed AssertLockHeld (Russell Yanofsky)
52cf68f Refactor: Add GetLegacyScriptPubKeyMan helper (Russell Yanofsky)

Pull request description:

  This PR implements suggested code cleanups from bitcoin#17260 review comments

ACKs for top commit:
  Sjors:
    ACK 53fe0b7
  achow101:
    ACK 53fe0b7
  MarcoFalke:
    ACK 53fe0b7

Tree-SHA512: a577b96cb21a9aa7185d7d900e4db0665c302adcd12097957b9d8e838a8548c7de8f901bcb83e7c46d231b841221345c9264f5e29ed069f3d9236896430f959b
@laanwj
Copy link
Member

laanwj commented Nov 1, 2019

This introduces 5 "warning: unused variable 'spk_man' [-Wunused-variable]" warnings in rpcdump.

Probably harmless: though if the function should be called if the return value is not used (so it's purely side effect) it might be good to document that so people don't do the obvious thing.

jonatack added a commit to jonatack/bitcoin that referenced this pull request Nov 1, 2019
@jonatack
wallet: rm unused LegacyScriptPubKeyMan& spk_man
074405a 
Follow-up to bitcoin#17300 "LegacyScriptPubKeyMan code cleanups".
@jonatack jonatack mentioned this pull request Nov 1, 2019
Closed
@ryanofsky
Copy link
Contributor Author

ryanofsky commented Nov 1, 2019 •
edited
Loading

This introduces 5 "warning: unused variable 'spk_man' [-Wunused-variable]" warnings in rpcdump.

Probably harmless: though if the function should be called if the return value is not used (so it's purely side effect) it might be good to document that so people don't do the obvious thing.

Good catch. I'm planning to make another cleanup PR after #17304 (which could be close to ready for merge) for more unaddressed review comments. I could fix the warning in that PR, or open a more limited PR sooner if the warnings are too annoying. Some of the unused spk_man variables in the warnings are used later in 07e0c23 from #17261 for locking cs_keystore, but it's unclear if these locks are actually necessary, and in any case it's confusing to have the unused variables. It might also help to rename GetLegacyScriptPubKeyMan to EnsureLegacyScriptPubKeyMan to be clear about what the function is doing when the return value is unused.

@ryanofsky ryanofsky mentioned this pull request Nov 4, 2019
Merged
@jonatack jonatack mentioned this pull request Nov 5, 2019
Closed
@ryanofsky ryanofsky mentioned this pull request Nov 5, 2019
Merged
laanwj added a commit that referenced this pull request Nov 6, 2019
@laanwj
Merge #17381: LegacyScriptPubKeyMan code cleanups
976cc76 
05b224a Add missing SetupGeneration error handling in EncryptWallet (Russell Yanofsky)
bfd826a Clean up nested scope in GetReservedDestination (Russell Yanofsky)
491a599 Get rid of confusing LegacyScriptPubKeyMan::TopUpKeyPool method (Russell Yanofsky)
4a0abf6 Pass CTxDestination to ScriptPubKeyMan::GetMetadata (Russell Yanofsky)
b07b07c Add EnsureLegacyScriptPubKeyMan and use in rpcwallet.cpp (Russell Yanofsky)

Pull request description:

  This PR implements suggested code cleanups from #17300 and #17304 review comments

ACKs for top commit:
  Sjors:
    re-ACK 05b224a
  laanwj:
    Code review ACK 05b224a

Tree-SHA512: 12fd86637088515b744c028e0501c5d21a9cf9ee9c9cfd70e9cb65d44611ea5643abd5f6f101105caa5aff015d74de606f074f08af7dae8429f929d21288ab45
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 7, 2019
@laanwj
Merge bitcoin#17381: LegacyScriptPubKeyMan code cleanups
19238f8 
05b224a Add missing SetupGeneration error handling in EncryptWallet (Russell Yanofsky)
bfd826a Clean up nested scope in GetReservedDestination (Russell Yanofsky)
491a599 Get rid of confusing LegacyScriptPubKeyMan::TopUpKeyPool method (Russell Yanofsky)
4a0abf6 Pass CTxDestination to ScriptPubKeyMan::GetMetadata (Russell Yanofsky)
b07b07c Add EnsureLegacyScriptPubKeyMan and use in rpcwallet.cpp (Russell Yanofsky)

Pull request description:

  This PR implements suggested code cleanups from bitcoin#17300 and bitcoin#17304 review comments

ACKs for top commit:
  Sjors:
    re-ACK 05b224a
  laanwj:
    Code review ACK 05b224a

Tree-SHA512: 12fd86637088515b744c028e0501c5d21a9cf9ee9c9cfd70e9cb65d44611ea5643abd5f6f101105caa5aff015d74de606f074f08af7dae8429f929d21288ab45
@promag promag mentioned this pull request Jan 14, 2020
Merged
@MarkLTZ MarkLTZ mentioned this pull request Apr 4, 2020
Closed
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Aug 6, 2020
@ryanofsky @majcosta
[backport#17300] LegacyScriptPubKeyMan code cleanups
089fb67 
Summary:
  Fix missing strFailReason in CreateTransaction (Russell Yanofsky)
  Fix misplaced AssertLockHeld (Russell Yanofsky)
  doc: Clarify WalletStorage / Wallet relation (Russell Yanofsky)
  Add back mistakenly removed AssertLockHeld (Russell Yanofsky)
  Refactor: Add GetLegacyScriptPubKeyMan helper (Russell Yanofsky)

Pull request description:

  This PR implements suggested code cleanups from bitcoin/bitcoin#17260 review comments

---

https://github.com/bitcoin/bitcoin/pull/17300/files

Depends on D7119

Backport of Core [[bitcoin/bitcoin#17300 | PR17300]]

Test Plan:
  ninja check check-functional

Reviewers: #bitcoin_abc, jasonbcox

Reviewed By: #bitcoin_abc, jasonbcox

Differential Revision: https://reviews.bitcoinabc.org/D7128
sidhujag pushed a commit to syscoin-core/syscoin that referenced this pull request Nov 10, 2020
Merge bitcoin#17300: LegacyScriptPubKeyMan code cleanups
97e495b 
53fe0b7 Fix missing strFailReason in CreateTransaction (Russell Yanofsky)
4b28a05 Fix misplaced AssertLockHeld (Russell Yanofsky)
2632b1f doc: Clarify WalletStorage / Wallet relation (Russell Yanofsky)
628d11b Add back mistakenly removed AssertLockHeld (Russell Yanofsky)
52cf68f Refactor: Add GetLegacyScriptPubKeyMan helper (Russell Yanofsky)

Pull request description:

  This PR implements suggested code cleanups from bitcoin#17260 review comments

ACKs for top commit:
  Sjors:
    ACK 53fe0b7
  achow101:
    ACK 53fe0b7
  MarcoFalke:
    ACK 53fe0b7

Tree-SHA512: a577b96cb21a9aa7185d7d900e4db0665c302adcd12097957b9d8e838a8548c7de8f901bcb83e7c46d231b841221345c9264f5e29ed069f3d9236896430f959b
sidhujag pushed a commit to syscoin-core/syscoin that referenced this pull request Nov 10, 2020
@laanwj
Merge bitcoin#17381: LegacyScriptPubKeyMan code cleanups
682a477 
05b224a Add missing SetupGeneration error handling in EncryptWallet (Russell Yanofsky)
bfd826a Clean up nested scope in GetReservedDestination (Russell Yanofsky)
491a599 Get rid of confusing LegacyScriptPubKeyMan::TopUpKeyPool method (Russell Yanofsky)
4a0abf6 Pass CTxDestination to ScriptPubKeyMan::GetMetadata (Russell Yanofsky)
b07b07c Add EnsureLegacyScriptPubKeyMan and use in rpcwallet.cpp (Russell Yanofsky)

Pull request description:

  This PR implements suggested code cleanups from bitcoin#17300 and bitcoin#17304 review comments

ACKs for top commit:
  Sjors:
    re-ACK 05b224a
  laanwj:
    Code review ACK 05b224a

Tree-SHA512: 12fd86637088515b744c028e0501c5d21a9cf9ee9c9cfd70e9cb65d44611ea5643abd5f6f101105caa5aff015d74de606f074f08af7dae8429f929d21288ab45
@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
No reviews
Assignees
No one assigned
Labels
Refactoring Wallet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ryanofsky @maflcko @Sjors @DrahtBot @achow101 @laanwj