Optimize batched RPC lookups in P2P branch

[Pantamis]

Batched RPC can be resolved faster in P2P branch.

Indeed, each RPC will require loading one block: the one at the height given by an index lookup. But several requests may require looking at the same block ! Currently, each RPC will load the block needed once no matter what.

An interesting improvement could be to first get all needed block heights for each RPC, sort the requests by block height and load a block only once even if it is required for several RPCs. Once loaded we search all the transactions we are looking for in it and send the responses in the order at which we find them.

This is very similar to what is done when loading blocks with scripthash status sync where several blocks are loaded but one block may contain several relevant transactions.

However I think this may require significant refactoring of the code....

[romanz]

Good suggestion, thanks!

However I think this may require significant refactoring of the code....

Indeed, current code handles each request in the RPC batch independently:

electrs/src/electrum.rs

Lines 370 to 376 in ffab4f4

	Ok(requests) => match requests {
	Requests::Single(request) => self.call(client, request),
	Requests::Batch(requests) => json!(requests
	.into_iter()
	.map(|request| self.call(client, request))
	.collect::<Vec<Value>>()),
	},

[Pantamis]

The most insteresting use case I know would be this one: janoside/btc-rpc-explorer#356

This change could be also relevant if the stored index is changed to contain the position of the transaction in its block since this still requires parsing a full block for each request. We discuss this possibility with @Kixunil here (not sure if this change is worth it but it is interesting)

[Kixunil]

I had a similar idea but was thinking about it too much without writing. :D I think it may also be interesting in case many outputs of same transaction are requested - this transaction has to be loaded multiple times.

I was thinking that with async we could do this: instead of fetching data from db or bitcoind directly we create a future which registers request (as method+params; if there's already such request just add itself as another requester) and returns WouldBlock. At the join point in batch request - or even somehow right before call to select (to work across multiple requests issued at about the same time; not sure how/if we need to modify executor) we do the actual request of tasks and only return Ready after they are done. Perhaps something can be done to avoid waking those tasks during processing of current batch.

If done right, I think this could also decrease synchronization overhead and chance of deadlocks.

But I think we should profile it before attempting anything. :)

[Kixunil]

Couldn't stop thinking about it a bit more, no modifications of executor would be required. Instead first create resource handler wrapper (one for bitcoind and one for DB), which gets passed as &mut to all request handlers then call some method on it which will register it globally (to coordinate with other threads) and returns a future or nothing if a task handling the requests already exists. This future, if present, goes together with the remaining ones into FuturesUnordered.

[romanz]

@Pantamis Could you please try #600?

[Pantamis]

I tested 70025e4 with my watch only JoinMarket wallet (thousands of subscriptions) in sparrow, the difference is very significant ! The UX is so much better ! (even better than ElectrumX !)

I don't think the impact is caused by smarter blocks lookups but rather parallel subscriptions because the scripthashes rarely appear in the same block in my wallet.

I guess this is not a priority currently but it would be really nice to have parallel subscriptions with smart blocks lookups for outpoints subscriptions for personal block explorers, something to add in #454. Smart block lookup would be very powerful to load all spending transactions of outpoints of a big transaction in janoside/btc-rpc-explorer#356 because many such outpoints are often spend at the same block (in the 10 next blocks). Not having to re-parse the same block would have a great impact on the loading time.

[Kixunil]

@Pantamis that needs a spec change but I don't see why it wouldn't be made - it is very reasonable and simple.