Introduce Zmq and interrupt the main loop wait if zmq is setup and a block hash notification is received #111
Conversation
RCasatta
force-pushed
the
zmq
branch
2 times, most recently
from
73a70a4
to
bfb2985
Compare
There's an WIP implementation of this I wrote awhile ago in this branch. It has basic working functionality for updating the mempool view with individual transactions as they arrive from zmq rather than doing full periodic updates, but is still incomplete and missing some important optimizations (in particular for updating the mempool scripthash history indexes, which was already inefficient but is in a much hotter code patch with this change).
This can be independent of a tx cache if you patch the mempool view directly with new transactions, instead of keeping them around but only applying them on the next periodic update. My understanding is that in the current blockstream.info setup, there's a ZMQ client running alongside electrs that receives Given this, is there much of an advantage to moving the I do see a significant advantage to using zmq for updating the mempool with individual transactions, both for performance and to keep the electrs mempool view more closely in-sync with bitcoind's, without the latency introduced by periodic updates. However if that's our goal, perhaps we should aim straight for an MR that includes it too? I'm not sure that integrating zmq is worth it just for block updates, and it might be beneficial to design the zmq integration with mempool updates in mind from the get go. |
Yes, so we can get rid of a service in the infra written in go that listen to zmq and send signals, it simplify the infra to have this logic in electrs instead.
We would need zmq thread similar to this for it? This seems a step in that direction and also useful to get rid of a part of the infra (the service with the purpose of sending signals). But I am fine also if we want to go with the branch instead (adding also the wakeup in the loop) if it can be done in a reasonable timeframe. |
Right, we do. If we do just blocks subscription, then there are some things I slightly prefer in the branch (using an optional feature flag and the ZmqSyncer encapsulation) but none are critical and we could go with this PR too.
In the branch there no longer is a sync loop when ZMQ is enabled, it relies on the subscriptions instead of doing periodic updates:
Well define reasonable :) ZMQ-based mempool syncing is quite more involved than just block subscription so it would take longer to get right and require more throughout testing. The upside is big and worth it imo, but if doing just block subscription to get rid of the zmq service is useful on its own then I wouldn't hold it up for that. |
src/bin/electrs.rs
Outdated
| // We are going to wait 5 secs (50*100ms) if nothings happens. | ||
| // We are stopping if we receive a TERM or INT signal. | ||
| // We are interrupting the wait if we receive a block hash notification (if zmq enabled) or receiving a USR1 signal. | ||
| for _ in 0..50 { |
There was a problem hiding this comment.
Perhaps we could use crossbeam_channel's select! to wait for the first receiver that emits, instead of polling block_hash_receive on an interval?
Also, this could be made part of Waiter::wait(), with accept_sigusr changed into accept_new_block_notif to cover both cases.
There was a problem hiding this comment.
Changed to use select! in latest force push which is also rebased.
Note that since we want to use select! I removed previous latest commit, introduced select! so that one branch is the timeout, then added the zmq branch
this commit aim to achieve the same behaviour as before but using the select! construct instead of recv_timeout because in the following commits we want to add a case to the select!
src/new_index/zmq.rs
Outdated
|
|
||
| use crate::util::spawn_thread; | ||
|
|
||
| pub fn start(url: &str, block_hash_notify: Option<Sender<BlockHash>>) { |
There was a problem hiding this comment.
Does block_hash_notify need to be an Option?
src/signal.rs
Outdated
| } | ||
| } | ||
| pub fn start() -> (Sender<BlockHash>, Waiter) { | ||
| let (block_hash_notify, block_hash_receive) = channel::bounded(1); |
There was a problem hiding this comment.
It feels more natural to me to create the Sender outside and accept it as an argument, but I don't feel strongly about this
In a follow up MR we can also listen to transactions from zmq so that the mempool update already finds transactions and doesn't need to fetch most of them from the node (probably needs a tx cache #110 )