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

Support batched JSONRPC requests #230

Closed
romanz opened this issue Apr 8, 2020 · 14 comments
Closed

Support batched JSONRPC requests #230

romanz opened this issue Apr 8, 2020 · 14 comments

Comments

@romanz
Copy link
Owner

romanz commented Apr 8, 2020

Following #228

@vlad20012
Copy link

What do you think about using https://github.com/paritytech/jsonrpc/tree/master/tcp ?

@romanz
Copy link
Owner Author

romanz commented Apr 16, 2020

Thanks for the suggestion, will take a look :)

@vesparny
Copy link

Hi, is this feature being considered for future implementation?
Batched requests would definitely give a performance boost, especially for wallets containing multiple transactions

@Kixunil
Copy link
Contributor

Kixunil commented Jan 12, 2021

@vesparny if you're asking because you're experiencing performance issues it could be actually caused by inefficiency in some electrs code. We were brainstorming about it in #308

If you have the time, you could check the next branch (and accompanied bitcoind fork) to see if your issue is fixed there. The branch still has to be changed but at least we would have more data.

@craigraw
Copy link

I believe that when Electrs is used over Tor (which is increasingly becoming the default), there is no improvement that will have a bigger impact on perceived performance. To illustrate this I created a custom build of Sparrow, loaded the same wallet twice and configured a connection to an ElectrumX server over Tor. One of the wallets used batched JSONRPC, the other did not. The difference was stark - the batched load completed in 7 secs while the non-batched in 68 seconds. Here are the loading logs - same wallet, same server:

batching

@Kixunil
Copy link
Contributor

Kixunil commented May 20, 2021

@craigraw I believe you're performing requests one-by-one, each issued only after previous completes, right? In that case it should be possible to send several requests in parallel to achieve better performance.

Of course I do agree that batching is just better. If parallel requests are easy to implement for you it might be a useful temporary solution.

@romanz
Copy link
Owner Author

romanz commented May 21, 2021

Please take a look at #405 :)

@craigraw
Copy link

For the purposes of detecting and switching to batched JSON-RPC, can the 0.9.0 version be used (from the client_name of the server.version request)?

@romanz
Copy link
Owner Author

romanz commented May 31, 2021

It's possible, but I would suggest sending a simple "batched" request to see if the server actually supports that :)
For example (note the [...] brackets):

$ echo '[{"jsonrpc": "2.0", "method": "server.ping", "id": 0}]' | netcat 127.0.0.1 50001
[{"id":0,"jsonrpc":"2.0","result":null}]

@Overtorment
Copy link

to my experience, you might accidentally crash an electrum server if you send unsupported request 😂

@romanz
Copy link
Owner Author

romanz commented May 31, 2021

It was the case, but it should be resolved by #398 - i.e. electrs 0.8.10 shouldn't crash when getting an unsupported request :)

$ echo '[{"jsonrpc": "2.0", "method": "server.ping", "id": 0}]' | netcat 127.0.0.1 50001 
{"error":{"code":-32700,"message":"parse error"},"id":null,"jsonrpc":"2.0"}

@romanz
Copy link
Owner Author

romanz commented Jun 2, 2021

Tested on latest https://github.com/sparrowwallet/sparrow version - seems to be working as expected :)

@romanz romanz closed this as completed Jun 2, 2021
@craigraw
Copy link

This is embarrassing to say, but adding the -rc* to the end of the Electrs version broke the batching detection in Sparrow. My bad!

I'll fix this going forward of course, but I'd greatly appreciate it if the 0.9.0 final release had no suffix so the current Sparrow release works. I'm sure this will be the case anyway. Thanks!

@romanz
Copy link
Owner Author

romanz commented Sep 25, 2021

No worries, and thanks for letting me know :)

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

6 participants