This is a POC, please don't review the code, just the approach. It needs to define the change format among other details.

Seems like this should probably use longpolling instead?

Pulling can also block until changes are available.

@luke-jr ☝️

Might be a good idea to optionally start with full getrawmempool results(if it's not already)

@instagibbs that's already the case, start fills the stream with add <hash> entries (don't mind the format) so a client never needs to call getrawmempool and compare/diff and sync with zmq notifications.

Longpolling would support multiple clients on a single buffer. So each request would get a new position id, and requesting with that position id would tell the node where in the shared buffer to begin with new data.

@luke-jr yes, it would be possible to share the buffer for all active clients. The problem is when a new client starts a stream, it needs to receive the current mempool and then resume from the shared buffer. I consider this an optimization.

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• Assert that RPCArg names are equal to CRPCCommand ones (blockchain,rawtransaction) #19849 (Assert that RPCArg names are equal to CRPCCommand ones (blockchain,rawtransaction) by MarcoFalke)
• [RPC] Add connection type to getpeerinfo, improve logs #19725 ([RPC] Add connection type to getpeerinfo, improve logs by amitiuttarwar)
• ZMQ: Create "sequence" notifier, enabling client-side mempool tracking #19572 (ZMQ: Create "sequence" notifier, enabling client-side mempool tracking by instagibbs)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Anyone interested in this may be interested in #19572 as well.

I promised a quick review after the developer IRC chat. My concerns would be:

If a client is struggling to handle a particular message type, it's not unimaginable that it might keep re-connecting for a new stream, getting a message and failing. Each time the MempoolChanges tracker gets abandoned by the client, but keeps accumulating new events until it hits the backlog-limit. The side-effects of not being able to tie the event stream to a particular client's auto-closable resource (ie. a particular TCP session) are wide ranging. It's likely we'd have to limit the number of currently open streams, and stop serving new mempoolchanges start requests, to avoid death by malloc fails. As we don't know how long it will take until the old streams die from overflowing (max depth/current event rate) what would be a sensible retry for a client in this situation? You can't guarantee evicting the oldest stream would help, as that might be a good consumer who is keeping up. So perhaps you'd have to walk the list of open sessions and evict the 'most-backlogged' when a new stream is requested and we are over limit. Which in itself is quite a bit of code to do and will always trigger with worst-case full lists to walk. Many magic parameters to tune.

Since there's no TCP session for each resource, how would I troubleshoot a bitcoind that's no longer responding to mempoolchanges start? With ZMQ there is a kernel max-backlog depth for accepting sockets, and I can use netstat or my OS tools of choice to find the culprit who is hogging the resources. With these anonymous sessions, I'd need some sort of a debug command in bitcoind to show me all the open MempoolChanges instances, with the last connecting endpoint's IP address/hostname, and the backlog for each for it to be of any use. An administrators nightmare.

There is also notably no isolation between streams. I can cause havoc (or at least, defeat the whole reliable nature of this scheme) by requesting an event from a different client's stream. The original client is then gaped and has no idea. You could defend against this by including the stream sequence number in the reply (so the client can detect gaps) and issuing the stream identifier numbers secure-randomly (to make them non-predictable), but these are both extra bytes you need to carry around on every request/response that a per-subscriber TCP stream does away with.

Thanks for the comments @shuckc

I'm not that worried about memory. Of course it would be a problem if you start lots of streams and you don't poll from them. We could limit the sum of all streams sizes. Anyway, internally streams could share the same data, see #19476 (comment).

Currently the client has to periodically request for changes, but the connection can be reused as we support multiple requests on the same connection. Long polling is not implemented, but once it's implemented it only reduces unnecessary empty requests.

I think this approach would be preferable in the REST interface with actual long polling, where the response doesn't actually finishes, but the client keeps reading them. But we don't have support in http server for this yet.

With these anonymous sessions, I'd need some sort of a debug command in bitcoind to show me all the open MempoolChanges instances, with the last connecting endpoint's IP address/hostname, and the backlog for each for it to be of any use.

I thought of mempoolchanges status where it would return the list and stream size.

There is also notably no isolation between streams. I can cause havoc (or at least, defeat the whole reliable nature of this scheme) by requesting an event from a different client's stream. The original client is then gaped and has no idea.

I'm assuming clients don't mess around with other streams, that's why the stream id isn't reused. I though about letting the client give a name for the stream (start foo) but meh.

but these are both extra bytes you need to carry around on every request/response that a per-subscriber TCP stream does away with.

A per-subscriber TCP stream isn't fault tolerant, that's the point here where the server holds the changes until that subscriber comes again.

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

Closing now that #19572 is merged.