Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

zmq: mempool notifications #7753

Closed

Conversation

@laanwj
Copy link
Member

laanwj commented Mar 27, 2016

Add notifications when transactions enter or leave the mempool.

These can be enabled with zmqpubmempool:

  • mempooladded: a transaction was added to the mempool
  • mempoolremoved: a transaction was removed from the mempool

This allows third-party software to keep track of what is in the mempool in real time. For example a mempool monitor I'm working on:
schermafdruk van 2016-03-27 17-42-16
It also makes it possible to keep statistics on why transactions disappear from the mempool.

Full documentation: https://github.com/bitcoin/bitcoin/blob/325afe68381d59f1b95a690927c4a8ea886ac791/doc/zmq.md#mempooladd

See individual commits for details.

Current issues:

  • Warning NotifyEntryAdded: WARNING: zmq send failed with code -1 spam in log if "-zmqpubmempool" is not enabled.
@laanwj laanwj added the RPC/REST/ZMQ label Mar 27, 2016
@laanwj laanwj force-pushed the laanwj:2016_03_zmq_mempool_notifications branch to 4e3039b Mar 27, 2016
@promag

This comment has been minimized.

Copy link
Member

promag commented Mar 27, 2016

I was planning to submit a pull request to change this kind of notifications. The current solution is not very elegant and doesn't scale. The command line arguments should be clean.

So if you don't mind I'll try to explain here the idea using this example:

Usage: -notify "mempool ! json ! zmqpub address=tcp://127.0.0.1:28332"

Here the notify argument creates one notification pipeline composed by one source of events (mempool), one filter element to transform the event in json and one sink of events (zmqpub), all connected with the operator !. There can be more than one notifier configured.

Is this design, zmq elements are concrete types of sinks. But we could have more, like log to file or pub to nsq queue.

We could have chain and wallet sources for instance. We could have filters to transform to other types.

WDYT?

@dcousens

This comment has been minimized.

Copy link
Contributor

dcousens commented Mar 27, 2016

concept ACK

@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented Mar 28, 2016

@promag I'm all for making it more flexible, but what you're proposing sounds like the gstreamer syntax and that's IMO even harder to use than the current arguments. I've needed to do a few media pipelines in my PhD research but never managed to really master it.

Our basic idea with the ZMQ notifications is that one notification system exists. From there on everything (e.g. forward to the notification system of your choice) can be handled by external software. I don't want too much notification-related complexity in core.

Aside, this is the wrong place to discuss this. Let's focus on the software change here. Please create a new issue.

@promag

This comment has been minimized.

Copy link
Member

promag commented Mar 28, 2016

@laanwj yes gstreamer all over the comment. I'll move the discussion to a new issue.

utACK.

@@ -1230,7 +1230,7 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler)
AddOneShot(strDest);

#if ENABLE_ZMQ
pzmqNotificationInterface = CZMQNotificationInterface::CreateWithArguments(mapArgs);
pzmqNotificationInterface = CZMQNotificationInterface::CreateWithArguments(mapArgs, &mempool);

This comment has been minimized.

Copy link
@promag

promag Mar 28, 2016

Member

Feels a bit hacky.

This comment has been minimized.

Copy link
@laanwj

laanwj Mar 28, 2016

Author Member

Better suggestions?
As I see ti there are two alternatives:

  • use the global mempool object (ugly)
  • pass the mempool in (slightly les ugly)

Another option would be to move the instantiation of the CZMQPublishMempoolNotifier(mempool) here. But I'd like as little zmq specific code in init. But it may be the least ugly option.

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Mar 28, 2016

Member

I think this implementation approach is reasonable and I also don't see a more elegant way to do this.

This comment has been minimized.

Copy link
@promag

promag Mar 28, 2016

Member

Then I would rename CreateWithArguments to just Create and move the following from src/init.cpp to Create.

if (pzmqNotificationInterface) {
    RegisterValidationInterface(pzmqNotificationInterface);
}

Edit:
Because in your notifier you connect the signals so it also makes sense to connect the remaining signals here.

This comment has been minimized.

Copy link
@laanwj

laanwj Apr 5, 2016

Author Member

I agree, will move that line.

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Mar 28, 2016

Awesome!
Nice work! I hope you have also plans to opensource the ncurses UI.

Some ideas / conceptual questions:

  • How could a listener keep track of the overall mempool size without listening to a fresh start (listening from the beginning when the mempool was at a size of 0), also there is a risk of a delta in case you missed a notification in case you calculate the mempool overall size on the "listeners side".
  • would a notification after a significant (every X MB, every X transaction added/removed) in the mempool size or structure make sense?
  • Adding the total mempool size in every mempool notification is probably to much?
@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Mar 28, 2016

Code Review utACK.

@promag

This comment has been minimized.

Copy link
Member

promag commented Mar 28, 2016

Adding the total mempool size in every mempool notification is probably to much?

👍

@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented Mar 29, 2016

How could a listener keep track of the overall mempool size without listening to a fresh start (listening from the beginning when the mempool was at a size of 0), also there is a risk of a delta in case you missed a notification in case you calculate the mempool overall size on the "listeners side".

What if you use the following sequence:

  • Start listening (queue up events)
  • Request getrawmempool
  • Start applying events

