ZMQ: add options to configure outbound message high water mark, aka SNDHWM

mruddy commented at 7:18 am on August 25, 2018: contributor

This is my attempt at https://github.com/bitcoin/bitcoin/pull/13315

fanquake added the label RPC/REST/ZMQ on Aug 25, 2018

mruddy force-pushed on Aug 25, 2018

in src/init.cpp:449 in 445559f926 outdated

442@@ -443,11 +443,19 @@ void SetupServerArgs()
443     gArgs.AddArg("-zmqpubhashtx=<address>", "Enable publish hash transaction in <address>", false, OptionsCategory::ZMQ);
444     gArgs.AddArg("-zmqpubrawblock=<address>", "Enable publish raw block in <address>", false, OptionsCategory::ZMQ);
445     gArgs.AddArg("-zmqpubrawtx=<address>", "Enable publish raw transaction in <address>", false, OptionsCategory::ZMQ);
446+    gArgs.AddArg("-zmqpubhashblockhwm=<n>", strprintf("Set publish hash block outbound message high water mark (default: %d)", DEFAULT_ZMQ_SNDHWM), false, OptionsCategory::ZMQ);
447+    gArgs.AddArg("-zmqpubhashtxhwm=<n>", strprintf("Set publish hash transaction outbound message high water mark (default: %d)", DEFAULT_ZMQ_SNDHWM), false, OptionsCategory::ZMQ);
448+    gArgs.AddArg("-zmqpubrawblockhwm=<n>", strprintf("Set publish raw block outbound message high water mark (default: %d)", DEFAULT_ZMQ_SNDHWM), false, OptionsCategory::ZMQ);
449+    gArgs.AddArg("-zmqpubrawtxhwm=<n>", strprintf("Set publish raw transaction outbound message high water mark (default: %d)", DEFAULT_ZMQ_SNDHWM), false, OptionsCategory::ZMQ);

domob1812 commented at 10:05 am on August 25, 2018:

I don’t really like the duplication here (one argument for each publisher cross (address, hwm)), but I think it is probably the easiest solution. At least I don’t see a good alternative that would not be over-engineered if we want the HWM configurable for each publisher separately.

mruddy commented at 10:30 am on August 25, 2018:

yeah, i figured that at least it fit a pattern and was easy to code and review. unless someone has a better idea, i think i’ll leave it simple.

reel commented at 11:19 am on August 25, 2018:

Maybe one flag to set them all would be interesting?

mruddy commented at 12:37 pm on August 25, 2018:

that was how it was originally done in the other PR and then someone wanted it broken out i believe. it makes sense to me to have them separate because the tx publishes definitely need way more message capacity than the block pubs.

domob1812 commented at 4:18 pm on August 25, 2018:

Perhaps there could be just two (not four) HWMs, one for tx (raw and hash) and one for blocks? (But that’s just a thought - having all four is totally fine for me if someone has use for separate values.)

mruddy commented at 4:29 pm on August 25, 2018:

there is still a big potential message size difference between just hash and raw{block,tx} so i think having them separate still is better. it just gives people the most flexibility. btw, thanks for the review!

luke-jr commented at 12:49 pm on August 30, 2018:

-zmqpubrawtx=<address>;hwm=<hwm> perhaps?

promag commented at 4:45 pm on August 30, 2018:

@luke-jr FYI #14060 (comment)

domob1812 commented at 10:05 am on August 25, 2018: contributor

It might also make sense to return the configured HWM in getzmqnotifications.

DrahtBot commented at 10:08 am on August 25, 2018: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#13686 (ZMQ: Small cleanups in the ZMQ code by domob1812)
#13315 (configurable ZMQ message limit by FeedTheWeb)

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.

mruddy force-pushed on Aug 25, 2018

mruddy commented at 10:29 am on August 25, 2018: contributor

@domob1812 sure, added hwm to getzmqnotifications

mruddy force-pushed on Aug 25, 2018

promag commented at 10:59 am on August 25, 2018: member

Shouldn’t set the option per address, not per notification type?

mruddy force-pushed on Aug 25, 2018

mruddy commented at 12:36 pm on August 25, 2018: contributor

@promag there can only be one of each of the notification types and each can have only one address. i updated the messages to show the address too. while i was doing that, i also made all of the zmq log messages prefix with “zmq: " because some were missing that.

mruddy commented at 2:07 pm on August 25, 2018: contributor

for what it’s worth, i tested synching a bitcoin core node that was 5 days behind with a zmqpubhashtx tcp socket that PUBlished to a ODROID-C1+ SUBscriber across town (with roughly 15-20ms ping RTT) and -zmqpubhashtxhwm=20000 allowed me to get no gaps in transaction hashes. i had gaps (i.e.- dropped messages) with zmqpubhashtxhwm=10000. this is by no means scientific, but does give some idea of what kinds of message backlogs people could possibly be dealing with.

in src/zmq/zmqnotificationinterface.cpp:106 in 23cff8c08c outdated

 99@@ -98,11 +100,11 @@ bool CZMQNotificationInterface::Initialize()
100         CZMQAbstractNotifier *notifier = *i;
101         if (notifier->Initialize(pcontext))
102         {
103-            LogPrint(BCLog::ZMQ, "  Notifier %s ready (address = %s)\n", notifier->GetType(), notifier->GetAddress());
104+            LogPrint(BCLog::ZMQ, "zmq: Notifier %s ready (address = %s)\n", notifier->GetType(), notifier->GetAddress());

domob1812 commented at 4:19 pm on August 25, 2018:

I think this change makes sense - but it is unrelated to your other change, so perhaps move it at least to a separate commit?

mruddy commented at 4:27 pm on August 25, 2018:

like a separate pull request? i know what you mean, and for a personal repo i would, but my past experience here is that if i have more than one commit, nothing will get merged until i squash it down to one.

promag commented at 9:57 pm on August 27, 2018:

That’s not true, just look at merged PR’s. In this case I agree that a commit with this change is fine. However I would prefer to have it in a separate PR, precisely to avoid noise like this. Generally if a commit is not required in a PR, then I push it in a different PR.

promag commented at 10:12 pm on August 27, 2018: member

@promag there can only be one of each of the notification types and each can have only one address. @mruddy right, at some point I was going to add support for multiple notifications of the same type but for some reason I don’t remember why I quit that, so that got me confused.

My suggestion for the long term is to extend the existing argument, like:

0-zmqpubhashtx=tcp://127.0.0.1:28332,hwm=20000,foo=bar,...

And this would allow multiple pubhashtx with different high water mark and other options.

Given the above, for now, a global/default hwm looks enough, and the above could be implemented later.

in src/zmq/zmqabstractnotifier.h:50 in 23cff8c08c outdated

40@@ -39,6 +41,7 @@ class CZMQAbstractNotifier
41     void *psocket;
42     std::string type;
43     std::string address;
44+    int outbound_message_high_water_mark; // aka SNDHWM

practicalswift commented at 7:33 am on September 5, 2018:

outbound_message_high_water_mark not initialized in the constructor?

mruddy commented at 10:29 pm on September 11, 2018:

thanks. it was set, just not by the constructor. i changed things around to address that. i also found that CZMQAbstractPublishNotifier.nSequence was not initialized, so i initialized that too now.

mruddy force-pushed on Sep 11, 2018

mruddy force-pushed on Sep 17, 2018

mruddy commented at 1:15 pm on September 17, 2018: contributor

rebased and updated the functional test just a moment ago

mruddy force-pushed on Sep 20, 2018

mruddy commented at 10:37 am on September 20, 2018: contributor

rebased and added a comma in the getzmqnotifications help message

in src/zmq/zmqabstractnotifier.h:35 in 9ac2f9468f outdated

29@@ -28,6 +30,12 @@ class CZMQAbstractNotifier
30     void SetType(const std::string &t) { type = t; }
31     std::string GetAddress() const { return address; }
32     void SetAddress(const std::string &a) { address = a; }
33+    int GetOutboundMessageHighWaterMark() const { return outbound_message_high_water_mark; }
34+    void SetOutboundMessageHighWaterMark(const int sndhwm) {
35+        if ((sndhwm >= 0) && (sndhwm <= std::numeric_limits<int>::max())) {

practicalswift commented at 7:43 pm on September 25, 2018:

02018-09-25 21:25:16 clang(pr=14060): zmq/zmqabstractnotifier.h:35:38: warning: result of comparison 'const int' <= 2147483647 is always true [-Wtautological-type-limit-compare]

Use static_assert instead?

mruddy commented at 0:11 am on September 26, 2018:

ok, i’ll just check it like zmq checks it: https://github.com/zeromq/libzmq/blob/master/src/options.cpp#L310-L315

also rebased. thanks!

in src/zmq/zmqnotificationinterface.cpp:62 in 9ac2f9468f outdated

58@@ -59,6 +59,7 @@ CZMQNotificationInterface* CZMQNotificationInterface::Create()
59             CZMQAbstractNotifier *notifier = factory();
60             notifier->SetType(entry.first);
61             notifier->SetAddress(address);
62+            notifier->SetOutboundMessageHighWaterMark(gArgs.GetArg(arg + "hwm", CZMQAbstractNotifier::DEFAULT_ZMQ_SNDHWM));

practicalswift commented at 7:44 pm on September 25, 2018:

02018-09-25 21:25:16 clang(pr=14060): zmq/zmqnotificationinterface.cpp:62:61: warning: implicit conversion loses integer precision: 'int64_t' (aka 'long') to 'int' [-Wshorten-64-to-32]

mruddy commented at 0:11 am on September 26, 2018:

explicit cast added

mruddy force-pushed on Sep 26, 2018

DrahtBot added the label Needs rebase on Oct 8, 2018

mruddy force-pushed on Oct 12, 2018

mruddy commented at 11:46 am on October 12, 2018: contributor

rebased

sipa commented at 7:45 pm on October 12, 2018: member

Will this make ZMQ publishing blocking on the bitcoind side when used? If so, that needs clear documentation.

mruddy commented at 10:08 am on October 13, 2018: contributor

Will this make ZMQ publishing blocking on the bitcoind side when used? If so, that needs clear documentation.

No, it will not. To add more color, without this PR, the SNDHWM is 1000. These new options just allow changing that value to something else. The drop/block behavior does not change. The motivation for this change was dropped messages. And from the ZMQ API docs, we see that the documented behavior is to drop (which is what we want, and what was experienced as the motivation): ‘When a ZMQ_PUB socket enters the mute state due to having reached the high water mark for a subscriber, then any messages that would be sent to the subscriber in question shall instead be dropped until the mute state ends."

jmcorgan commented at 10:00 pm on October 18, 2018: contributor

ACK

ZMQ: add options to configure outbound message high water mark, aka SNDHWM a4edb168b6

mruddy force-pushed on Oct 19, 2018

mruddy commented at 11:42 am on October 19, 2018: contributor

@jmcorgan thanks for reviewing! also i just wanted to mention that i just removed a single whitespace in the help text for getzmqnotifications to make the member descriptions align. that’s the only change for the latest commit.

DrahtBot removed the label Needs rebase on Oct 20, 2018

reel commented at 4:06 pm on October 20, 2018: none

Tested. Works great

laanwj merged this on Nov 5, 2018

laanwj closed this on Nov 5, 2018

laanwj referenced this in commit dac2caa371 on Nov 5, 2018

mruddy deleted the branch on Nov 5, 2018

mruddy referenced this in commit 86cddf0856 on Nov 10, 2018

laanwj referenced this in commit 2e8f9dc2bb on Nov 14, 2018

jfhk referenced this in commit 4b50a1d782 on Nov 14, 2018

JeremyRubin referenced this in commit c17b9fba08 on Nov 24, 2018

HashUnlimited referenced this in commit 37c6228c2c on Nov 26, 2018

Sjors commented at 6:17 pm on December 8, 2018: member

Came across this while seeing a flood of (apparently innocuous) “Unable to receive ZMQ rawtx message: …” messages from Lnd. Can you add a release note in a followup PR?

luke-jr referenced this in commit fc06f32054 on Dec 18, 2018

luke-jr referenced this in commit c84c99ee0f on Dec 24, 2018

kallewoof referenced this in commit 66c015529e on Oct 4, 2019

deadalnix referenced this in commit b160ed70b7 on Jun 9, 2020

PastaPastaPasta referenced this in commit 44f2da3495 on Jun 27, 2021

PastaPastaPasta referenced this in commit f48ff35201 on Jun 28, 2021

PastaPastaPasta referenced this in commit c54b296f4b on Jun 29, 2021

Munkybooty referenced this in commit b80868dc6d on Jul 29, 2021

Munkybooty referenced this in commit 3f6fac3226 on Aug 3, 2021

Munkybooty referenced this in commit 2bcf95951f on Aug 5, 2021

dzutto referenced this in commit 2017e0ec69 on Aug 23, 2021

dzutto referenced this in commit 896295c7a7 on Aug 23, 2021

dzutto referenced this in commit e225795144 on Aug 23, 2021

dzutto referenced this in commit a7414e35b0 on Sep 3, 2021

dzutto referenced this in commit 1d503a7b5e on Sep 8, 2021

DrahtBot locked this on Sep 8, 2021

ZMQ: add options to configure outbound message high water mark, aka SNDHWM #14060

Conflicts