wallet: remove unused variable spk_man in import* RPCs

[theStack]

Each of the following RPC functions

• importprivkey
• importaddress
• importpubkey
• importwallet
• importmulti

define a variable reference LegacyScriptPubKeyMan& spk_man which is not used, leading to compiler warnings in the current master branch (bdda137):

  CXX      wallet/libbitcoin_wallet_a-rpcdump.o
wallet/rpcdump.cpp:137:28: warning: unused variable 'spk_man' [-Wunused-variable]
    LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*wallet);
                           ^
wallet/rpcdump.cpp:265:28: warning: unused variable 'spk_man' [-Wunused-variable]
    LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*pwallet);
                           ^
wallet/rpcdump.cpp:468:28: warning: unused variable 'spk_man' [-Wunused-variable]
    LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*wallet);
                           ^
wallet/rpcdump.cpp:552:28: warning: unused variable 'spk_man' [-Wunused-variable]
    LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*wallet);
                           ^
wallet/rpcdump.cpp:1349:28: warning: unused variable 'spk_man' [-Wunused-variable]
    LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*wallet);
                           ^
5 warnings generated.

The call to GetLegacyScriptPubKeyMan() still serves the purpose to throw an error if the wallet doesn't have a LegacyScriptPubKeyMan instance (indicating that the command is not supported, as pointed out by the error message):

bitcoin/src/wallet/rpcdump.cpp

Lines 90 to 97 in bdda137

	static LegacyScriptPubKeyMan& GetLegacyScriptPubKeyMan(CWallet& wallet)
	{
	LegacyScriptPubKeyMan* spk_man = wallet.GetLegacyScriptPubKeyMan();
	if (!spk_man) {
	throw JSONRPCError(RPC_WALLET_ERROR, "This type of wallet does not support this command");
	}
	return *spk_man;
	}

This commit casts the functions return value to void to point out that the value is thrown away, and adds a comment on why the function call is needed as well. introduces a new function RequireLegacyScriptPubKeyMan() which in turn calls GetLegacyScriptPubKeyMan() but discards its return value.

[practicalswift]

@theStack Some context is found in #17338 :)

[jonatack]

See #17300 (comment).

[jonatack]

@practicalswift Snap! :)

[theStack]

@practicalswift @jonatack Oops, I overlooked that. Mental note to myself: also search through merged/closed PRs about a change that is likely to having been discussed already, not only currently open ones. Thanks for pointing out!

[Sjors]

From the earlier comment:

Some of the unused spk_man variables in the warnings are used later in 07e0c23 from #17261 for locking cs_keystore

You could add a function void RequireLegacyScriptPubKeyMan() that only serves to check the presence. That in turn can call GetLegacyScriptPubKeyMan(). Later commits can then switch RequireLegacyScriptPubKeyMan() to GetLegacyScriptPubKeyMan() if they need the return value.

cc @achow101 @ryanofsky

[theStack]

@Sjors: Good idea, I updated the commit and the PR description accordingly.

If someone else wants to work on this who has more background knowledge about future use etc., probably on the course of a larger cleanup PR (@ryanofsky, @achow101 ?), feel free to close it though -- I wouldn't have opened it in the first place if I knew about #17338 and #17300 (comment).

[Sjors]

ACK 606422b (works, but prefer ryanofsky's solution) ./configure -Werror=unused-variable is happy again. @MarcoFalke didn't we have a Travis instance with that config at some point?

[ryanofsky]

Sorry for causing these warnings. Feel free to pull in changes from b07b07c in #17381 to avoid conflicts with that PR. I think the changes in that commit are a little better than current changes here (606422b), because they extend the getter function to cover rpcwallet as well as rpcdump, and use Ensure naming consistent with EnsureWalletIsAvailable and EnsureWalletIsUnlocked. They implement what I previously suggested to jonatack in #17338 (comment)

[Sjors]

Agree @ryanofsky's version is more thorough, so let's go with that.

[fanquake]

Closing in favour of #17381.

[laanwj]

Whoops. We should probably merge something to get rid of these warnings to avoid people wasting their time creating PR after PR fixing them.