Add "warmup mode" for RPC server. #5007

pull domob1812 wants to merge 1 commits into bitcoin:master from domob1812:btc-clientwarmup changing 6 files +110 −32
  1. domob1812 commented at 9:54 AM on September 30, 2014: contributor

    Start the RPC server before doing all the (expensive) startup initialisations like loading the block index. Until the node is ready, return all calls immediately with a new error signalling "in warmup" with an appropriate status message (similar to the init message).

    This is useful for RPC clients to know that the server is there (e. g., they don't have to start it) but not yet available. It is used in Namecoin and Huntercoin already for some time, and there exists a UI hooked onto the RPC interface that actively uses this to its advantage.

  2. domob1812 commented at 10:52 AM on September 30, 2014: contributor

    I guess this "breaks" -rpcwait. bitcoin-cli -rpcwait should treat the new warmup error like a connection error. I will modify the code accordingly.

  3. BitcoinPullTester commented at 12:10 PM on September 30, 2014: none

    Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/p5007_0ba552b68d7db8aeb9280beca7f80e777f76aefb/ for binaries and test log.

    This could happen for one of several reasons:

    1. It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts (please tweak those patches in qa/pull-tester)
    2. It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
    3. It does not build on either Linux i386 or Win32 (via MinGW cross compile)
    4. The test suite fails on either Linux i386 or Win32
    5. The block test-cases failed (lookup the first bNN identifier which failed in https://github.com/TheBlueMatt/test-scripts/blob/master/FullBlockTestGenerator.java)

    If you believe this to be in error, please ping BlueMatt on freenode or TheBlueMatt here.

    This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

  4. jgarzik commented at 7:29 PM on September 30, 2014: contributor

    There's still a window when the code is active but the server is not.

  5. domob1812 commented at 5:46 AM on October 1, 2014: contributor

    That's true, but it is much shorter than before. Do you suggest to start the server even earlier?

  6. laanwj commented at 6:54 AM on October 1, 2014: member

    You cannot go much earlier; be careful not to start the RPC server before we have the lock on the data directory.

  7. sipa commented at 2:10 AM on October 2, 2014: member

    I remember this was suggested a (very) long time ago, and chosen not to do. I can't remember why though. Does anyone? @jgarzik @gavinandresen?

  8. jgarzik commented at 2:15 AM on October 2, 2014: contributor

    @sipa It introduced various crashes, because certain RPCs expected the program to be further along in initializatoin. It became a game of whack-a-mole to audit each RPC, perhaps gatewaying based on init progress.

    In general, it starts something early, and then doesn't use it until much later.

  9. laanwj commented at 7:13 AM on October 2, 2014: member

    Indeed. Auditing all RPCs would be the wrong way to go at it. It should just disallow all RPCs until the init process is finished. Which (on a quick skim) seems to be what this pull does.

  10. in src/rpcserver.cpp:None in 0ba552b68d outdated 
      33 | @@ -34,6 +34,8 @@ using namespace std;
  34 |  static std::string strRPCUserColonPass;
  35 |  
  36 |  static bool fRPCRunning = false;
  37 | +static const char* rpcWarmupStatus = "RPC server started";
    laanwj commented at 7:14 AM on October 2, 2014:

    Please don't double-use a const char* as a flag. Add another boolean (for ex.) fRPCInWarmup.

    Nit2: also better to use a std::string, so that it's possible to pass dynamic messages if necessary (for example if progress is to be reported at some point).

  11. domob1812 commented at 10:21 AM on October 2, 2014: contributor

    Yes, this patch disables all RPC until everything is initialised (at least it is intended to do that). I've updated the patch to use a new flag fRPCInWarmup in addition to the string rpcWarmupStatus, as suggested.

  12. in src/init.cpp:None in 5f7da23829 outdated 
     755 |  
 756 |      // ********************************************************* Step 5: verify wallet database integrity
 757 |  #ifdef ENABLE_WALLET
 758 |      if (!fDisableWallet) {
 759 |          LogPrintf("Using wallet %s\n", strWalletFile);
 760 | +        SetRPCWarmupStatus("verifying wallet");
    laanwj commented at 10:43 AM on October 2, 2014:

    I see a duplication here; you could subscribe SetRPCWarmupStatus to uiInterface.InitMessage signals and avoid it.

    domob1812 commented at 10:46 AM on October 2, 2014:

    I thought about that and decided not to do it (at least for the first proposal) because the init message is translated and the RPC status message (if it is kept generic) might be interpreted by clients in other ways than just presenting it to a user literally. (Think: RPC is an interface for machines not (only) humans.) But I'm definitely open to changing that if you think it is better to reuse the init message literally.

    laanwj commented at 11:23 AM on October 2, 2014:

    Yes I do think it is better to do it that way. The translation is a bit of a red herring - for bitcoind _() is a no-op. But in general If you intend this to be machine-readable, you need a status byte/word. A message can be changed at any time and should not be used as stable interface.

    Edit: IE, I mean passing an enum InitStage as well as the message to InitMessage and exposing that on the RPC

    laanwj commented at 11:34 AM on October 2, 2014:

    BTW: You could also give more detailed progress reporting by hooking uiInterface.ShowProgress.

  13. domob1812 commented at 11:28 AM on October 7, 2014: contributor

    Updated to connect to uiInterface.InitMessage instead of updating the RPC status in parallel.

  14. in src/rpcserver.cpp:None in 284e251feb outdated 
      33 | @@ -34,6 +34,9 @@ using namespace std;
  34 |  static std::string strRPCUserColonPass;
  35 |  
  36 |  static bool fRPCRunning = false;
  37 | +static bool fRPCInWarmup = true;
  38 | +static std::string rpcWarmupStatus("RPC server started");
    laanwj commented at 6:56 AM on October 8, 2014:

    You'll have to protect these with a mutex (CCriticalSection), as setting and reading them is done from different threads.

  15. laanwj commented at 6:57 AM on October 8, 2014: member

    utACK except for threading nit.

  16. gmaxwell commented at 8:22 AM on October 8, 2014: contributor

    Needs to be loudly released noted, as it may break some HA schemes (e.g. pass traffic to the first bitcoind that accepts a connection). Not something we should avoid breaking, but it should be visible.

  17. laanwj added this to the milestone 0.10.0 on Oct 8, 2014
  18. laanwj commented at 8:37 AM on October 8, 2014: member

    Agreed @gmaxwell, we need to announce it in the release notes as it breaks some expectations that clients may make at the moment. Luckily this is something that is extremely apparent (every RPC call failing), I don't expect it to introduce sneaky problems.

  19. domob1812 commented at 11:12 AM on October 8, 2014: contributor

    Done, thanks for the hint! (Since the status was only ever set from false to true, I thought it wasn't too critical - but it is better this way, of course. Also not sure about whether the std::string could cause problems if not synchronised.)

    If the patch is accepted, I will squash the commits properly.

  20. in src/bitcoin-cli.cpp:None in 8477f7c126 outdated 
      53 | +//
  54 | +class CConnectionFailed : public std::runtime_error
  55 | +{
  56 | +public:
  57 | +
  58 | +    explicit inline CConnectionFailed(const std::string& msg)
    Diapolo commented at 11:16 AM on October 8, 2014:

    Nit: Can you make this look like (indentation is wrong):

    <pre> explicit inline CConnectionFailed(const std::string& msg) : std::runtime_error(msg) {} </pre>

  21. in src/rpcserver.cpp:None in 8477f7c126 outdated 
     757 | +}
 758 | +
 759 | +void SetRPCWarmupFinished()
 760 | +{
 761 | +    LOCK(cs_rpcWarmup);
 762 | +    assert (fRPCInWarmup);
    Diapolo commented at 11:20 AM on October 8, 2014:

    Nit: Can you remove the space after the assert please.

    domob1812 commented at 11:39 AM on October 8, 2014:

    Ah yes. I usually use a different formatting style (which I honestly like better) but tried to adapt to Bitcoin's. It seems I missed some pieces. I'll correct them.

    Diapolo commented at 11:43 AM on October 8, 2014:

    Great, thank you :).

  22. laanwj commented at 12:27 PM on October 9, 2014: member

    Works for me. Tested ACK. @domob1812 mind adding a short piece in doc/release-notes.md telling people about this RPC warmup mode, and how this change can affect them? (can be based on the OP here)

  23. domob1812 commented at 8:02 AM on October 10, 2014: contributor

    Done. Is the formulation fine or should I extend/shorten it in any way?

    Let me know when the patch has a "final ACK" - I will then rebase and squash it.

  24. laanwj commented at 2:59 PM on October 22, 2014: member

    The release notes look good to me

    I'd like to see at least one other ACK before merging.

  25. gavinandresen commented at 6:15 PM on October 28, 2014: contributor

    utACK, but needs rebase.

  26. domob1812 force-pushed on Oct 28, 2014
  27. domob1812 commented at 8:09 PM on October 28, 2014: contributor

    Rebased and squashed the commits.

  28. in src/init.cpp:None in 2ec1598c5a outdated 
     747 | @@ -748,6 +748,17 @@ bool AppInit2(boost::thread_group& threadGroup)
 748 |              threadGroup.create_thread(&ThreadScriptCheck);
 749 |      }
 750 |  
 751 | +    /* Start the RPC server already.  It will be started in "warmup" modus
    laanwj commented at 11:15 AM on October 29, 2014:

    btw I'm not native English but is 'modus' the correct word to use here, not 'mode'?

    domob1812 commented at 11:30 AM on October 29, 2014:

    Neither am I, but I think you are right. If noone has any other suggestions, I'll change it accordingly.

  29. domob1812 force-pushed on Oct 29, 2014
  30. domob1812 commented at 5:12 PM on October 29, 2014: contributor

    Fixed the typo.

  31. sipa commented at 2:07 PM on November 4, 2014: member

    Needs rebase.

  32. Add "warmup mode" for RPC server.
    Start the RPC server before doing all the (expensive) startup
initialisations like loading the block index.  Until the node is ready,
return all calls immediately with a new error signalling "in warmup"
with an appropriate status message (similar to the init message).

This is useful for RPC clients to know that the server is there (e. g.,
they don't have to start it) but not yet available.  It is used in
Namecoin and Huntercoin already for some time, and there exists a UI
hooked onto the RPC interface that actively uses this to its advantage.
    af82884ab7
  33. domob1812 force-pushed on Nov 4, 2014
  34. domob1812 commented at 3:13 PM on November 4, 2014: contributor

    Done.

  35. laanwj merged this on Nov 4, 2014
  36. laanwj closed this on Nov 4, 2014
  37. laanwj referenced this in commit 06037f3f46 on Nov 4, 2014
  38. domob1812 deleted the branch on Nov 4, 2014
  39. MarcoFalke locked this on Sep 8, 2021
Contributors
domob1812BitcoinPullTesterjgarziklaanwjsipagmaxwellDiapologavinandresen

Milestone
0.10.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-21 18:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me