Move RPC registration out of AppInitParameterInteraction #11603

ryanofsky · 2017-11-03T20:39:36Z

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.

promag

Of course ACK.

promag · 2017-11-03T21:51:02Z

src/init.cpp


 bool AppInitServers(boost::thread_group& threadGroup)
 {
+    RegisterAllCoreRPCCommands(tableRPC);


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().

TheBlueMatt · 2017-11-03T22:04:06Z

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

promag · 2017-11-06T00:42:05Z

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

== Control ==
help ( "command" )
stop
uptime

laanwj · 2017-11-06T09:07:34Z

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 · 2017-11-07T01:31:45Z

Updated ccb2a53 -> abbd230 (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 · 2017-11-07T07:13:52Z

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.

promag · 2017-11-09T00:58:51Z

src/init.cpp

    GetMainSignals().RegisterBackgroundSignalScheduler(scheduler);

+    /* Register RPC commands regardless of -server setting so they will be
+     * available in the GUI RPC console even if external calls are disabled.


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

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).

Yeah agree.

laanwj · 2017-11-23T21:10:14Z

Tested ACK abbd230

abbd230 Move RPC registration out of AppInitParameterInteraction (Russell Yanofsky) Pull request description: 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. Tree-SHA512: 373e18f2ef8d21999ad36295d69326128a3086044acfc8ed537abd5497c8d3620b9832f7f6aa87c0c0939bb5e0d92be8a3c006b5997e9e6fa20334f5610c89bc

TheBlueMatt · 2017-11-27T15:33:55Z

Post-merge utACK abbd230

7e4bd19 Add BitcoinApplication & RPCConsole tests (Russell Yanofsky) ca20b65 Move BitcoinApplication to header so it can be tested (Russell Yanofsky) Pull request description: Add test coverage for Qt initialization code & basic RPC console functionality Motivation for this change was a bug in #11603 which existing tests failed to catch. Tree-SHA512: f66546ffc84b8e07679c66a73b265023fbf6a0cb8f24f1606a5fcae2dd3b4dc7b2c6d26c69dedcec53398a26ef17c4d5fb28c055698fa6e45e89aa2995cefe2f

…eraction abbd230 Move RPC registration out of AppInitParameterInteraction (Russell Yanofsky) Pull request description: 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. Tree-SHA512: 373e18f2ef8d21999ad36295d69326128a3086044acfc8ed537abd5497c8d3620b9832f7f6aa87c0c0939bb5e0d92be8a3c006b5997e9e6fa20334f5610c89bc

7a5d181 Use the character based overload for std::string::find. (Alin Rus) c19401b Don't use pass by reference to const for cheaply-copied types (bool, char, etc.). (practicalswift) 4c5fe36 [Refactor] Remove unused fQuit var from checkqueue.h (donaloconnor) fda7a5f Cleanup: remove unneeded header includes (random-zebra) ac2476c Add test cases covering the relevant key length boundaries: 64 bytes +/- 1 byte for HMAC-SHA256 and 128 bytes +/- 1 byte for HMAC-SHA512 (practicalswift) a346262 Fix code constness in CBlockIndex::GetAncestor() overloads (Dan Raviv) 65e3f4e Refactor: More range-based for loops (random-zebra) dd3d3c4 Use range-based for loops (C++11) when looping over map elements (practicalswift) 5a7750a Move RPC registration out of AppInitParameterInteraction (Russell Yanofsky) 402b4c4 Use compile-time constants instead of unnamed enumerations (practicalswift) bbac2d0 [Trivial] Fix indentation in coins.cpp (random-zebra) e539c62 Small refactor of CCoinsViewCache::BatchWrite() (Dan Raviv) ec91759 Use MakeUnique<T>(...) instead of std::unique_ptr<T>(new T(...)) (random-zebra) 93487b1 Use unique_ptr for pcoinscatcher/pcoinsdbview/pcoinsTip/pblocktree (random-zebra) ff43d69 Use unique_ptr for pdbCopy (Db) and fix potential memory leak (practicalswift) b4d9641 Use unique_ptr for dbenv (DbEnv) (practicalswift) 36108b9 Use unique_ptr for pfilter (CBloomFilter) (practicalswift) ff1c454 Use unique_ptr for sem{Addnode,Outbound} (CSemaphore) (practicalswift) 93daf17 Use unique_ptr for httpRPCTimerInterface (HTTPRPCTimerInterface) (practicalswift) 020ac16 Init: Remove redundant exit(EXIT_FAILURE) instances and replace with (random-zebra) b9f5d1f Remove duplicate uriParts.size() > 0 check (practicalswift) 440d961 Remove redundant check (!ecc is always true) (practicalswift) bfd295b Remove redundant NULL checks after new (practicalswift) 97aad32 Make fUseCrypto atomic (MeshCollider) 2711f78 Consistent parameter names in txdb.h (MeshCollider) d40df3a Fix race for mapBlockIndex in AppInitMain (random-zebra) 03b7766 Cleanup: remove unused functions to Hash the concat of 4 or more objects (random-zebra) c520e0f Remove some unused functions and methods (Pieter Wuille) 508f1a1 range-based loops and const qualifications in net.cpp (Marko Bencun) 79b1e50 Refactor: implement CPubKey::data() (random-zebra) 614d26c Refactor: more &vec[0] to vec.data() (random-zebra) 02b6337 Changing &vec[0] to vec.data(), what 9804 missed (MeshCollider) c1c8b05 Ensure that data types are consistent (jjz) 732bb9d Fix potential null dereferences (MeshCollider) 80f35f9 Remove unreachable code (practicalswift) Pull request description: This is a collection of simple refactorings coming from upstream Bitcoin v0.16 (adapting/extending to PIVX-specific code where needed). Pull requests backported: - bitcoin#10845 (practicalswift) - bitcoin#11238 (MeshCollider) - bitcoin#11232 (jjz) - bitcoin#10793 (MeshCollider) - bitcoin#10888 (benma) - ~~bitcoin#11351 (danra)~~ [edit: removed - included in #2423] - bitcoin#11385 (sipa) - bitcoin#11107 (MeshCollider) - bitcoin#10898 (practicalswift) - bitcoin#11511 (donaloconnor) - bitcoin#11043 (practicalswift) - bitcoin#11353 (danra) - bitcoin#10749 (practicalswift) - bitcoin#11603 (ryanofsky) - bitcoin#10493 (practicalswift) - bitcoin#11337 (danra) - bitcoin#11516 (practicalswift) - bitcoin#10574 (practicalswift) - bitcoin#12108 (donaloconnor) - bitcoin#10839 (practicalswift) - bitcoin#12159 (kekimusmaximus) ACKs for top commit: Fuzzbawls: ACK 7a5d181 furszy: re-ACK 7a5d181 after rebase, no code changes. Merging.. Tree-SHA512: d92f5df47f443391a6531274a2efb9a4882c62d32eff628f795b3abce703f108c8b40ec80ac841cde6c5fdd5c9ff2b6056a31546ac2edda279f5f18fccc99c32

Move RPC registration out of AppInitParameterInteraction

abbd230

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.

promag reviewed Nov 3, 2017

View reviewed changes

fanquake added the Refactoring label Nov 4, 2017

ryanofsky mentioned this pull request Nov 7, 2017

Add BitcoinApplication & RPCConsole tests #11625

Merged

ryanofsky force-pushed the pr/rpcinit branch from ccb2a53 to abbd230 Compare November 7, 2017 01:23

promag reviewed Nov 9, 2017

View reviewed changes

laanwj merged commit abbd230 into bitcoin:master Nov 23, 2017

random-zebra mentioned this pull request Jun 29, 2021

[Refactoring] Bitcoin 0.16 cherries PIVX-Project/PIVX#2462

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Move RPC registration out of AppInitParameterInteraction #11603

Move RPC registration out of AppInitParameterInteraction #11603

Uh oh!

Conversation

ryanofsky commented Nov 3, 2017

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Nov 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Nov 3, 2017

Uh oh!

promag commented Nov 6, 2017

Uh oh!

laanwj commented Nov 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Nov 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Nov 7, 2017

Uh oh!

promag Nov 9, 2017

Choose a reason for hiding this comment

Uh oh!

laanwj Nov 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag Nov 9, 2017

Choose a reason for hiding this comment

Uh oh!

laanwj commented Nov 23, 2017

Uh oh!

TheBlueMatt commented Nov 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

promag Nov 3, 2017 •

edited

Loading

laanwj commented Nov 6, 2017 •

edited

Loading

ryanofsky commented Nov 7, 2017 •

edited

Loading

laanwj Nov 9, 2017 •

edited

Loading