ENABLE_WALLET hides !wallet features, misses others #3406

Reviewing the reqWallet field of the RPC call table reveals ENABLE_WALLET bugs.

RPC 'validateaddress' denotes reqWallet==false in the RPC call table, but it is disabled with --disable-wallet at compile time. Ditto 'createmultisig', 'verifymessage', getgenerate, setgenerate, ...

'settxfee' has reqWallet==true, but is not covered by #ifdef.

reqWallet was a handy, carefully audited reference. Use it!

I've erred on the safe side. None of the functions in the table with ENABLE_WALLET make use of the pWalletMain object. I've checked this by looking at link-time references to the actual wallet, which is arguably even more fool proof than blindly relying on that row (which I did look at).

• settxfee doesn't require the wallet. You might argue that it is pointless without wallet functionality though.

On the other hand, other functions use the wallet 'under the hood':

• setgenerate and getgenerate do require the wallet, they are part of the "wallet enabled mining" functions. At least setgenerate does an internal check for the wallet, and getgenerate is kind of pointless without it.

You're correct that three calls that could be included are missing:

• validateaddress can optionally use the wallet (and was excluded because it happened to be in rpcwallet.cpp).
• verifymessage was excluded because it happened to be in rpcwallet.cpp
• creatmultisig was excluded because it happened to be in rpcwallet.cpp

I could move these to another rpcXXX file and enable them. But I thought people using bitcoin as P2P node only wouldn't be too interested in these.

I vote for allowing all calls that are possible with a disabled wallet.

For verifymessage and createmultisig there is no argument against. But let me give some background, --disable-wallet is part of a project of me trying to modularize bitcoind.

The first (and currently only) module is to be the current wallet, which provides certain RPC functions related to the wallet. I already have code in another branch that registers RPC calls dynamically and gets rid of all but one of the #ifdef ENABLE_WALLETs sprinkled throughout init.cpp and some of rpc*.cpp.

validateaddress has different functionality depending on whether a wallet is used or not. With a wallet it checks for validity and whether the address is in the wallet or not, without wallet it checks only for validity. This complicates modularization.

• The most straightforward way to handle this would be to have a base implementation of validateaddress that is overridden when the wallet module is used.
• Another way would be a module entry point AddressIsMine or something like that, that is called from a common validateaddress implementation.

In any case, it's a complicating factor.

Ok my suggestion for the short-term fix would be:

• move verifymessage, createmultisig, validateaddress as well as getinfo (which is currently in rpcnet.cpp where it doesn't really belong) to a new implementation file rpcmisc.cpp as they don't belong in any of the other rpc*.cpp.
• then remove them from the #ifdef ENABLE_WALLET in rpcserver.cpp
• add a #ifdef ENABLE_WALLET to validateaddress around the wallet-using part, so it also works when compiling with --disable-wallet
• move settxfee to rpcwallet.cpp and move it into the #ifdef ENABLE_WALLET in rpcserver.cpp

RE setgenerate and getgenerate: not true. Look closely, they do "work" without a wallet, in that, they provide a sane return value in the absence of a wallet, rather than simply being missing.

ACK your proposed short-term fix, @laanwj

setgenerate simply raises an exception without wallet, just as if it doesn't exist:

if (pwalletMain == NULL)
    throw JSONRPCError(RPC_METHOD_NOT_FOUND, "Method not found (disabled)");

getgenerate returns a value, sure, but that value is always 0. I don't see any point in including them in non-wallet builds.

See #3412

I have a solution for the modular case as well. The Module Interface (to replace the ui_interface) already needs a few entry points so that modules can add information to the getinfo, getmining RPC calls

GetInfo(Object &obj)
GetMiningInfo(Object &obj)

So let's just add one for validateaddress too

GetDestinationInfo(Object &obj, const CTxDestination *dest)

getgenerate/setgenerate/gethashespersec/getwork would ideally belong in a different module internalminer which requires a wallet (or at least, some interface to pull new public keys from), not in the wallet module itself.