wallet: rm unused LegacyScriptPubKeyMan& spk_man #17338

pull jonatack wants to merge 1 commits into bitcoin:master from jonatack:rm-unused-spk-man-in-rpcdump changing 1 files +0 −10

jonatack commented at 12:36 PM on November 1, 2019: contributor

Follow-up to #17300 "LegacyScriptPubKeyMan code cleanups" and #17300 (comment).

wallet: rm unused LegacyScriptPubKeyMan& spk_man

Follow-up to #17300 "LegacyScriptPubKeyMan code cleanups".

074405ad1d

fanquake added the label Wallet on Nov 1, 2019
jonatack commented at 12:37 PM on November 1, 2019: contributor

Not sure if these should be removed or documented.
ryanofsky commented at 1:54 PM on November 1, 2019: contributor

This PR as of 074405ad1ded63d1ba83518f458fde61cfd220f5 isn't really doing the right thing. The rpcdump methods are supposed to trigger "This type of wallet does not support this command" errors when called on descriptor wallets where CWallet::GetLegacyScriptPubKeyMan returns null (in the future after 4d0e6f063e34d3869f89d410ccdc4b6255a00150 from #17261).

To clean this up I would rename the rpcdump function GetLegacyScriptPubKeyMan to EnsureLegacyScriptPubKeyMan and keep calling it even when the returned reference is unused, just deleting the unused rpc_man variables, but not the function calls.

Also, for more cleanup I would move EnsureLegacyScriptPubKeyMan to rpcwallet.h / rpcwallet.cpp and start calling it from the other two rpc methods there that throw "This type of wallet does not support this command" errors.
jonatack commented at 2:20 PM on November 1, 2019: contributor

Thank you @ryanofsky for your feedback. Will do and look into PR #17261.
laanwj commented at 10:11 AM on November 2, 2019: member

This PR as of 074405a isn't really doing the right thing.

FWIW this is exactly what I warned about in #17300 (comment) :smile: But yes, might be better to just wait for @ryanofsky's follow up on this.
jonatack commented at 10:20 AM on November 2, 2019: contributor

Heh, I suspected as much. Closing rather than following up, then.
jonatack closed this on Nov 2, 2019
jonatack deleted the branch on Nov 2, 2019
Sjors cross-referenced this on Nov 2, 2019 from issue refactor: Move many functions into LegacyScriptPubKeyMan and further separate it from CWallet by achow101
practicalswift cross-referenced this on Nov 5, 2019 from issue wallet: remove unused variable spk_man in import* RPCs by theStack
ryanofsky added this to the "PRs" column in a project

ryanofsky removed this from the "PRs" column in a project

bitcoin locked this on Dec 16, 2021

Contributors

Labels

Linked (view graph)

#17261 Make ScriptPubKeyMan an actual interface and the wallet to have multiple #17300 LegacyScriptPubKeyMan code cleanups #17304 refactor: Move many functions into LegacyScriptPubKeyMan and further separate it from CWallet #17377 wallet: remove unused variable spk_man in import* RPCs