search

Blockstream / electrs

An efficient re-implementation of Electrum Server in Rust
MIT License
318 stars 130 forks source link

Batching with array #108

Open RCasatta opened 1 month ago

RCasatta commented 1 month ago

Currently electrs support batching by separating requests via new lines, however, other impls support it also via json array as explained in the cited issue. This add support for batching requests via json array

close https://github.com/Blockstream/esplora/issues/516

shesek commented 3 weeks ago

Should we limit the number of requests per batch?

RCasatta commented 3 weeks ago

Should we limit the number of requests per batch?

Do we have limits for "new-line separated" batch requests? Is it different here? How much do you propose as the limit here?

shesek commented 2 weeks ago

Do we have limits for "new-line separated" batch requests? Is it different here?

We don't, but it is different here. New-lines separated requests are streamed, and individual responses are sent as they become available. We only hold up to 10 pending requests in the queue, then block and stop reading from the TCP socket until a pending request gets processed to free room (a bounded mpsc::sync_channel).

With array batching all the requests and all the response has to be buffered in-memory at once, which could use significant resources and be a DoS vector if there are no limits in place.

RCasatta commented 1 week ago

How much do you propose as the limit here

Do you think we can afford 20 as the limit for batch requests? I think it's common number because is used as gap limit...

shesek commented 5 days ago

20 seems fine, yes :+1:

However it seems the tests are failing. The failure isn't directly related to the batching but to the TestRunner used to test it.

RCasatta commented 4 days ago

I didn't realize locally because i launched the test singularly, I think we don't support having multiple concurrent tests. My solution is to make the test ignored by default and test it specifically.