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

inefficient calls to electrum #72

Open
zoedberg opened this issue Oct 2, 2024 · 2 comments
Open

inefficient calls to electrum #72

zoedberg opened this issue Oct 2, 2024 · 2 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@zoedberg
Copy link
Contributor

zoedberg commented Oct 2, 2024

When using electrum as indexer, the service logs the following warning repeatedly:

your wallet uses less efficient method of querying electrs, consider contacting the developer of your wallet. Reason: blockchain.scripthash.get_history called for unsubscribed scripthash

To address this we should use the subscription feature and get histories only for scripts with updates.
To implement this we could store a map (in memory) and call script_subscribe/script_pop to update it. Then we should call script_get_history only if the hash returned by these 2 calls has changed from the last time we synced the wallet. Since script_subscribe should be called only once I think the cleaner way to address this is by implementing the Indexer::update method (which currently is unimplemented). This way the Indexer::create method should just call script_subscribe (and script_get_history) for all addresses once, while subsequent wallet syncs would go through Indexer::update and call script_pop (and call script_get_history only where necessary).

Otherwise, if there are no plans in the near future to implement Indexer::update we can implement the logic to check if we already subscribed to a script directly in the Indexer::create.

@dr-orlovsky please let me know what do you think, I can work on this if you prefer

@dr-orlovsky
Copy link
Member

Yes, I left the work on update with indexers on after v0.11 release, since it doesn't introduce any breaks and can be done in v0.11.1, v0.11.2 etc. But if you have time now, you are of course welcome.

I already asked Bitlight Labs help on the issue and they did #67 which is now needs re-base. Maybe you can jointly work on that PR or use it in your work

@dr-orlovsky dr-orlovsky added the enhancement New feature or request label Oct 3, 2024
@dr-orlovsky dr-orlovsky added this to the v0.11.x milestone Oct 3, 2024
@zoedberg
Copy link
Contributor Author

zoedberg commented Oct 3, 2024

#67 doesn't fix the issue reported here (i.e. the inefficient usage of electrum APIs) and introduces a layer of cache that I think could be quite dangerous (caching is evil and very hard to implement correctly). I think we should use a cache only when there are no alternatives.

In case of the electrum APIs the script_subscribe and script_pop methods allow to detect changes and so they allow to call script_get_history only when there are updates, so the part "Caches the last history response for each address to determine changes" in the PR's description I think should be addressed differently. Anyway I'm happy to work with the Bitlight team to find the best solution and to better understand the rationales of the proposed changes.

@will-bitlight would you like to work together on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: No status
Development

No branches or pull requests

2 participants