rpc: Add a `-rpcwaittimeout` parameter to limit time spent waiting #21056

cdecker commented at 5:34 PM on February 1, 2021: contributor

Adds a new numeric -rpcwaittimeout that can be used to limit the time we spend waiting on the RPC server to appear. This is used by downstream projects to provide a bit of slack when bitcoinds RPC interface is not available right away.

This makes the -rpcwait argument more useful, since we can now limit how long we'll ultimately wait, before potentially giving up and reporting an error to the caller. It was discussed in the context of the BTCPayServer wanting to have c-lightning wait for the RPC interface to become available but still have the option of giving up eventually (4355).

I checked with laanwj whether this is already possible (comment), and whether this would be a welcome change. Initially I intended to repurpose the (optional) argument to -rpcwait, however I decided against it since it would potentially break existing configurations, using things like rpcwait=1, or rpcwait=true (the former would have an unintended short timeout, when old behavior was to wait indefinitely).

~Due to its simplicity I didn't implement a test for it yet, but if that's desired I can provide one.~ Test was added during reviews.

cdecker force-pushed on Feb 1, 2021

DrahtBot added the label RPC/REST/ZMQ on Feb 1, 2021

in src/bitcoin-cli.cpp:718 in 7f19f5f83d outdated

 713 | @@ -712,6 +714,9 @@ static UniValue ConnectAndCallRPC(BaseRequestHandler* rh, const std::string& str
 714 |      UniValue response(UniValue::VOBJ);
 715 |      // Execute and handle connection failures with -rpcwait.
 716 |      const bool fWait = gArgs.GetBoolArg("-rpcwait", false);
 717 | +    const int timeout = gArgs.GetArg("-rpcwaittimeout", DEFAULT_WAIT_CLIENT_TIMEOUT);
 718 | +    const int64_t deadline = GetTime() + timeout;

jonatack commented at 7:24 PM on February 1, 2021:

I think GetTime is deprecated in favor of GetSystemTimeInSeconds (not mockable) or GetTime<T> (mockable), see src/util.time.h

cdecker commented at 7:55 PM on February 1, 2021:

Ok, amended the use of GetTime with the GetTime<T> version (had to look it up, hopefully I got the right version).

in src/bitcoin-cli.cpp:77 in 7f19f5f83d outdated

  70 | @@ -70,6 +71,7 @@ static void SetupCliArgs(ArgsManager& argsman)
  71 |      argsman.AddArg("-rpcport=<port>", strprintf("Connect to JSON-RPC on <port> (default: %u, testnet: %u, signet: %u, regtest: %u)", defaultBaseParams->RPCPort(), testnetBaseParams->RPCPort(), signetBaseParams->RPCPort(), regtestBaseParams->RPCPort()), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::OPTIONS);
  72 |      argsman.AddArg("-rpcuser=<user>", "Username for JSON-RPC connections", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
  73 |      argsman.AddArg("-rpcwait", "Wait for RPC server to start", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
  74 | +    argsman.AddArg("-rpcwaittimeout=<n>", strprintf("Timeout in seconds to wait for the RPC server to start, or 0 for no timeout. (default: %d)", DEFAULT_WAIT_CLIENT_TIMEOUT), ArgsManager::ALLOW_INT, OptionsCategory::OPTIONS);

jonatack commented at 7:29 PM on February 1, 2021:

Judging from the PR description you likely thought about this, but could the timeout still be an argument in -rpcwait and only kick in if the value is greater than 1? That would avoid introducing another config option and as a bonus would be shorter to type. IDK what kind of timeout values API clients need.

cdecker commented at 7:56 PM on February 1, 2021:

I'm quite happy to switch it around, but having 1 as a special value is kinda weird as well. I thought I'd go the most cautious route first and see which way wins in the review xD

jonatack commented at 8:04 PM on February 1, 2021:

Fair. Is the real time ever that low anyway? Maybe counting -rpcwait=1 as one second would be fine...I don't have a fast computer and generally run debug builds, so I may see slower times than others, but rpcwait is ~3 seconds for me on signet and much longer on regtest (~10s), testnet (~95s) and mainnet.

cdecker commented at 12:17 PM on February 3, 2021:

I don't expect many users to run into issues if we were to special case -rpcwait=1, but given the "creativity" of users I wouldn't put it past them to use -rpcwait=42 or -rpcwait=31337. Some users seem to see "any value" as a personal challenge :smiley:

jonatack commented at 3:36 PM on February 16, 2021:

I'd still be in favor of combining rpcwait and rpcwaittimeout, but if we prefer two arguments for this, it may be helpful to mention the relationship to -rpcwait in this description.

in src/bitcoin-cli.cpp:42 in 7f19f5f83d outdated

  38 | @@ -39,6 +39,7 @@ UrlDecodeFn* const URL_DECODE = urlDecode;
  39 |  
  40 |  static const char DEFAULT_RPCCONNECT[] = "127.0.0.1";
  41 |  static const int DEFAULT_HTTP_CLIENT_TIMEOUT=900;
  42 | +static const int DEFAULT_WAIT_CLIENT_TIMEOUT=0;

jonatack commented at 7:30 PM on February 1, 2021:

nit, could be constexpr and apply clang formatting for new code

cdecker commented at 7:55 PM on February 1, 2021:

Had to catch up about constexpr, but good point :+1:

jonatack commented at 7:32 PM on February 1, 2021: member

Concept ACK. Could append a test to the existing -rpcwait coverage at the end of test/functional/interface_bitcoin_cli.py to add coverage.

theStack commented at 10:59 PM on February 1, 2021: member

Concept ACK

laanwj commented at 5:49 AM on February 2, 2021: member

Concept ACK

practicalswift commented at 12:29 PM on February 2, 2021: contributor

Concept ACK

cdecker commented at 12:19 PM on February 3, 2021: contributor

Thanks for the reviews and concept ACKs everybody :+1:

I'm a bit puzzled by the appveyo failure, which doesn't surface any details related to these changes, is this something my changes broke or was it something with master at the time of the branching?

promag commented at 12:29 PM on February 3, 2021: member

Concept ACK, code change looks good to me.

Just noting that effective minimum timeout is 1s, maybe sleep with min(timout, 1s)? An alternative would be to specify attempt count.

cdecker commented at 12:36 PM on February 3, 2021: contributor

Just noting that effective minimum timeout is 1s, maybe sleep with min(timout, 1s)? An alternative would be to specify attempt count.

Excellent point/ The reason I went for the deadline approach was that in between sleeps we are actually attempting to connect over the network, which has its own timeout (DEFAULT_HTTP_CLIENT_TIMEOUT), which would cause drift if we were to just count the attempts instead of using the delta.

Effectively the time we can wait in the worst case is -rpcwaittimeout + DEFAULT_HTTP_CLIENT_TIMEOUT if we happen to start an attempt right before the deadline gets triggered. If we were counting we'd have a maximum of -rpcwaittimeout * (1s + DEFAULT_HTTP_CLIENT_TIMEOUT) , which isn't really the interface I'd feel comfortable using.

jonatack commented at 1:33 PM on February 3, 2021: member

I'm a bit puzzled by the appveyo failure, which doesn't surface any details related to these changes, is this something my changes broke or was it something with master at the time of the branching?

Appveyor is a bit flakey ATM, it's probably unrelated.

cdecker force-pushed on Feb 5, 2021

cdecker commented at 11:55 AM on February 5, 2021: contributor

Rebased on top of master and squashed, let's see if the flake persists.

cdecker force-pushed on Feb 5, 2021

cdecker commented at 11:16 AM on February 8, 2021: contributor

Yep seems, like CI is happier now ^^

promag commented at 11:50 AM on February 8, 2021: member

Code review ACK 7dcead4319258b058eb8ee5f2271e5c6dad264aa.

nit, could have a release note.

in src/bitcoin-cli.cpp:719 in 7dcead4319 outdated

 714 | @@ -713,6 +715,9 @@ static UniValue ConnectAndCallRPC(BaseRequestHandler* rh, const std::string& str
 715 |      UniValue response(UniValue::VOBJ);
 716 |      // Execute and handle connection failures with -rpcwait.
 717 |      const bool fWait = gArgs.GetBoolArg("-rpcwait", false);
 718 | +    const int timeout = gArgs.GetArg("-rpcwaittimeout", DEFAULT_WAIT_CLIENT_TIMEOUT);
 719 | +    const int64_t deadline = GetTime<std::chrono::microseconds>().count() + timeout;

darosior commented at 9:47 PM on February 10, 2021:

I think you want this to be std::chrono::seconds instead of std::chrono::microseconds, or to change the doc above to mention the parameter is passed in microseconds.

darosior changes_requested

darosior commented at 9:48 PM on February 10, 2021: member

Concept ACK

Small note too: you should remove the @ from the OP or laanwj will get spammed by all the altcoins merge (and the merge script will refuse it anyways) :)

cdecker commented at 10:41 AM on February 15, 2021: contributor

Code review ACK 7dcead4.

nit, could have a release note.

Can you point me to the conventions for release notes? This is my first PR that might need them 🎉

laanwj commented at 10:55 AM on February 15, 2021: member

Can you point me to the conventions for release notes? This is my first PR that might need them

I couldn't find anything in CONTRIBUTING.md (this would make sense to add). We prefer to use 'release notes fragments' to avoid introducing merge hotspots. These will be collected before the next major release.

These have a) the broad category (in this case, I guess "RPC client functionality") b) a very brief description (not full documentation!) of the user-visible change.

See for example: https://github.com/bitcoin/bitcoin/blob/master/doc/release-notes-18077.md https://github.com/bitcoin/bitcoin/blob/master/doc/release-notes-19776.md

cdecker commented at 11:04 AM on February 15, 2021: contributor

Can you point me to the conventions for release notes? This is my first PR that might need them

I couldn't find anything in CONTRIBUTING.md (this would make sense to add). We prefer to use 'release notes fragments' to avoid introducing merge hotspots. These will be collected before the next major release.

These have a) the broad category (in this case, I guess "RPC client functionality") b) a very brief description (not full documentation!) of the user-visible change.

