search

RCasatta / electrsd

Utility to run a regtest electrsd process connected to a given bitcoind instance, useful in integration testing environment.
MIT License
21 stars 18 forks source link

Add configurable workdir options #36

Closed rajarshimaitra closed 2 years ago

rajarshimaitra commented 2 years ago

Fixes #35

Replay the same changes of https://github.com/RCasatta/bitcoind/pull/52 for electrs index directory.

I couldn't figure a good way to stop the electrs server. So right now the drop is same as before. I am not entirely sure if that's total safe.. Looking or suggestions..

RCasatta commented 2 years ago

electrs handle signals https://github.com/romanz/electrs/blob/466a8987bb442ca52c1f261bb798bab5148c7921/src/signals.rs#L58

and we already send SIGUSR here https://github.com/RCasatta/electrsd/blob/b5f1d10b48ecab90949a8f229c9e3c2e6651656a/src/lib.rs#L227

So what I think we should do is a terminate fn which sends SIGINT, then a cycle checking the process exited with process.try_wait (do it like 20 times every 250ms) then kill()

rajarshimaitra commented 2 years ago

Hmm.. So it seems the self.process.kill() is equivalent to sending a SIGKILL to the process as per the doc.. https://doc.rust-lang.org/std/process/struct.Child.html#method.kill

So if electrs is by default killed by ctrl+c type of behavior without needing for extra care, then I think we are already good here?? Because BitcoinD already handles dropping properly if persistent db is provided.. So ElectrsD can retain its current behavior??

Let me know if I am missing something..

RCasatta commented 2 years ago

IIUC Electrs handle shutdown gracefully with SIGINT (Ctrl-c) through the code I linked, while SIGKILL shutdown abruptely and could potentially cause data loss and problems if restarted

rajarshimaitra commented 2 years ago

Yes ok makes sense.. I sketched a basic version of it.. Didn't wait in loop, blocked the thread until the electrsd exits or errors..

Also I had to remove the trigger feature because now we need nix by default so having trigger is now redundant.. Cargo would error otherwise..

rajarshimaitra commented 2 years ago

Thanks @RCasatta for the look.. Updated as suggested.. Didn't understood one of them..

rajarshimaitra commented 2 years ago

@RCasatta updated with all the suggestions.. Sorry took some time to get back to this.. Squashed the commits for easier final review..

RCasatta commented 2 years ago

LGTM, it misses a test with the persistent dir, but we can do it in another PR