LegacyScriptPubKeyMan code cleanups

ryanofsky commented at 4:39 pm on October 29, 2019: member

This PR implements suggested code cleanups from #17260 review comments

Refactor: Add GetLegacyScriptPubKeyMan helper

Suggested by João Barbosa <joao.paulo.barbosa@gmail.com>
https://github.com/bitcoin/bitcoin/pull/17260#discussion_r339505236

52cf68f7ff

Add back mistakenly removed AssertLockHeld

Suggestion from MarcoFalke <falke.marco@gmail.com>
https://github.com/bitcoin/bitcoin/pull/17260#discussion_r340029481

628d11b2ba

doc: Clarify WalletStorage / Wallet relation

Suggested by MarcoFalke <falke.marco@gmail.com>
https://github.com/bitcoin/bitcoin/pull/17260#discussion_r340031507

2632b1f124

Fix misplaced AssertLockHeld

Suggestion from MarcoFalke <falke.marco@gmail.com>
https://github.com/bitcoin/bitcoin/pull/17260#discussion_r340033021

4b28a05f08

Fix missing strFailReason in CreateTransaction

Suggested by MarcoFalke <falke.marco@gmail.com>
https://github.com/bitcoin/bitcoin/pull/17260#discussion_r340036269

53fe0b70ad

MarcoFalke added the label Wallet on Oct 29, 2019

MarcoFalke added the label Refactoring on Oct 29, 2019

MarcoFalke commented at 4:48 pm on October 29, 2019: member

In commit aa0e6d8ab1761d6bbc729614663da7ad7fb67dac:

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 commented at 5:15 pm on October 29, 2019: member

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” 66d2e2206267695f2fc722481252f63f7c74636a 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” 00b7802570970d6328bc566b9978f93be0f18f66 from #17261 and all the if (auto spk_man = m_spk_man.get()) blocks turn into for loops.

Sjors commented at 5:18 pm on October 29, 2019: member

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.

MarcoFalke commented at 5:32 pm on October 29, 2019: member

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

ryanofsky force-pushed on Oct 29, 2019

ryanofsky commented at 6:00 pm on October 29, 2019: member

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 aa0e6d8ab1761d6bbc729614663da7ad7fb67dac. Another advantage of not expanding this is it will keep #17300 from conflicting with #17261 too much.

DrahtBot commented at 8:11 pm on October 29, 2019: member

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.

MarcoFalke commented at 1:16 am on October 30, 2019: member

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

Sjors commented at 4:23 pm on October 31, 2019: member

ACK 53fe0b70adeffe4cb94e6fa18a9abbdf674a2cd0

achow101 commented at 5:50 pm on October 31, 2019: member

ACK 53fe0b70adeffe4cb94e6fa18a9abbdf674a2cd0

MarcoFalke commented at 6:40 pm on October 31, 2019: member

ACK 53fe0b70adeffe4cb94e6fa18a9abbdf674a2cd0

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 53fe0b70adeffe4cb94e6fa18a9abbdf674a2cd0
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUg83wwAgwAEPoOoUUuaq/slVcJkmBK2g/zAdTLvLCsBV0PNQiI5v9QN1DcAbFgW
 8vTWN0+u2VaehZGhJoqw5riAOdXrY+MQrcn01Ljx4b49550F1w0Dbf4BLu41yQ/qj
 956sZzYxSyZ0x5E6sogazCOhwKWZ6KEyht3aZR2YPx7y8Gp1BNF53Whmb0LVSsm9C
10xIKg3Z+/aP3FzBLL3VD39lhxSZjjw0yxTqlZPNgEcHK7xQvn7uf+VkhIVoXdNcSf
11kPgljx5KscWrsMAP/i8NViZ8k86n5QRlcO/iQIZbIyWk2wywzQpq/SBf8uxF+V9Y
12SNiEnTwSnvLl4IyLwLcHio58S7+PXgQtV3L1wgGDhsXD/ZNRuiwAnshrnlECPMe+
133ZgE82cPznKEj/b22lo/Jyt/XMlLkrcEiuFDw9Z+eqMO1c/+6UxGoPX/E/YFPv0M
14GPK3fen95qdbmwknlSoALUUbnJQvS5QkAwWug0Bm6C+zMy5eBwp2l84bOT69CMa7
15IYipWyK3
16=8EPP
17-----END PGP SIGNATURE-----

Timestamp of file with hash f41dcbac528b83e896b46a19b8757a3b59f8e6f343c9e7f4830f5b6cb65d028e -

MarcoFalke referenced this in commit 100fa0a62a on Oct 31, 2019

MarcoFalke merged this on Oct 31, 2019

MarcoFalke closed this on Oct 31, 2019

sidhujag referenced this in commit 5273f889bb on Oct 31, 2019

laanwj commented at 10:52 am on November 1, 2019: member

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 referenced this in commit 074405ad1d on Nov 1, 2019

ryanofsky commented at 1:09 pm on November 1, 2019: member

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 07e0c2369353623a289b5b92da1f13f6d2be1ace 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 added this to the "PRs" column in a project

fanquake moved this from the "PRs" to the "Done" column in a project

laanwj referenced this in commit 976cc766c4 on Nov 6, 2019

sidhujag referenced this in commit 19238f8e6c on Nov 7, 2019

jasonbcox referenced this in commit 089fb67955 on Aug 6, 2020

sidhujag referenced this in commit 97e495b1a0 on Nov 10, 2020

sidhujag referenced this in commit 682a4772a0 on Nov 10, 2020

DrahtBot locked this on Dec 16, 2021

LegacyScriptPubKeyMan code cleanups #17300

Conflicts