Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Poll bitcoind at startup instead of trying only once #2739

Merged
merged 3 commits into from
Sep 27, 2023
Merged

Conversation

t-bast
Copy link
Member

@t-bast t-bast commented Sep 7, 2023

When we start, bitcoind may not be ready to handle requests if we restarted it around the same time. We thus poll it for 30 seconds before giving up. This is especially useful for tests (e.g. on regtest).

@tee8z this should fix the start-up issue you're seeing for https://github.com/tee8z/doppler

@codecov-commenter
Copy link

codecov-commenter commented Sep 7, 2023

Codecov Report

Merging #2739 (14caaba) into master (37f3fbe) will increase coverage by 0.00%.
Report is 1 commits behind head on master.
The diff coverage is 100.00%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@           Coverage Diff           @@
##           master    #2739   +/-   ##
=======================================
  Coverage   85.87%   85.87%           
=======================================
  Files         216      216           
  Lines       18077    18070    -7     
  Branches      772      732   -40     
=======================================
- Hits        15523    15517    -6     
+ Misses       2554     2553    -1     
Files Coverage Δ
...ir-core/src/main/scala/fr/acinq/eclair/Setup.scala 75.14% <100.00%> (ø)

... and 12 files with indirect coverage changes

When we start, bitcoind may not be ready to handle requests if we restarted
it around the same time. We thus poll it for 30 seconds before giving up.
This is especially useful for tests (e.g. on regtest).
We've updated eclair accordingly but failed to update the startup
requirement.
Copy link
Member

@sstone sstone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I would just not change our current behaviour, and there should be a test to verify that we do wait for bitcoind to be ready (maybe a simple tests like restarting bitcoind inside a Future instantiating an eclair node would be enough?)

eclair-core/src/main/resources/reference.conf Show resolved Hide resolved
@t-bast
Copy link
Member Author

t-bast commented Sep 27, 2023

there should be a test to verify that we do wait for bitcoind to be ready (maybe a simple tests like restarting bitcoind inside a Future instantiating an eclair node would be enough?)

This is quite hard to do in a reliable way, isn't it? I'll think about it, but that sounds quite hard to do without introducing a lot of new code...

@sstone
Copy link
Member

sstone commented Sep 27, 2023

This is quite hard to do in a reliable way, isn't it? I'll think about it, but that sounds quite hard to do without introducing a lot of new code...

What about something like:

  test("wait for bitcoind") {
    import scala.concurrent.ExecutionContext.Implicits.global
    val sender = TestProbe()
    stopBitcoind()
    Future {
      startBitcoind()
      waitForBitcoindUp(sender)
      sender.send(bitcoincli, BitcoinReq("loadwallet", defaultWallet))
      sender.expectMsgType[JValue]
    }
    val config = ConfigFactory.parseMap(Map("eclair.bitcoind.wait-for-bitcoind-up" -> "true").asJava).withFallback(createConfig(wallet_opt = Some(defaultWallet)))
    instantiateEclairNode("A", config)
  }

@t-bast
Copy link
Member Author

t-bast commented Sep 27, 2023

I added a test in 14caaba, but it's annoying because it takes 30 seconds to run (there seems to be a timeout somewhere on bitcoind restart, when calling bitcoind.exitValue()), I'm trying to figure out why and if we can improve this.

@t-bast t-bast requested a review from sstone September 27, 2023 15:52
@sstone
Copy link
Member

sstone commented Sep 27, 2023

Yes it's annoying, and it is the bitcoin core process that takes an extra 30s to shutdown, I could not figure out why either.

@t-bast t-bast merged commit 0e4985c into master Sep 27, 2023
1 check passed
@t-bast t-bast deleted the wait-bitcoind-up branch September 27, 2023 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants