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

Electrum connection on mobile device constatly breaks #51

Open
dr-orlovsky opened this issue Mar 19, 2021 · 10 comments
Open

Electrum connection on mobile device constatly breaks #51

dr-orlovsky opened this issue Mar 19, 2021 · 10 comments

Comments

@dr-orlovsky
Copy link
Contributor

dr-orlovsky commented Mar 19, 2021

On Apple mobile devices (all iPhones and iPads, iOS 14) Electrum connection gets broken after some small time even if app is not in the background, This does not happen on iOS simulator though. Unfortunately I was not yet able to acquire debug/trace logs of what's going wrong (iOS device does not print debug info in rust code and I am still figuring how to do that), So I just see that ElectrumClient starts returning an error on all method calls and not yet able to trace which specific error is that. Will keep updated, but opening an issue in case if you know something about this problem already.

I assume that this is caused by iOS closing TCP connection after certain period of time (even for foreground apps).

@RCasatta
Copy link
Member

I worked on gdk single-sig which is used by iOS wallet Aqua and use this library, even if I am not directly involved anymore from what I know from @lvaccaro the library is pretty stable and there are almost no crash.

Other than #33 and #34 that may improve library stability, I came across the descriptor-wallet repo code (not sure if the app it's based on that) and I think the wrapping of the Client in the RefCell in ElectrumTxResolver of the descriptor-wallet repo it's not needed and may cause some issue, could you try to remove it?

If that is not the case, we may need some other details to understand the issue such as logs, backtraces, and also the mobile app code using the library if it's open-source

@dr-orlovsky
Copy link
Contributor Author

wrapping of the Client in the RefCell in ElectrumTxResolver of the descriptor-wallet repo it's not needed and may cause some issue, could you try to remove it?

Unfortunately it is needed to be used alongside rust-gtk (which is about GTK UI library, not the same as Blockstream's GDK) in Bitcoin Pro :(

But I will try to do that just to check wether this is the source of the issue.

@dr-orlovsky
Copy link
Contributor Author

Mobile app source code is opensource, but it is a Rust wrapped in C wrapped in Swift lib wrapped in Swift UI, so I assume it will take a lot of your effort to investigate.

Rust wallet: https://github.com/mycitadel/citadel-runtime
C lib on top: https://github.com/mycitadel/libcitadel
Swift lib on top: https://github.com/mycitadel/citadelkit
UI part: https://github.com/mycitadel/mycitadel-swiftui

@RCasatta
Copy link
Member

I was forgetting to tell you that now methods on Client doesn't require anymore &mut self, so it should be easier

@dr-orlovsky
Copy link
Contributor Author

Oh great, I have removed RefCell. However it happened that I was using Electrum in Citadel directly, without TxResolver, so it was not the case of the issue :(

It happens in places like this one: https://github.com/mycitadel/citadel-runtime/blob/master/src/runtime/processor/chain_sync.rs#L74-L80

And client is intialized here: https://github.com/mycitadel/citadel-runtime/blob/master/src/runtime/service.rs#L83-L91

There are no other Electrum use than these (aside of other places calling electrum exactly the same way and failing exactly the same way)

@RCasatta
Copy link
Member

I see, then I don't understand exactly what is happening.

I may suggest wrapping the electrum error in your error type, so that we may have a better clue?

FYI if you don't already know, is that the client may be configured to retry on error . It's not active by default and current release may have an issue on exposing the needed Config #52

@dr-orlovsky
Copy link
Contributor Author

Hm, will try with retry, nice point.

I was not wrapping errors since they were missing many important derives and wrapping them required regression on my error types. But I suppose that was fixed with my previous PRs here, which I assume already got into released version, so will do that.

For now I also tried not to persist Electrum connection and instead initialize new electrum instance each time we need it. Bad practice, but (1) it is called really rarely (and no chain monitoring background service is yet there) and (2) is a temporary measure to figure out am I right that this is closed TCP connection.

@dr-orlovsky
Copy link
Contributor Author

@RCasatta I do confirm than when Electrum client is not kept around but initialized in place for a single use the error does not happen. I did this for bitcoin part and left RGB to use permanent connection and it resulted in working bitcoin part and RGB failing to use electrum after a certain period of time or when application got at least once into background.

So, most probably TCP connection got closed by OS. Will do more study once will update error type and get information from them.

Regarding retry; does it retries to re-establish TCP connection or just resends the same request over a broken connection?

PS. It is really strange that Aqua does not experience the same issue on iOS; may be they are do not keep permanent electrum connectivity

@RCasatta
Copy link
Member

Regarding retry; does it retries to re-establish TCP connection or just resends the same request over a broken connection?

it instatiates a new client, so it recreates the TCP connection https://github.com/bitcoindevkit/rust-electrum-client/blob/master/src/client.rs#L76

PS. It is really strange that Aqua does not experience the same issue on iOS; may be they are do not keep permanent electrum connectivity

For sure until I was there there were no permanent connections, because IMO especially on mobile they are a source of trouble. (They may be required tough for realtime requirements of course)

@dr-orlovsky
Copy link
Contributor Author

For sure until I was there there were no permanent connections, because IMO especially on mobile they are a source of trouble. (They may be required tough for realtime requirements of course)

I see. Will propose to add a doc note for mobile devs.

With RGB and LNP Node I use ZMQ a lot, and no problem with permanent connectivity there - but it's not TCP, true.

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

No branches or pull requests

2 participants