See for example: https://github.com/bitcoin/bitcoin/blob/master/doc/release-notes-18077.md https://github.com/bitcoin/bitcoin/blob/master/doc/release-notes-19776.md

Awesome, thanks. I was expecting something like this to avoid hotspots, and it's way easier than c-lightning's Changelog-in-the-commit-message method :+1:

promag commented at 11:05 AM on February 15, 2021: member

We have details at https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#release-notes

laanwj commented at 11:10 AM on February 15, 2021: member

We have details at https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#release-notes

Oh great! I somehow couldn't find this (I did grep for a few things). Might make sense to link to this from CONTRIBUTING.md.

cdecker force-pushed on Feb 16, 2021

cdecker commented at 2:10 PM on February 16, 2021: contributor

Added preliminary release notes in a new file, and fixed up the units in the GetTime<T> template as well as the unit of the sleep call. Let me know if the release notes are not in the desired format, and I'll fix them up asap.

in doc/release-notes-21056.md:6 in d37379384c outdated

   0 | @@ -0,0 +1,6 @@
   1 | +RPC client functionality
   2 | +------------------------
   3 | +
   4 | +- The new `-rpcwaittimeout` argument to `bitcoin-cli` sets the timeout
   5 | +  in seconds to use with `-rpcwait`. If the timeout expires
   6 | +  `bitcoin-cli` will report a failure. (#21056)

jonatack commented at 3:29 PM on February 16, 2021:

Among the current headers in doc/release-notes.md, "New settings" might be the closest, but I'm not sure. There doesn't seem to be a CLI header. Suggestion follows, feel free to ignore.

-RPC client functionality
-------------------------
+New settings
+------------
 
-- The new `-rpcwaittimeout` argument to `bitcoin-cli` sets the timeout
-  in seconds to use with `-rpcwait`. If the timeout expires
+- A new `-rpcwaittimeout` argument to `bitcoin-cli` sets the timeout
+  in seconds to use with `-rpcwait`. If the timeout expires,
   `bitcoin-cli` will report a failure. (#21056)

laanwj commented at 4:30 PM on February 26, 2021:

I don't know if that header is better. As a compromise "New bitcoin-cli settings" maybe?

I do think it's important to keep mentions of server and other tools's command line arguments separated. Then again, all of this can be organized when merging the fragments—it's not an automatic process.

jonatack commented at 5:25 PM on February 26, 2021:

Yes, "New bitcoin-cli settings" seems better to me, too. Though as mentioned in #21056 (review), if we add the argument to the existing -rpcwait= (as a CLI setting intended for human manual use rather than by client software, ISTM it may be less subject to API constraints and would be less verbose than -rpcwait -rpcwaittimeout=), it could be "Updated bitcoin-cli settings".

in src/bitcoin-cli.cpp:758 in d37379384c outdated

 761 | @@ -757,8 +762,9 @@ static UniValue ConnectAndCallRPC(BaseRequestHandler* rh, const std::string& str
 762 |              }
 763 |              break; // Connection succeeded, no need to retry.
 764 |          } catch (const CConnectionFailed&) {
 765 | -            if (fWait) {
 766 | -                UninterruptibleSleep(std::chrono::milliseconds{1000});
 767 | +            const int64_t now = GetTime<std::chrono::seconds>().count();
 768 | +            if (fWait && (timeout == 0 || now < deadline)) {

jonatack commented at 3:44 PM on February 16, 2021:

The error I'm seeing is a bit confusing. It would be good to raise a specific error for the timeout value exceeded.

$ bitcoind -regtest -daemon ; bitcoin-cli -regtest -rpcwait -rpcwaittimeout=3 -getinfo
Bitcoin Core starting
error: server in warmup

jonatack commented at 3:45 PM on February 16, 2021: member

A few comments below. Haven't checked, but ISTM the logic could use std::chrono throughout rather than converting values back to integers. A functional test verifying the timeout works correctly and a specific error type is raised might be a good idea.

luke-jr approved

luke-jr commented at 5:12 PM on February 23, 2021: member

utACK, but I think actual software shouldn't be using bitcoin-cli at all...

laanwj commented at 8:23 PM on February 25, 2021: member

utACK, but I think actual software shouldn't be using bitcoin-cli at all...

For what it's worth I agree with @luke-jr here. Never really understood why lightningd doesn't use the RPC API directly. It's generally the more stable way, less moving parts, less potential ambiguity in input arguments. But it's an aside of course and off topic here.

cdecker commented at 10:16 AM on February 26, 2021: contributor

For what it's worth I agree with @luke-jr here. Never really understood why lightningd doesn't use the RPC API directly. It's generally the more stable way, less moving parts, less potential ambiguity in input arguments. But it's an aside of course and off topic here.

Not trying to make this a lightningd specific proposal, but since it was brought up I'll try to reconstruct the rationale at the time.

It was/is easier to call out to an existing program that includes the logic for authentication, error handling and timeouts than to implement it from scratch on top of out io library which doesn't have good http support (this is likely self-inflicted, but it's what we've got and it's working well for us so far). Authentication here being the big win, since if you have bitcoin-cli working you don't have to do anything to configure lightningd to be able to talk to bitcoind (no need to handle cookie auth, user/pass auth, binding and connecting, no need to implement TLS). If we had implemented all these things we'd likely have ended up replicating all the functionality bitcoin-cli already has, which didn't seem like a good investment of time given the already huge amount of work to be done.

Are there downsides of calling out to bitcoin-cli? Sure. Will we eventually end up replacing it with an internal implementation? Maybe. Is it working well as it is now? I think so 😉

That being said, I'm sure we're not the only ones that could use a wait timeout (thinking primarily of scripts and operations).

laanwj commented at 4:15 PM on February 26, 2021: member

Are there downsides of calling out to bitcoin-cli? Sure. Will we eventually end up replacing it with an internal implementation? Maybe. Is it working well as it is now? I think so

I guess it's mostly down to personal preference, I prefer to use APIs directly, calling out to external programs reminds me of shell scripts which always are very environment-dependent. One concrete case is that bitcoin RPC might be forwarded from another container. Right now it's necessary to copy bitcoin-cli to the container hosting c-lightning.

Also as bitcoin-cli is aimed at human users, it has some client-side processing for arguments—some arguments are parsed as JSON others string-ized. Sending JSON directly is more "pure". Due to this added "moving part" I would personally prefer to avoid it from robust software.

Also accessing the API directly gives you full control over things such as timeouts. No need to wait for PRs to be merged here and then part of a release so that users have it :smile: JSONRPC handling is straightforward if you already have JSON for your own APIs.

It was/is easier to call out to an existing program that includes the logic for authentication

For authentication—I'd somewhat agree, but we don't do anything complex such as HMAC or challenge/response. The only available authentication mechanism is HTTP basic authentication with a) send $DATADIR/.cookie as user/password or b) manually provide user/password.

no need to implement TLS

There hasn't been TLS support in bitcoin-cli for years.

Sorry for derailing this further. Again, this is not meant as criticism on this change nor on lightningd (and I'm not volunteering to make a patch to change this). As you say, it works :woman_shrugging:

cdecker force-pushed on Mar 8, 2021

cdecker commented at 1:19 PM on March 8, 2021: contributor

Reworded the release notes as proposed by @jonatack, and rebased on top of master.

Open questions are:

Add a specific error message pointing towards a timeout if we get something like RPC warmup
- Pointed out here
- I think passing through the error is correct here, and not amend it since the timeout is merely retrying a failing attempt and eventually we give up passing through the cause. If we were to customize the error we could end up hiding the underlying cause for the failure (timeout ok, but what caused the timeout to fail ultimately?)
Instead of adding a new command line option in the form of -rpcwaittimeout, merge it instead with -rpcwait, redefining the semantics of the value. See above for discussion but here's a summary:
- Pro: merging avoids adding a new option, making the expression more concise
- Con: users may have scripts and utilities that use -rpcwait=1 to opt into the waiting, but with the new semantics we'd just wait for 1 second. Also suddenly things like -rpcwait=true would be invalid or we'd require workarounds mapping true, on etc to 0.

Anything I'm missing?

laanwj commented at 7:50 AM on March 15, 2021: member

If we were to customize the error we could end up hiding the underlying cause for the failure (timeout ok, but what caused the timeout to fail ultimately?)

Tend to agree. Maybe printing both would be a possibility? E.g. timeout on transient error: ….

Instead of adding a new command line option in the form of -rpcwaittimeout, merge it instead with -rpcwait, redefining the semantics of the value.

I'm, in the abstract, not a big fan of Swiss-army-knife overloading of arguments to options either. It quickly gets too clever for its own good. Would be fine with keeping it like this.

cdecker commented at 7:26 PM on March 16, 2021: contributor

Tend to agree. Maybe printing both would be a possibility? E.g. timeout on transient error: ….

I'll look into it, sounds like a good idea :+1:

cdecker commented at 1:43 PM on March 23, 2021: contributor

Prefixed the error message with the prefix proposed by @laanwj. One thing that I'm not quite clear on is whether the string allocated by strprintf is freed (not too familiar how the memory ownership in the presence of exceptions in C++ is). Would that require additional handling or is it ok in this case to wait for the exit to clean up memory?

laanwj commented at 3:48 PM on March 23, 2021: member

Thanks!

One thing that I'm not quite clear on is whether the string allocated by strprintf is freed

Yes, the exception object (including the string it refers to) is destructed when it leaves the scope where it is catched. This should be okay.

cdecker commented at 11:09 AM on April 6, 2021: contributor

Concept ACK. Could append a test to the existing -rpcwait coverage at the end of test/functional/interface_bitcoin_cli.py to add coverage.

Going through the reviews I noticed that I hadn't addressed this so far. Added a test for it where suggested.

cdecker requested review from darosior on Apr 15, 2021

cdecker requested review from jonatack on Apr 15, 2021

cdecker requested review from laanwj on Apr 15, 2021

cdecker commented at 12:40 PM on April 15, 2021: contributor

As far as I can see the only unresolved thing in this PR is the discussion about whether to reuse -rpcwait and redefine the argument to be the timeout in seconds, or whether the current addition of a new flag is preferrable.

in test/functional/interface_bitcoin_cli.py:258 in 4840dee30d outdated

 245 | @@ -246,6 +246,10 @@ def run_test(self):
 246 |          self.nodes[0].wait_for_rpc_connection()
 247 |          assert_equal(blocks, BLOCKS + 25)
 248 |  
 249 | +        self.log.info("Test -rpcwait option waits at most -rpcwaittimeout seconds for startup")
 250 | +        self.stop_node(0)  # stop the node so we time out
 251 | +        assert_raises_process_error(1, "Could not connect to the server", self.nodes[0].cli('-rpcwait', '-rpcwaittimeout=5').echo)
 252 | +

promag commented at 8:06 AM on April 22, 2021:

nit, could check elapsed time is >= to 5s.

cdecker commented at 9:03 AM on May 11, 2021:

Added check using assert_greater_than_or_equal and had to import time but that should work ok.

promag commented at 10:05 AM on June 3, 2021:

This should be assert_greater_than_or_equal(time.time(), start_time + 5).

in src/bitcoin-cli.cpp:799 in 4840dee30d outdated

 739 | @@ -738,6 +740,9 @@ static UniValue ConnectAndCallRPC(BaseRequestHandler* rh, const std::string& str
 740 |      UniValue response(UniValue::VOBJ);
 741 |      // Execute and handle connection failures with -rpcwait.
 742 |      const bool fWait = gArgs.GetBoolArg("-rpcwait", false);
 743 | +    const int timeout = gArgs.GetArg("-rpcwaittimeout", DEFAULT_WAIT_CLIENT_TIMEOUT);

promag commented at 9:05 AM on April 22, 2021:

nit, is it fine to accept <0? A negative value is same as -rpcwait=false.

cdecker commented at 8:57 AM on May 11, 2021:

I think so, the alternative would be to throw an error, but that might be a bit much.

in src/bitcoin-cli.cpp:758 in 4840dee30d outdated

 752 | @@ -748,11 +753,12 @@ static UniValue ConnectAndCallRPC(BaseRequestHandler* rh, const std::string& str
 753 |                  }
 754 |              }
 755 |              break; // Connection succeeded, no need to retry.
 756 | -        } catch (const CConnectionFailed&) {
 757 | -            if (fWait) {
 758 | -                UninterruptibleSleep(std::chrono::milliseconds{1000});
 759 | +        } catch (const CConnectionFailed& e) {
 760 | +            const int64_t now = GetTime<std::chrono::seconds>().count();
 761 | +            if (fWait && (timeout == 0 || now < deadline)) {

promag commented at 9:08 AM on April 22, 2021:

Maybe timeout <= 0 - zero or negative means no timeout.

cdecker commented at 9:02 AM on May 11, 2021:

Amended the == to become <= :+1:

promag commented at 9:09 AM on April 22, 2021: member

Tested ACK 4840dee30d8c679a2df999d7b9d48e19653ad3bd.

As far as I can see the only unresolved thing in this PR is the discussion about whether to reuse -rpcwait and redefine the argument to be the timeout in seconds, or whether the current addition of a new flag is preferrable.

I think current approach is fine otherwise how would the user disable waiting - -rpcwait=0 is disable or wait forever? @cdecker you can update OP now that there is a test.

DrahtBot commented at 6:00 PM on May 2, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

cdecker force-pushed on May 11, 2021

cdecker commented at 9:03 AM on May 11, 2021: contributor

Rebased on top of master, addressed @promag's feedback and amended OP to mention the tests that are now included.

promag commented at 5:44 PM on May 11, 2021: member

@cdecker I think you forgot to autosquash?

cdecker commented at 7:47 PM on May 11, 2021: contributor

Sorry, used to the c-lightning workflow where we keep fixups to show how things were addressed. Will squash as soon as I wake up 🙂

cdecker force-pushed on May 14, 2021

rpc: Add a `-rpcwaittimeout` parameter to limit time spent waiting

Adds a new numeric `-rpcwaittimeout` that can be used to limit the
time we spend waiting on the RPC server to appear. This is used by
downstream projects to provide a bit of slack when `bitcoind`s RPC
interface is not available right away.

a7fcc8eb59

doc: Add release notes for the `-rpcwaittimeout` cli parameter c490e17ef6

rpc: Prefix rpcwaittimeout error with details on its nature

As proposed by @laanwj the error message is now prefixed with the
"timeout on transient error:" prefix, to explain why the error is
suddenly considered terminal.

f76cb10d7d

cdecker commented at 9:23 AM on June 3, 2021: contributor

Rebased on top of master

cdecker force-pushed on Jun 3, 2021

promag commented at 10:06 AM on June 3, 2021: member

Tested ACK 5685cdcacd but the test needs to be fixed.

rpc: Add test for -rpcwaittimeout

Suggested-by: Jon Atack <@jonatack>

b9e76f1bf0

cdecker force-pushed on Jun 3, 2021

promag commented at 8:58 PM on June 3, 2021: member

ACK b9e76f1bf08c52fcd402b2314e00db4ad247ebc8.

cdecker commented at 10:27 AM on June 4, 2021: contributor

Tested ACK 5685cdc but the test needs to be fixed.

Yep, I switched the arguments around apparently :wink:

cdecker commented at 9:54 AM on June 21, 2021: contributor

Is this good in its current state or does something need to be addressed? Probably not the highest priority issue, but would love to remove it from my watchlist, e.g., by merging it :wink:

laanwj commented at 10:58 AM on June 21, 2021: member

Code review ACK b9e76f1bf08c52fcd402b2314e00db4ad247ebc8

MarcoFalke added the label Utils/log/libs on Jun 21, 2021

laanwj merged this on Jun 21, 2021

laanwj closed this on Jun 21, 2021

sidhujag referenced this in commit 448f334df4 on Jun 24, 2021

cdecker deleted the branch on Jun 24, 2021

luke-jr referenced this in commit 636defd880 on Jun 27, 2021

luke-jr referenced this in commit ada919883e on Jun 27, 2021

luke-jr referenced this in commit ec996d1ede on Jun 27, 2021

gwillen referenced this in commit a83d2595bc on Jun 1, 2022

DrahtBot locked this on Aug 16, 2022