You may have some duplicates in the first run, e.g., adds which already show up in the mempool requested, removes which were already removed, but I think due to the idempotent nature of the events (just add and remove) the overall state will be the same?

there is a risk of a delta in case you missed a notification

Missing notifications are a possibiltiy with zmq. But I'd say this is a matter of sizing buffers appropriately, and dedicating a thread to listening to zmq requests.

To reliably detect missing notifications we could add sequence numbers to the events, as I think zmq guarantees that events from a single source are delivered in order.

Then again if we do this we probably want it for all the event types... I actually suggested it for the first zmq proposal.

Adding the total mempool size in every mempool notification is probably to much?

I thought about sending a generic stats packet with every mempool event, but as you can receive tons of them I don't want to make the individual events too big and include things you can compute yourself. But no problems with just the size / # transactions.

Nice work! I hope you have also plans to opensource the ncurses UI.

Absolutely, I'll open source it soon, will be part of a bc-monitor repo. Eventually I hope to make a tool like tor-arm (just renamed to nyx IIRC) which allows watching all aspects of a node.

@laanwj laanwj force-pushed the laanwj:2016_03_zmq_mempool_notifications branch to 4e3039b Mar 29, 2016
@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Mar 29, 2016

Tested ACK 4e3039b
I also think the addNotifier()-refactor makes it much cleaner.

@paveljanik

This comment has been minimized.

Copy link
Contributor

paveljanik commented Mar 30, 2016

lightly tested ACK 4e3039b

/** Reason why a transaction was removed from the mempool,
* this is passed to the notification signal.
*/
enum MemPoolRemovalReason {

This comment has been minimized.

Copy link
@sipa

sipa Apr 5, 2016

Member

This enum seems to be part of the ZMQ API, so we'll probably want some stability across versions. Things like MPR_EXPIRY and MPR_SIZELIMIT seem quite specific to the existing implementation.

LogPrint("zmq", "zmq: mempool entry removed: %s, reason %i\n", entry.GetTx().GetHash().ToString(), (int)reason);
uint8_t data[32 + 1];
WriteHash(&data[0], entry.GetTx().GetHash());
data[32] = reason;

This comment has been minimized.

Copy link
@sipa

sipa Apr 5, 2016

Member

I would prefer it if the value of the (internal) MemPoolRemovalReason did not leak into the ZMQ protocol, though that can be fixed later by introducing a new enum in the ZMQ definition.

This comment has been minimized.

Copy link
@laanwj

laanwj Apr 5, 2016

Author Member

Yes, I'll add another enum here and add a mapping.

@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented Apr 6, 2016

Ok added two commits:

  • The first adds a (currently, one to one) mapping between the mempool reasons from the mempool and those on the ZMQ interface.
  • The second is a slight refactoring to make all ZMQ publish notifiers register (and unregister) themselves for the events that they want to receive, instead of relying on the events being propagated manually though CZMQNotificationInterface @promag
@laanwj laanwj force-pushed the laanwj:2016_03_zmq_mempool_notifications branch Apr 19, 2016
@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented Apr 19, 2016

Rebased after #7762

laanwj added 5 commits Mar 26, 2016
Add notification signals to make it possible to subscribe to mempool
changes:

- NotifyEntryAdded(const CTxMemPoolEntry &)>
- NotifyEntryRemoved(const CTxMemPoolEntry &, MemPoolRemovalReason)>

Also add a mempool removal reason enumeration, which is passed to the
removed notification based on why the transaction was removed from
the mempool.
Replace factories list with calls to a function.

This simplifies the code (every notifier is only created once anyway),
and makes it possible to pass arguments to notifier contructors.
Add notifications when transactions enter or leave the mempool.

These can be enabled with `pubmempool`:

- `mempooladded`: a transaction was added to the mempool
- `mempoolremoved`: a transaction was removed from the mempool

This allows third-party software to keep track of what is in the mempool
in real time.
Make the registration process less circuitous by making
the notifiers listen to their appropriate events themselves
@laanwj laanwj force-pushed the laanwj:2016_03_zmq_mempool_notifications branch to 1bbe366 May 12, 2016
@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented May 12, 2016

Rebased, and changed the enums (both for the mempool and for the zmq protocol) to c++11 enums.

@paveljanik

This comment has been minimized.

Copy link
Contributor

paveljanik commented May 12, 2016

lightly tested ACK 1bbe366

@sipa

This comment has been minimized.

Copy link
Member

sipa commented May 25, 2016

Needs rebase.

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Jun 10, 2016

Still needs rebase.

@laanwj laanwj added this to the Future milestone Jun 13, 2016
@jmcorgan

This comment has been minimized.

Copy link
Contributor

jmcorgan commented Aug 19, 2016

ACK. I have a rebased version of this branch at 'jmcorgan/2016_03_zmq_mempool_notifications'.

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Aug 19, 2016

@jmcorgan feel free to open a PR of your rebases version and take over the lead.

@jmcorgan

This comment has been minimized.

Copy link
Contributor

jmcorgan commented Aug 19, 2016

Continued in #8549

@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented Oct 18, 2016

Closing in favor of #8549

@laanwj laanwj closed this Oct 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.