RPC: cleanups in rpc/server

jtimon commented at 11:25 pm on February 23, 2017: contributor

There’s no reason to declare the following functions in rpc/server.h when they’re defined in wallet/rpcwallet.cpp. They can be declared in wallet/rpcwallet.h instead:

HelpRequiringPassphrase EnsureWalletIsAvailable EnsureWalletIsUnlocked

Also, there’s no reason to use extern for functions declared in rpc/server.h. It is really never needed (see http://stackoverflow.com/questions/11712707/extern-functions-in-c-vs-c/11712820#11712820 ), but it seems more strange when those functions are also defined in rpc/server.cpp (in the same module).

fanquake added the label RPC/REST/ZMQ on Feb 24, 2017

sipa commented at 3:28 am on February 24, 2017: member

utACK

dcousens approved

kallewoof commented at 4:53 am on February 24, 2017: member

utACK 073613b

jonasschnelli approved

jonasschnelli commented at 7:16 am on February 24, 2017: contributor

utACK 073613b36a8780dd720e2a17f1314980173e200d

paveljanik commented at 9:34 am on February 24, 2017: contributor

utACK 073613b

laanwj commented at 9:50 am on February 24, 2017: member

Good idea, rpcserver is supposed to be bitcoin-agnostic, those methods don’t belong there.

jtimon commented at 11:25 pm on February 24, 2017: contributor

There’s also GetDifficulty (declared in rpc/server.h, defined in rpc/blockchain.cpp, used in several rpc places) which isn’t taken care of here. I wasn’t sure what to do with it.

NicolasDorier commented at 5:31 am on February 28, 2017: contributor

utACK 073613b36a8780dd720e2a17f1314980173e200d

MarcoFalke commented at 10:54 am on February 28, 2017: member

utACK 073613b36a8780dd720e2a17f1314980173e200d

in src/rpc/server.h: in 073613b36a outdated

192+uint256 ParseHashV(const UniValue& v, std::string strName);
193+uint256 ParseHashO(const UniValue& o, std::string strKey);
194+std::vector<unsigned char> ParseHexV(const UniValue& v, std::string strName);
195+std::vector<unsigned char> ParseHexO(const UniValue& o, std::string strKey);
196 
197 extern int64_t nWalletUnlockTime;

jnewbery commented at 3:00 am on March 2, 2017:

Can you also move nWalletUnlockTime to rpcwallet.h as an extern declaration? You’ll need to #include wallet/rpcwallet.h in rpc/misc.cpp, protected by #ifdef ENABLE_WALLET

laanwj commented at 10:43 am on March 2, 2017:

Good catch. Agreed.

jnewbery approved

jnewbery commented at 3:05 am on March 2, 2017: member

tested ACK. Looks good.

There’s also GetDifficulty (declared in rpc/server.h, defined in rpc/blockchain.cpp, used in several rpc places) which isn’t taken care of here

Could that become a member function of CBlockIndex in chain.cpp?

laanwj commented at 1:30 pm on March 5, 2017: member

Needs rebase (and last nit-fix)

laanwj assigned laanwj on Mar 5, 2017

RPC: Trivial: Remove unneeded 'extern' in functions declarations in rpc/server.h 24b2c40d2f

jtimon force-pushed on Mar 6, 2017

jtimon commented at 9:58 pm on March 6, 2017: contributor

Rebased. Rebasing makes the first commit unnecessary after #8775 “a4356328 Move wallet RPC declarations to rpcwallet.h” . It seems nWalletUnlockTime was moved in #8775 too (see “2e518e31 Move nWalletUnlockTime to CWallet::nRelockTime, and name timed task unique per CWallet” ). This leaves the PR to a single commit removing the unnecessary ’extern’ in functions, trivial to review.

MarcoFalke commented at 10:18 pm on March 6, 2017: member

removing the unnecessary ’extern’ in functions

I think there are other functions with ’extern’. You might as well do it for the whole code base.

On Mon, Mar 6, 2017 at 10:59 PM, Jorge Timón notifications@github.com wrote:

Rebased. Rebasing makes the first commit unnecessary after #8775 https://github.com/bitcoin/bitcoin/pull/8775 “a4356328 Move wallet RPC declarations to rpcwallet.h” . It seems nWalletUnlockTime was moved in #8775 https://github.com/bitcoin/bitcoin/pull/8775 too (see “2e518e31 Move nWalletUnlockTime to CWallet::nRelockTime, and name timed task unique per CWallet” ). This leaves the PR to a single commit removing the unnecessary ’extern’ in functions, trivial to review.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/9845#issuecomment-284547039, or mute the thread https://github.com/notifications/unsubscribe-auth/AGGmv_Wl53kJR1JBq0nGpahZfM9Q7dgzks5rjIG1gaJpZM4MKol3 .

jtimon commented at 10:39 pm on March 6, 2017: contributor

@MarcoFalke I guess at that point I should create another PR since the tittle “RPC: cleanups in rpc/server” wouldn’t fit anymore. I’m not planning to do that in the short term, but you are right. I would happily ACK such a PR to replace this one if anybody does it. I was just working on rpcwallet and rpc/server and saw easy things to fix without knowing about #8775 . I may end up doing it myself if nobody else jumps at it, but for now I’ll just leave this PR open as it is.

laanwj commented at 10:07 am on March 7, 2017: member

I think there are other functions with ’extern’. You might as well do it for the whole code base.

For whoever does that: do add instruction in developer-notes.md on when to use extern and when not to. It seems safe to add extern to these cases, just redundant, so I’d see this as quite low priority.

in src/rpc/server.h: in 24b2c40d2f

200+std::vector<unsigned char> ParseHexV(const UniValue& v, std::string strName);
201+std::vector<unsigned char> ParseHexO(const UniValue& o, std::string strKey);
202+
203+CAmount AmountFromValue(const UniValue& value);
204+UniValue ValueFromAmount(const CAmount& amount);
205+double GetDifficulty(const CBlockIndex* blockindex = NULL);

laanwj commented at 6:24 am on March 8, 2017:

Why is GetDifficulty still here? :-)

jtimon commented at 1:59 pm on March 21, 2017: contributor

extern should never be used for functions, but I’m not sure that is worth to add “please don’t add extern to functions because it doesn’t do anything” to the developer notes, at least not more than “please don’t add variables that aren’t used”. Agreed removing extern is low priority, as said, the most interesting parts of this PR were already done by #8775

Should we merge this anyway (since it’s trivial, has been reviewed and it’s not disruptive) or should I just close it?

jtimon commented at 8:34 pm on March 23, 2017: contributor

Closing, it seems at this point would be preferable to remove extern from all functions from the project at once (I bet they’re not that many, but I didn’t looked yet) as @MarcoFalke suggests. In some way, all utACKs for this were just partial acks for merged #8775

jtimon closed this on Mar 23, 2017

laanwj commented at 7:19 am on March 24, 2017: member

I think removing GetDifficulty from server.h would have made a lot of sense here, along with the other cleanups as those lines were touched anyway, but indeed it’s very low priority.

jtimon commented at 1:02 pm on March 25, 2017: contributor

Well, as said I wasn’t sure what to do with GetDifficulty, so this PR never moved it.

laanwj commented at 12:32 pm on March 27, 2017: member

No need to be clever here, if it’s defined in src/rpc/blockchain.cpp the interface should be declared in src/rpc/blockchain.h. Will file a PR, see #10095.

DrahtBot locked this on Sep 8, 2021

RPC: cleanups in rpc/server #9845