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

pull mruddy wants to merge 1 commits into bitcoin:master from mruddy:zmqhwm changing 11 files +59 −14
  1. mruddy commented at 7:18 AM on August 25, 2018: contributor

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

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

  2. fanquake added the label RPC/REST/ZMQ on Aug 25, 2018
  3. mruddy force-pushed on Aug 25, 2018
  4. 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)

  5. domob1812 commented at 10:05 AM on August 25, 2018: contributor

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

  6. DrahtBot commented at 10:08 AM on August 25, 2018: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

  7. mruddy force-pushed on Aug 25, 2018
  8. mruddy commented at 10:29 AM on August 25, 2018: contributor

    @domob1812 sure, added hwm to getzmqnotifications

  9. mruddy force-pushed on Aug 25, 2018
  10. promag commented at 10:59 AM on August 25, 2018: member

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

  11. mruddy force-pushed on Aug 25, 2018
  12. 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.

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

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

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

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

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

  17. mruddy force-pushed on Sep 11, 2018
  18. mruddy force-pushed on Sep 17, 2018
  19. mruddy commented at 1:15 PM on September 17, 2018: contributor

    rebased and updated the functional test just a moment ago

  20. mruddy force-pushed on Sep 20, 2018
  21. mruddy commented at 10:37 AM on September 20, 2018: contributor

    rebased and added a comma in the getzmqnotifications help message

  22. 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: 
    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?

    mruddy commented at 12: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!

  23. 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: 
    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]
    mruddy commented at 12:11 AM on September 26, 2018:

    explicit cast added

  24. mruddy force-pushed on Sep 26, 2018
  25. DrahtBot added the label Needs rebase on Oct 8, 2018
  26. mruddy force-pushed on Oct 12, 2018
  27. mruddy commented at 11:46 AM on October 12, 2018: contributor

    rebased

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

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

  30. jmcorgan commented at 10:00 PM on October 18, 2018: contributor

    ACK

  31. ZMQ: add options to configure outbound message high water mark, aka SNDHWM a4edb168b6
  32. mruddy force-pushed on Oct 19, 2018
  33. 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.

  34. DrahtBot removed the label Needs rebase on Oct 20, 2018
  35. reel commented at 4:06 PM on October 20, 2018: none

    Tested. Works great

  36. laanwj merged this on Nov 5, 2018
  37. laanwj closed this on Nov 5, 2018
  38. laanwj referenced this in commit dac2caa371 on Nov 5, 2018
  39. mruddy deleted the branch on Nov 5, 2018
  40. mruddy referenced this in commit 86cddf0856 on Nov 10, 2018
  41. laanwj referenced this in commit 2e8f9dc2bb on Nov 14, 2018
  42. jfhk referenced this in commit 4b50a1d782 on Nov 14, 2018
  43. JeremyRubin referenced this in commit c17b9fba08 on Nov 24, 2018
  44. HashUnlimited referenced this in commit 37c6228c2c on Nov 26, 2018
  45. 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?

  46. luke-jr referenced this in commit fc06f32054 on Dec 18, 2018
  47. luke-jr referenced this in commit c84c99ee0f on Dec 24, 2018
  48. kallewoof referenced this in commit 66c015529e on Oct 4, 2019
  49. deadalnix referenced this in commit b160ed70b7 on Jun 9, 2020
  50. PastaPastaPasta referenced this in commit 44f2da3495 on Jun 27, 2021
  51. PastaPastaPasta referenced this in commit f48ff35201 on Jun 28, 2021
  52. PastaPastaPasta referenced this in commit c54b296f4b on Jun 29, 2021
  53. Munkybooty referenced this in commit b80868dc6d on Jul 29, 2021
  54. Munkybooty referenced this in commit 3f6fac3226 on Aug 3, 2021
  55. Munkybooty referenced this in commit 2bcf95951f on Aug 5, 2021
  56. dzutto referenced this in commit 2017e0ec69 on Aug 23, 2021
  57. dzutto referenced this in commit 896295c7a7 on Aug 23, 2021
  58. dzutto referenced this in commit e225795144 on Aug 23, 2021
  59. dzutto referenced this in commit a7414e35b0 on Sep 3, 2021
  60. dzutto referenced this in commit 1d503a7b5e on Sep 8, 2021
  61. DrahtBot locked this on Sep 8, 2021
Contributors
mruddydomob1812reelluke-jrpromagDrahtBotpracticalswiftsipajmcorganSjors
Labels
RPC/REST/ZMQ
Linked (view graph)
#13315 configurable ZMQ message limit#14704 doc: add detached release notes for #14060
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-19 00:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me