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: add options to configure outbound message high water mark, aka SNDHWM #14060

Merged
merged 1 commit into from Nov 5, 2018

Conversation

Projects
None yet
@mruddy
Copy link
Contributor

mruddy commented Aug 25, 2018

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

This is my attempt at #13315

@mruddy mruddy force-pushed the mruddy:zmqhwm branch from 54e2091 to 445559f Aug 25, 2018

@domob1812
Copy link
Contributor

domob1812 left a comment

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

Show resolved Hide resolved src/init.cpp Outdated
@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Aug 25, 2018

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 mruddy force-pushed the mruddy:zmqhwm branch from 445559f to 489bf7d Aug 25, 2018

@mruddy

This comment has been minimized.

Copy link
Contributor

mruddy commented Aug 25, 2018

@domob1812 sure, added hwm to getzmqnotifications

@mruddy mruddy force-pushed the mruddy:zmqhwm branch from 489bf7d to 5789a31 Aug 25, 2018

@promag

This comment has been minimized.

Copy link
Member

promag commented Aug 25, 2018

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

@mruddy

This comment has been minimized.

Copy link
Contributor

mruddy commented Aug 25, 2018

@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

This comment has been minimized.

Copy link
Contributor

mruddy commented Aug 25, 2018

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.

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

This comment has been minimized.

@domob1812

domob1812 Aug 25, 2018

Contributor

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

This comment has been minimized.

@mruddy

mruddy Aug 25, 2018

Contributor

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.

This comment has been minimized.

@promag

promag Aug 27, 2018

Member

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

This comment has been minimized.

Copy link
Member

promag commented Aug 27, 2018

@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:

-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.

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

This comment has been minimized.

@practicalswift

practicalswift Sep 5, 2018

Member

outbound_message_high_water_mark not initialized in the constructor?

This comment has been minimized.

@mruddy

mruddy Sep 11, 2018

Contributor

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 mruddy force-pushed the mruddy:zmqhwm branch 2 times, most recently from 313c5e1 to 38677c1 Sep 11, 2018

@mruddy

This comment has been minimized.

Copy link
Contributor

mruddy commented Sep 17, 2018

rebased and updated the functional test just a moment ago

@mruddy mruddy force-pushed the mruddy:zmqhwm branch from 38677c1 to 9ac2f94 Sep 20, 2018

@mruddy

This comment has been minimized.

Copy link
Contributor

mruddy commented Sep 20, 2018

rebased and added a comma in the getzmqnotifications help message

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

This comment has been minimized.

@practicalswift

practicalswift Sep 25, 2018

Member
2018-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?

This comment has been minimized.

@mruddy

mruddy Sep 26, 2018

Contributor

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!

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

This comment has been minimized.

@practicalswift

practicalswift Sep 25, 2018

Member
2018-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]

This comment has been minimized.

@mruddy

mruddy Sep 26, 2018

Contributor

explicit cast added

@mruddy mruddy force-pushed the mruddy:zmqhwm branch from 9ac2f94 to 89840a3 Sep 26, 2018

@mruddy mruddy force-pushed the mruddy:zmqhwm branch from 89840a3 to a325a21 Oct 12, 2018

@mruddy

This comment has been minimized.

Copy link
Contributor

mruddy commented Oct 12, 2018

rebased

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Oct 12, 2018

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

@mruddy

This comment has been minimized.

Copy link
Contributor

mruddy commented Oct 13, 2018

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

This comment has been minimized.

Copy link
Contributor

jmcorgan commented Oct 18, 2018

ACK

@mruddy mruddy force-pushed the mruddy:zmqhwm branch from a325a21 to a4edb16 Oct 19, 2018

@mruddy

This comment has been minimized.

Copy link
Contributor

mruddy commented Oct 19, 2018

@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 DrahtBot removed the Needs rebase label Oct 20, 2018

@DrahtBot DrahtBot referenced this pull request Oct 20, 2018

Closed

Rpc help helper class #14502

@reel

This comment has been minimized.

Copy link

reel commented Oct 20, 2018

Tested. Works great

@laanwj laanwj merged commit a4edb16 into bitcoin:master Nov 5, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Nov 5, 2018

Merge #14060: ZMQ: add options to configure outbound message high wat…
…er mark, aka SNDHWM

a4edb16 ZMQ: add options to configure outbound message high water mark, aka SNDHWM (mruddy)

Pull request description:

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

  This is my attempt at #13315

Tree-SHA512: a4cc3bcf179776899261a97c8c4f31f35d1d8950fd71a09a79c5c064879b38e600b26824c89c4091d941502ed5b0255390882f7d44baf9e6dc49d685a86e8edb

@mruddy mruddy deleted the mruddy:zmqhwm branch Nov 5, 2018

mruddy added a commit to mruddy/bitcoin that referenced this pull request Nov 10, 2018

laanwj added a commit that referenced this pull request Nov 14, 2018

Merge #14704: doc: add detached release notes for #14060
86cddf0 doc: add detached release notes for #14060 (mruddy)

Pull request description:

  Adding detached release notes for #14060 in order to assist with #14688.

Tree-SHA512: 170cd0b6ac27237f58875f301132e0695dee36eec5d90a0c1234c0fa9db125151e6cebd94e63e634ddc4ddcfb77d7a4a07434ffd3725cb0dfa13fc4ccfbde2a0

jfhk added a commit to jfhk/bitcoin that referenced this pull request Nov 14, 2018

JeremyRubin added a commit to JeremyRubin/bitcoin that referenced this pull request Nov 24, 2018

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Nov 26, 2018

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Dec 8, 2018

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 added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 18, 2018

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment