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

Unacceptable performance issue affecting versions 2.19.0 up to the latest making the SDK unusable for a mobile wallet #1130

Open
robkainz opened this issue Nov 8, 2024 · 11 comments · Fixed by #1141
Assignees
Labels
bug Something isn't working
Milestone

Comments

@robkainz
Copy link

robkainz commented Nov 8, 2024

Description

Per this article https://hedera.com/blog/deprecation-of-the-insecure-hedera-consensus-service-hcs-mirror-node-endpoints I am trying to get Wallawallet (IOS version) updated ASAP in the IOS app store before next week's deadline, yet when I tried the latest Hedera go sdk build (v2.49.0) it takes 44 seconds to fetch the balance and token balances from my IOS app and 4 minutes to generate a recovery phrase using the SDKs methods. The version of the SDK I am upgrading from (v2.17.6) from took less than 3 seconds, and there is no issue with 2.18.0. So the problem was introduced in version 2.19.0.

This is not a problem with query responses, but rather seems to be an issue with blocking the main thread from the limited amount of time I have had to look at this. I'm guessing perhaps there are some background init tasks that are blocking the main thread? Maybe checking node statuses, etc.? As a result, the user is left hanging for very long periods of time.

I am using React Native (contact me privately to learn versions being used if needed), go and gomobile. I changed nothing but my go library that is built with a Hedera sdk dependency. I also feed my own json config to it.

Please advise how I am to meet the deadline next week for mirror node/SDK changes as I am unable to find a working version of the SDK that is in the acceptable range of SDKs defined in the Hedera blog article referenced above. I would appreciate a quick fix and an extension of the deadline so that my app can be properly tested before going into production.

If needed I can provide some Wallawallet Test Flight builds to share/test versions of Wallawallet with any particular build you may recommend that meets the upcoming requirements.

Thanks

Steps to reproduce

  • Create a React Native mobile app, using the React Native bridge with gomobile and go code to call v2.49.0 Hedera go SDK methods from React Native.
  • Launch the app on an iPhone
  • Invoke the method for generating the random seed words or try to get your balance

Additional context

No response

Hedera network

mainnet

Version

v2.49.0

Operating system

Other

@robkainz robkainz added the bug Something isn't working label Nov 8, 2024
@Neurone
Copy link

Neurone commented Nov 8, 2024

Hi, as a temporary workaround while we investigate this issue, v2.23.0 is the oldest version you can use that is not affected by the endpoint change.

Can you test to see if you are experiencing the same issue with this version?

@robkainz
Copy link
Author

robkainz commented Nov 8, 2024

That was the second version I had tested to be honest, because it was the first version listed as acceptable in the blog entry. It failed too. I then worked ahead from the build I was using up to 2.19.0, where I saw the problem introduced. The issue definitely began with 2.19.0.

@Neurone
Copy link

Neurone commented Nov 8, 2024

Thanks for looking into this, we put the removal of the old endpoints on hold until this issue is investigated and resolved.

v2.19 is really old (2022) and it seems we don't have any problems using the most updated SDKs via standalone applications. Perhaps this is something more subtle, related to the iOS world, ReactNative or gomobile.

@robkainz
Copy link
Author

robkainz commented Nov 8, 2024

That's a relief. Thank you. Yes, I think it's almost certainly related to the React Native bridge / gomobile.

@0xivanov
Copy link
Contributor

Hello, I did some investigation and the change from v.2.18 to v2.19 that might be problematic is this: https://github.com/hashgraph/hedera-sdk-go/pull/628/files#r1836237841

This is a schedule based task that is running continuously on a separate gorountine. I am not sure how gomobile lib handles gorountines. @robkainz what do you think?

@0xivanov 0xivanov added this to the v2.51.0 milestone Nov 12, 2024
@robkainz
Copy link
Author

I just found time to look at this, and I believe that is exactly the issue. I had completely forgotten about this, but when I had abandoned wallet connect integration due to the poor state of the library a year ago, I had upgraded to this version of the sdk:

github.com/hashgraph/hedera-sdk-go/v2 v2.34.2-0.20240227140407-a8994c7f66aa

and spent substantial time working around the issue to make sure that background thread never got initialized (I still have this work in a branch). The problem is, I don't think that's possible with the newer SDKs which I tried tonight (2.49.0), because my workaround involved passing my own network config in hopes of never needing that not so "background" thread and I see that the call to client.go's ClientFromConfig() always results in a _NewClient instance, which uses the mirror node to schedule the network update via client._ScheduleNetworkUpdate(). I can't remember if version 2.34.2 did that or not offhand too, but now that I think of it, I remember testing the wallet connect version of Wallawallet was painfully slow (in dev environment connected directly to the phone, I never took it further than that anyway.)

I still have my own process that queries Hedera consensus nodes periodically to see if they are up and construct the config from that list. Is there anyway you can add/modify the API so that if a JSON config is provided there is no need for the address book update thread? The React Native bridge definitely doesn't like that.

@0xivanov
Copy link
Contributor

Is there anyway you can add/modify the API so that if a JSON config is provided there is no need for the address book update thread?

Yes, I will create a workaround and will release a beta version for you to test with.

@0xivanov
Copy link
Contributor

@robkainz Can you test using the new beta release v2.51.0-beta.1

@0xivanov 0xivanov reopened this Nov 18, 2024
@robkainz
Copy link
Author

I can test it this evening.

@robkainz
Copy link
Author

I spent substantial time on this today and here's what I found:

Line 161 in client.go's _NewClient() method introduces a substantial delay when executing _UpdateAddressBook() that can take more than 20 seconds to execute. It's much worse because I create a new client for every transaction, because I pass the network and transaction config to the native methods and keep track of node statuses on my own. There was never a long delay before to have to worry about in the older SDKs using this approach and this delay happens on every transaction now. In my case I don't need to _UpdateAddressBook() because I am providing the network config myself. When I commented out line 161, performance and responsiveness was much better. Acceptable, but not as perfect as I would like, because I tried rapidly refreshing balances and received 503 errors intermittently and sometimes "exceptional precheck status BUSY" as well. My old code (still in production) helped nearly eliminate those kinds of errors as Wallawallet would periodically refresh the network config from our server which maintained a list of nodes currently active.

Even with line 161 commented and overall reduced hangs, xcode still reports a priority inversion happening, where a higher-priority UI thread is waiting for a lower-priority thread (my calls over the React Native bridge to the go methods.) There are a number of ways to attempt to address this, both in React Native and Go, but some of them require substantial changes to the interfaces and new async methods in both React Native and Go and I won't be able to turn that around quickly if that turns out to be needed. The simpler changes didn't seem to work, though using a dispatch queue in the IOS code seemed to make it a tad more stable. But IMHO those changes shouldn't be necessary if the overhead for instantiating a new client was brought down to the bare minimum. The user experience is just awful with those _UpdateAddressBook() delays and in my case it affects every action a user takes requiring a client. Would it be possible to skip that update if the network config is provided?

@0xivanov
Copy link
Contributor

@robkainz Try using the new beta version v2.51.0-beta.2 and the new API ClientFromConfigWithoutScheduleNetworkUpdate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants