cculianu / Fulcrum

A fast & nimble SPV Server for BCH, BTC, and LTC
Other
343 stars 77 forks source link

JSON-RPC batching? #73

Closed Overtorment closed 2 years ago

Overtorment commented 3 years ago

Seems that JSON-RPC batching (as described in spec) is not supported:


$ echo '{"id": 1, "method": "blockchain.scripthash.get_balance", "params": ["716decbe1660861c3d93906cb1d98ee68b154fd4d23aed9783859c1271b52a9c"]}' | timeout 2 openssl s_client -quiet  -connect electrumx.no-ip.eu:50002 2>/dev/null
{"id":1,"jsonrpc":"2.0","result":{"confirmed":51432,"unconfirmed":0}}

$ echo '[{"id": 1, "method": "blockchain.scripthash.get_balance", "params": ["716decbe1660861c3d93906cb1d98ee68b154fd4d23aed9783859c1271b52a9c"]}]' | timeout 2 openssl s_client -quiet  -connect electrumx.no-ip.eu:50002 2>/dev/null
{"error":{"code":-31999,"message":"Json Error: expected object"},"id":null,"jsonrpc":"2.0"}

version tested:

$ echo '{"id": 1, "method": "server.version", "params": []}' | timeout 2 openssl s_client -quiet  -connect electrumx.no-ip.eu:50002  2>/dev/null
{"id":1,"jsonrpc":"2.0","result":["Fulcrum 1.4.0","1.4"]}
Overtorment commented 3 years ago

https://www.jsonrpc.org/specification#batch

cculianu commented 3 years ago

That's right, it's not supported.. sorry. Just send the requests 1 at a time.

Overtorment commented 3 years ago

https://github.com/BlueWallet/BlueWallet/pull/2495

cculianu commented 3 years ago

Yeah sorry about that. Batching wasn't enabled because it was felt that it could be abused by clients. The way the DoS controls work in fulcrum made it harder to prevent overloading the server if batching was enabled, so it has been disabled.

Sorry about that. Thanks for the workaround. I'll see if I can add it later. Will leave this issue open and re-label it.

craigraw commented 2 years ago

+1

I made a video indicating the effect of batching when loading a wallet using ElectrumX - same wallet, same server. Could it at least be made a configuration option?

batching

cculianu commented 2 years ago

Oh, so I guess the loop on the right is, in pseudocode:

  for (tx in txs)
    req = get_from_server(tx.id)
    wait_for_response(req)

Or something like that. Basically you don't move on to requesting the next tx until the previous one is done? You could just send a flurry of these requests one after the other (assuming your app's logic would work ok with that) -- that would be much faster. And then Fulcrum will reply to all the queued requests in time as it can. This should have the same effect as batching, more or less, even if less ergonomic.

I do see the issue here in that some smaller apps are harder to write without batching. Hmm. I'll ponder this. It would be a bit of an effort to support batching since it would require some parts be redesigned. But I'll think about how to accomplish it and perhaps implement it soon.

craigraw commented 2 years ago

Yes, the pseudocode is correct. I think a batched request would be more efficient than a bunch of single requests, particularly when communicating over a slow protocol like Tor. I suspect that in the example, the data transmission overhead is where most of the time is going.

cculianu commented 2 years ago

Yes, in that situation a batch is superior, for sure. Ok, I'll think on how to do this without introducing DoS nightmares. Hopefully the solution will be elegant, obvious, and free of regressions. :)

I may not get to it for a couple of weeks but I do consider this a priority now. Thanks for the upvote on this.

pajasevi commented 2 years ago

Hint: add a configuration option (maybe with some reasonable default) to set maximum number or requests batched per RPC message.

cculianu commented 2 years ago

Yeah good idea. Also max number of "outstanding concurrent batches". Like I don't want 1 client submitting 16 batches each of 100 requests.. monopolizing things. What do you think a reasonable default limit would be both for:

?

i almost want to make the latter one (concurrent batches) be 1 per connection, by default max (configurable to any value)... or something on that order.. thoughts?

pajasevi commented 2 years ago

I think that "requests-per-batch" might be difficult to set reasonably because that really depends on HW specs and number of clients. So I would pick an arbitrary number 100 as a default just because...

As for the "max concurrent batches", I agree that making it 1 per connection is the most clear and easy to reason about. Hard to say if some clients won't have a problem with that of course. I guess time and Github issues will tell 😉

cculianu commented 2 years ago

Hey so I implemented batching in a branch, as a work-in-progress. It has no logic to prevent flooding/DoS (yet) but it does work in my testing and is fast. It's always-on in this branch (I may make that configurable later after testing is complete).

Would you mind terribly if I asked you to build this branch: https://github.com/cculianu/Fulcrum/tree/wip_batch , and run and test it with your setup, @craigraw (or anybody else in this thread)?

Let me know if you want me to post pre-built static binaries if building for you is onerous/confusing/not-what-you-normally-do.

craigraw commented 2 years ago

Great news! I compiled Fulcrum on an RPi4 and have been indexing since, currently at 69% after 40 hours. I will recompile the branch as soon as it's done.

cculianu commented 2 years ago

Ok, I just merged in the changes from my wip_batch branch to master. I am closing this issue now since batching now is implemented.

craigraw commented 2 years ago

I've completed benchmarking of Electrs, ElectrumX and Fulcrum, and the results for Fulcrum are very impressive. To subscribe (and check) all addresses, Fulcrum is 22x faster than ElectrumX, ~300x faster than Electrs. To refresh (load all data from scratch), Fulcrum is 8x faster than ElectrumX, 1.5x faster than Electrs. This on an RPi4, ~3000 address wallet.

I'm greatly looking forward to the 1.6.0 release to update my server benchmark report. Particularly if an RPi-compatible binary is available, there would seem to be little reason to continue to prefer Electrs on popular node packages and guides.

Edit: Completely missed that the 1.6.0 release is already out! Will publish my results shortly.

pajasevi commented 2 years ago

Thank you @craigraw for doing the benchmarking. It looks like @cculianu has really done an amazing work and I can't wait to try Fulcrum.

cculianu commented 2 years ago

@craigraw Thanks for doing the benchmarking, and all very positive numbers for Fulcrum!

Just FYI -- 1.6.0 has been released. However, I didn't have time to create the RPi static build. You will have to still build it yourself (yet you did manage to do it before so.. should be fine).

craigraw commented 2 years ago

Updated server performance report, now including Fulcrum: https://www.sparrowwallet.com/docs/server-performance.html. As you can see, I am going to be recommending Fulcrum to Sparrow users going forward.

The Sparrow release with support for batching in Fulcrum (essentially detecting the 1.6.0 release to enable it) will be released later this week.

cculianu commented 2 years ago

Hey @craigraw that's great news! Yes, you will need to auto-detect server version to be Fulcrum >= 1.6.0 for sure.

I just had a look at the performance document. Very well written! I want to say that I just posted arm64 binaries to my release page (should run fine on a 64-bit OS Raspberry Pi that is Ubuntu-18 or newer). You may want to update that section for Fulcrum that talks about that which says "coming soon" to actually say that arm64 binaries are now available. They can be downloaded from the release page here: https://github.com/cculianu/Fulcrum/releases/tag/v1.6.0

cculianu commented 2 years ago

Hey another note -- Fulcrum also uses that tx_num scheme that ElectrumX does. It's a brilliant way to save on data (you don't need to store a full hash, just an int, in a scripthash's history), but you are still be able to look things up quickly! The Fulcrum schema is described here, in a very succint/brief/perhaps not as in-depth way as it could be: https://github.com/cculianu/Fulcrum/blob/master/src/Storage.h#L446

craigraw commented 2 years ago

Thanks, I've updated the doc both for the binaries and to include Fulcrum in the tx_num description. It's interesting to note that Fulcrum stores the full 32 script hash bytes in its scripthash_history - no false positives!

cculianu commented 2 years ago

Yeah believe it or not the false positive thing bit me once on one of my wallets (I have a wallet with like 25k addresses in it). I use Electron Cash and it quickly realizes it's bogus but that means each time it starts up it needs to re-dl the history fully because the scritphash_status is bogus. It was a pet peeve of mine so I insisted on doing it that way, ha ha. It does eat up some extra GBs of space.. but not as bad as it may appear initially due to the way rocksdb allocates tables (you end up wasting a minimal amount of space anyway for keys, so if the keys are below a minimum size the savings is not substantial in rocksdb anyway).