Move RPC registration out of AppInitParameterInteraction

ryanofsky commented at 8:39 pm on November 3, 2017: member

Move to AppInitServers. This doesn’t have any effects on bitcoin behavior. It was just strange to have this unrelated code in the middle of parameter interaction.

Move to AppInitServers. This doesn't have any effects on bitcoin behavior. It
was just strange to have this unrelated code in the middle or parameter
interaction.

abbd230217

in src/init.cpp:712 in ccb2a53d9b outdated

708@@ -709,6 +709,10 @@ bool InitSanityCheck(void)
709 
710 bool AppInitServers(boost::thread_group& threadGroup)
711 {
712+    RegisterAllCoreRPCCommands(tableRPC);

promag commented at 9:51 pm on November 3, 2017:

Nit, could be in OnRPCStarted, like there is no need to register if it fails to start? Edit: at the moment it never fails to start, see StartRPC().

promag commented at 9:54 pm on November 3, 2017: member

Of course ACK.

TheBlueMatt commented at 10:04 pm on November 3, 2017: member

Does this break using the in-Qt RPC console when running without -server?

fanquake added the label Refactoring on Nov 4, 2017

promag commented at 0:42 am on November 6, 2017: member

@TheBlueMatt is right, with this change executing help in the Qt console gives:

0== Control ==
1help ( "command" )
2stop
3uptime

laanwj commented at 9:07 am on November 6, 2017: member

Does this break using the in-Qt RPC console when running without -server?

Whops - it appears the logic is wrong there currently. Qt debug console is considered a front-end for RPC, just like libevent/HTTP. So StartRPC is definitely supposed to be called when Qt’s debug console is active. Same for the OnRPCStarted OnRPCStopped registrations in InitServers, probably. It’s just the HTTP and REST stuff which can be left out if -server is not provided.

Edit: Concept ACK on moving it out of InitParameterInteraction anyhow

ryanofsky force-pushed on Nov 7, 2017

ryanofsky commented at 1:31 am on November 7, 2017: member

Updated ccb2a53d9bf0858e963161b2fcaa76639457b410 -> abbd230217df2f3b0cc25527f3c15f6f11b9852e (pr/rpcinit.1 -> pr/rpcinit.2) to fix the problem with qt.

Added a test for the qt bug in #11625. @laanwj I can also go ahead and make the StartRPC() call not depend on -server, but that’s a little more awkward than it might otherwise be because StartRPC is sandwiched between InitHTTPServer and StartHTTPRPC calls. Would it be ok to move it above? Also it seems like the http server is not started at all if -rest is disabled. Is that intentional?

laanwj commented at 7:13 am on November 7, 2017: member

Would it be ok to move it above?

Yes - constraint is that StartRPC needs to be called before StartHTTPRPC - the relative order of InitHTTPServer and StartRPC shouldn’t matter because the parts don’t ‘know’ each other at that point.

Also it seems like the http server is not started at all if -rest is disabled. Is that intentional?

I’m fairly sure that’s not true in master, we’d have plenty of reports, as -rest is disabled by default.

in src/init.cpp:1240 in abbd230217

1235@@ -1241,6 +1236,14 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler)
1236 
1237     GetMainSignals().RegisterBackgroundSignalScheduler(scheduler);
1238 
1239+    /* Register RPC commands regardless of -server setting so they will be
1240+     * available in the GUI RPC console even if external calls are disabled.

promag commented at 0:58 am on November 9, 2017:

Only relevant in bitcoin-qt? No need to register if bitcoind without server.

laanwj commented at 8:20 am on November 9, 2017:

That’s very rarely used. It doesn’t hurt to register the commands anyway. If that helps keep the code simpler (esp. we don’t want UI specific workarounds in core code).

promag commented at 8:23 am on November 9, 2017:

Yeah agree.

laanwj commented at 9:10 pm on November 23, 2017: member

Tested ACK abbd230217df2f3b0cc25527f3c15f6f11b9852e

laanwj merged this on Nov 23, 2017

laanwj closed this on Nov 23, 2017

laanwj referenced this in commit a933cb14c7 on Nov 23, 2017

TheBlueMatt commented at 3:33 pm on November 27, 2017: member

Post-merge utACK abbd230217df2f3b0cc25527f3c15f6f11b9852e

MarcoFalke referenced this in commit fdbe7f41c0 on Jan 9, 2019

PastaPastaPasta referenced this in commit 9b359e406c on Jan 17, 2020

PastaPastaPasta referenced this in commit 63ebd6d1d7 on Jan 22, 2020

PastaPastaPasta referenced this in commit 7a5add1662 on Jan 22, 2020

PastaPastaPasta referenced this in commit 2b4f3e4be0 on Jan 29, 2020

PastaPastaPasta referenced this in commit 6579003dfa on Jan 29, 2020

PastaPastaPasta referenced this in commit baadb52aef on Jan 29, 2020

ckti referenced this in commit 84d0a3cd33 on Mar 28, 2021

furszy referenced this in commit 60d36292bc on Jul 26, 2021

MarcoFalke locked this on Sep 8, 2021

Move RPC registration out of AppInitParameterInteraction #11603