ZMQ: Create "sequence" notifier, enabling client-side mempool tracking #19572

instagibbs commented at 3:06 PM on July 23, 2020: member

This PR creates a new ZMQ notifier that gives a "total hash history" of block (dis)connection, mempool addition/substraction, all in one pipeline. It also exposes a "mempool sequence number" to both this notifier and getrawmempool results, which allows the consumer to use the results together without confusion about ordering of results and without excessive getrawmempool polling.

See the functional test interfaces_zmq.py::test_mempool_sync which shows the proposed user flow for the client-side tracking of mempool contents and confirmations.

Inspired by #19462 (comment) Alternative to #19462 due to noted deficiencies in current zmq notification streams.

Also fixes a legacy zmq test that didn't actually trigger a reorg because of identical blocks being generated on each side of the split(oops)

instagibbs force-pushed on Jul 23, 2020

instagibbs commented at 3:57 PM on July 23, 2020: member

fixed bug(was pushing to verbose list of transactions)

looking at another racey looking failure:

    assert_equal((payment_txid_2, "A", seq_num), seq.receive_sequence())
  File "/home/travis/build/bitcoin/bitcoin/ci/scratch/build/bitcoin-arm-linux-gnueabihf/test/functional/test_framework/util.py", line 49, in assert_equal
    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not(('1ac086d8f16212a6bab2bceee95742ff09e9477928c2e09e931a57c382d7b09b', 'A', 4) == ('1ac086d8f16212a6bab2bceee95742ff09e9477928c2e09e931a57c382d7b09b', 'A', 5))

edit: Pushed fix(I hope) for that.

DrahtBot commented at 4:57 PM on July 23, 2020: 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:

#19967 (test: Replace (dis)?connect_nodes globals with TestFramework methods by robot-dreams)
#19652 (Avoid locking CTxMemPool::cs recursively in Mempool{Info}ToJSON() by hebasto)
#19532 (test: Mypy next steps by Ghorbanian)
#18531 (rpc: Assert that RPCArg names are equal to CRPCCommand ones by MarcoFalke)
#18309 (zmq: Add support to listen on multiple interfaces by n-thumann)

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.

DrahtBot added the label Mempool on Jul 23, 2020

DrahtBot added the label RPC/REST/ZMQ on Jul 23, 2020

DrahtBot added the label Tests on Jul 23, 2020

DrahtBot added the label Validation on Jul 23, 2020

DrahtBot added the label Wallet on Jul 23, 2020

instagibbs force-pushed on Jul 23, 2020

in test/functional/interface_zmq.py:170 in 951507c4d1 outdated

 146 | @@ -147,8 +147,8 @@ def test_reorg(self):
 147 |          self.nodes[0].generatetoaddress(1, ADDRESS_BCRT1_UNSPENDABLE)
 148 |          assert_equal(self.nodes[0].getbestblockhash(), hashblock.receive().hex())
 149 |  
 150 | -        # Generate 2 blocks in nodes[1]
 151 | -        self.nodes[1].generatetoaddress(2, ADDRESS_BCRT1_UNSPENDABLE)
 152 | +        # Generate 2 blocks in nodes[1] to a different address to ensure split
 153 | +        self.nodes[1].generatetoaddress(2, ADDRESS_BCRT1_P2WSH_OP_TRUE)

promag commented at 10:23 PM on July 23, 2020:

951507c4d1152ceab2a930fb16fdac05504f0056

oops 😅 good catch.

in src/txmempool.h:735 in dc413fbd8d outdated

 730 | @@ -725,6 +731,17 @@ class CTxMemPool
 731 |          return (m_unbroadcast_txids.count(txid) != 0);
 732 |      }
 733 |  
 734 | +    /** Guards this internal counter for external reporting */
 735 | +    uint32_t GetAndIncrementSequence() const {

promag commented at 10:29 PM on July 23, 2020:

EXCLUSIVE_LOCKS_REQUIRED(cs) instead?

instagibbs commented at 1:39 AM on August 4, 2020:

done

promag commented at 10:37 PM on July 23, 2020: member

I still think #19476 makes life easier for the client in regards to fault tolerance and simplicity. With this PR, the client still has to bake its state from RPC and ZMQ.

I think you could extract CValidationInterface changes to a different PR.

instagibbs commented at 10:57 PM on July 23, 2020: member

From conversations with people who use the notifications already they seemed more keen to a solution like this that doesn't require long polling support. Also it makes wumpus less upset to just improve a current notification system 🤯

That's said they're not necessarily in competition.

On Thu, Jul 23, 2020, 6:37 PM João Barbosa notifications@github.com wrote:

@promag commented on this pull request.

I still think #19476 https://github.com/bitcoin/bitcoin/pull/19476 makes life easier for the client in regards to fault tolerance and simplicity. With this PR, the client still has to bake its state from RPC and ZMQ.

I think you could extract CValidationInterface changes to a different PR.

In test/functional/interface_zmq.py https://github.com/bitcoin/bitcoin/pull/19572#discussion_r459761022:

@@ -147,8 +147,8 @@ def test_reorg(self):
     self.nodes[0].generatetoaddress(1, ADDRESS_BCRT1_UNSPENDABLE)

     assert_equal(self.nodes[0].getbestblockhash(), hashblock.receive().hex())
   # Generate 2 blocks in nodes[1]
   self.nodes[1].generatetoaddress(2, ADDRESS_BCRT1_UNSPENDABLE)
   # Generate 2 blocks in nodes[1] to a different address to ensure split
   self.nodes[1].generatetoaddress(2, ADDRESS_BCRT1_P2WSH_OP_TRUE)
951507c https://github.com/bitcoin/bitcoin/commit/951507c4d1152ceab2a930fb16fdac05504f0056

oops 😅 good catch.

In src/txmempool.h https://github.com/bitcoin/bitcoin/pull/19572#discussion_r459763375:

@@ -725,6 +731,17 @@ class CTxMemPool
     return (m_unbroadcast_txids.count(txid) != 0);

 }
/** Guards this internal counter for external reporting */

uint32_t GetAndIncrementSequence() const {

EXCLUSIVE_LOCKS_REQUIRED(cs) instead?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/19572#pullrequestreview-454535546, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABMAFU6BN2AS2HWUGITFY7LR5C3UFANCNFSM4PF2HZKQ .

promag commented at 10:53 AM on July 26, 2020: member

That's said they're not necessarily in competition.

I wasn't suggesting otherwise 👍

jonasschnelli commented at 7:50 AM on July 27, 2020: contributor

Concept ACK

instagibbs commented at 12:52 PM on July 27, 2020: member

I'm running some extensive testing of this PR against testnet and mainnet nodes, tracking expected mempool for a couple weeks using a modified test harness. Once I'm satisfied I'll be updating the test for this PR with the updated logic(if any) and maybe the consumer tool itself.

instagibbs commented at 3:14 PM on July 27, 2020: member

added test showing conflicted tx from block announcement("R" shows up before "C")

instagibbs commented at 4:12 PM on July 27, 2020: member

Was able to replicate a seeming gap in mempool_sequence values being reported. Pushed a possibly controversial fix(touching mempool logic) showing that it fixes the test case written.

While that is a very minimal change I can understand it might rub people the wrong way.

The alternatives are: 0) Ignore the gaps(print a warning when you see a gap maybe), it doesn't seem to meaningfully interact with using the features as-is. Consumer will be tracking zmq sequence value, and any getrawmempool results will report an "expected" value.

"Just" account for these gaps. When hitting an "R" mempool_sequence gap, sum up those gaps, then make sure that value + the number of found mempool transactions being removed normally equals the mempool_sequence reported in the next mempool report.
Add another removal reporting hook which just announces all txhashes being removed.

(2) seems like way overkill. (0) is fine for mempool view tracking, (1) just allows the consumer to do more asserting I guess, you can take it or leave it.

Thoughts on last commit?

edit: I've decided to just keep behavior as-is to avoid touching mempool code, and update the sample usage test to reflect that check.

instagibbs force-pushed on Jul 27, 2020

DrahtBot added the label Needs rebase on Jul 30, 2020

in src/init.cpp:486 in b5de7c116a outdated

 482 | @@ -483,10 +483,12 @@ void SetupServerArgs(NodeContext& node)
 483 |      gArgs.AddArg("-zmqpubhashtx=<address>", "Enable publish hash transaction in <address>", ArgsManager::ALLOW_ANY, OptionsCategory::ZMQ);
 484 |      gArgs.AddArg("-zmqpubrawblock=<address>", "Enable publish raw block in <address>", ArgsManager::ALLOW_ANY, OptionsCategory::ZMQ);
 485 |      gArgs.AddArg("-zmqpubrawtx=<address>", "Enable publish raw transaction in <address>", ArgsManager::ALLOW_ANY, OptionsCategory::ZMQ);
 486 | +    gArgs.AddArg("-zmqpubsequence=<address>", "Enable publish hash block and tx sequence in <address>", ArgsManager::ALLOW_ANY, OptionsCategory::ZMQ);

luke-jr commented at 12:32 AM on July 31, 2020:

"sequence" doesn't really express what this does. Maybe "zmqpubmempool"?

instagibbs commented at 12:36 AM on July 31, 2020: member

Point taken but it also publishes a sequence of block hashes for connects and disconnects.

On Thu, Jul 30, 2020, 8:32 PM Luke Dashjr notifications@github.com wrote:

@luke-jr commented on this pull request.

In src/init.cpp https://github.com/bitcoin/bitcoin/pull/19572#discussion_r463343776:

@@ -483,10 +483,12 @@ void SetupServerArgs(NodeContext& node) gArgs.AddArg("-zmqpubhashtx=<address>", "Enable publish hash transaction in <address>", ArgsManager::ALLOW_ANY, OptionsCategory::ZMQ); gArgs.AddArg("-zmqpubrawblock=<address>", "Enable publish raw block in <address>", ArgsManager::ALLOW_ANY, OptionsCategory::ZMQ); gArgs.AddArg("-zmqpubrawtx=<address>", "Enable publish raw transaction in <address>", ArgsManager::ALLOW_ANY, OptionsCategory::ZMQ);

gArgs.AddArg("-zmqpubsequence=<address>", "Enable publish hash block and tx sequence in <address>", ArgsManager::ALLOW_ANY, OptionsCategory::ZMQ);

"sequence" doesn't really express what this does. Maybe "zmqpubmempool"?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/19572#pullrequestreview-458848709, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABMAFU4SHWAGULPMYAO77NLR6IGKNANCNFSM4PF2HZKQ .

instagibbs force-pushed on Aug 4, 2020

instagibbs commented at 1:39 AM on August 4, 2020: member

rebased on master.

Improved mempool sync test with code I've been running on mainnet/testnet for a few weeks now, last week with no issues. Added exclusive locks annotation.

instagibbs force-pushed on Aug 4, 2020

DrahtBot removed the label Needs rebase on Aug 4, 2020

in src/init.cpp:504 in e98f4b36be outdated

 500 | @@ -499,6 +501,7 @@ void SetupServerArgs(NodeContext& node)
 501 |      hidden_args.emplace_back("-zmqpubhashtxhwm=<n>");
 502 |      hidden_args.emplace_back("-zmqpubrawblockhwm=<n>");
 503 |      hidden_args.emplace_back("-zmqpubrawtxhwm=<n>");
 504 | +    hidden_args.emplace_back("-zmqpubseqhwm=<n>");

luke-jr commented at 6:28 PM on August 8, 2020:

Shouldn't this be spelled out?

instagibbs commented at 5:51 PM on August 18, 2020:

done

in src/init.cpp:504 in e98f4b36be outdated

 500 | @@ -499,6 +501,7 @@ void SetupServerArgs(NodeContext& node)
 501 |      hidden_args.emplace_back("-zmqpubhashtxhwm=<n>");

luke-jr commented at 6:28 PM on August 8, 2020:

Think you need zmqpubsequence in here too

instagibbs commented at 5:50 PM on August 18, 2020:

done

in src/rpc/blockchain.cpp:502 in e98f4b36be outdated

 498 | @@ -499,14 +499,25 @@ UniValue MempoolToJSON(const CTxMemPool& pool, bool verbose)
 499 |          }
 500 |          return o;
 501 |      } else {
 502 | +        uint32_t mempool_sequence = 0;

luke-jr commented at 1:47 AM on August 9, 2020:

This shouldn't be initialised here

instagibbs commented at 5:50 PM on August 18, 2020:

done

in src/rpc/blockchain.cpp:505 in e98f4b36be outdated

 482 | @@ -483,7 +483,7 @@ static void entryToJSON(const CTxMemPool& pool, UniValue& info, const CTxMemPool
 483 |      info.pushKV("unbroadcast", pool.IsUnbroadcastTx(tx.GetHash()));
 484 |  }
 485 |  
 486 | -UniValue MempoolToJSON(const CTxMemPool& pool, bool verbose)
 487 | +UniValue MempoolToJSON(const CTxMemPool& pool, bool verbose, bool include_mempool_sequence)
 488 |  {
 489 |      if (verbose) {

luke-jr commented at 1:50 AM on August 9, 2020:

Should throw an exception if include_mempool_sequence is true here. (Or support it)

instagibbs commented at 5:50 PM on August 18, 2020:

done, throws error + test

in src/txmempool.h:480 in e98f4b36be outdated

 473 | @@ -474,6 +474,12 @@ class CTxMemPool
 474 |      mutable uint64_t m_epoch;
 475 |      mutable bool m_has_epoch_guard;
 476 |  
 477 | +    // In-memory counter for external mempool tracking purposes.
 478 | +    // This number is incremented once every time a transaction
 479 | +    // is added or removed from the mempool(except when removed
 480 | +    // for block inclusion).

luke-jr commented at 1:54 AM on August 9, 2020:

The code increments it for block inclusion removals too...

instagibbs commented at 3:27 PM on August 10, 2020:

Right, stale comment. Will update.

instagibbs commented at 5:50 PM on August 18, 2020:

done

in src/txmempool.h:481 in e98f4b36be outdated

 473 | @@ -474,6 +474,12 @@ class CTxMemPool
 474 |      mutable uint64_t m_epoch;
 475 |      mutable bool m_has_epoch_guard;
 476 |  
 477 | +    // In-memory counter for external mempool tracking purposes.
 478 | +    // This number is incremented once every time a transaction
 479 | +    // is added or removed from the mempool(except when removed
 480 | +    // for block inclusion).
 481 | +    mutable uint32_t sequence_number{1};

luke-jr commented at 1:54 AM on August 9, 2020:

m_sequence_number

luke-jr commented at 1:57 AM on August 9, 2020:

Maybe uint64_t would be better?

Possibly even a sequence_number_t typedef in case it were to ever change.

instagibbs commented at 3:29 PM on August 10, 2020:

I believe my original theory was that the zmq sequence values themselves were also 32 bit, so I figured not much to be gained since any consumer has to handle this wrap around anyways?

instagibbs commented at 1:36 PM on August 18, 2020:

Offline someone noted that the ZMQ sequence numbers(probably) reset if the connection itself is reset, but the mempool sequence numbers will continue. I think this is a good argument to expand to uint64_t. Will do that soon.

instagibbs commented at 5:50 PM on August 18, 2020:

done

instagibbs commented at 5:50 PM on August 18, 2020:

done

in src/validationinterface.h:106 in e98f4b36be outdated

  97 | @@ -98,6 +98,17 @@ class CValidationInterface {
  98 |       * Called on a background thread.
  99 |       */
 100 |      virtual void TransactionAddedToMempool(const CTransactionRef& tx) {}
 101 | +    /**
 102 | +     * Notifies listeners of a transaction having been added to mempool.
 103 | +     *
 104 | +     * Required because of legacy ZMQ notifications including block
 105 | +     * transactions when using TransactionAddedToMempool. See:
 106 | +     * CZMQNotificationInterface::TransactionAddedToMempool

luke-jr commented at 2:01 AM on August 9, 2020:

Ugly hack. Just change CZMQNotificationInterface

instagibbs commented at 3:30 PM on August 10, 2020:

Sorry, change it how specifically? If you mean I should change the legacy notifications too, I disagree? Might be misunderstanding.

instagibbs commented at 7:04 PM on August 18, 2020:

Figured out what you meant. Done in follow-on commit.

luke-jr changes_requested

instagibbs force-pushed on Aug 18, 2020

instagibbs commented at 7:07 PM on August 18, 2020: member

Updated PR with fixes/cleanups, and changed sequence number to 8 bytes.

Dogfooding consumer still running strong after 2+ weeks on testnet/mainnet, no mempool inconsistencies or dropped messages.

jonatack commented at 7:25 PM on August 18, 2020: member

Concept ACK

(You had me at "dogfooding")

instagibbs commented at 3:41 PM on August 20, 2020: member

Looks like the consumer logic failed during the recent mainnet reorg of one block https://twitter.com/BitMEXResearch/status/1296431817592057857

A->D->A occurred and the mempool sequence number jumped by a couple hundred between the two As which is unexpected based on my assumptions of Core locking. Investigating more. (Note that this worst case means you have to take an additional getrawmempool snapshot) If this ends up being too messy I may change the format to notifying for each removal by mining as well.

MarcoFalke removed the label Tests on Aug 23, 2020

MarcoFalke removed the label Wallet on Aug 23, 2020

instagibbs commented at 5:06 PM on August 25, 2020: member

After some consideration of the failure detected above, I'm going to stick with the current format, and eat the cost of a single getrawmempool call every time an unexpected mempool sequence value is encountered. If all evictions are announced for block inclusion we get dangerously close to blowing out zmq buffers, and in practice is creating more traffic for little gain.

In other words, please continue review :)

DrahtBot added the label Needs rebase on Aug 31, 2020

instagibbs force-pushed on Sep 1, 2020

instagibbs commented at 2:44 AM on September 1, 2020: member

rebased

DrahtBot removed the label Needs rebase on Sep 1, 2020

n-thumann commented at 6:25 PM on September 1, 2020: contributor

Tested ACK, works as expected on macOS 10.15.6. Just two minor hints: I think this should also be added to doc/zmq.md and explicitly explain the difference between the already existing sequence number and this new mempool sequence number to prevent confusion. Maybe also add these new notifications to contrib/zmq_sub.py with unpacking of message type (Tx addition/removal, Block connect/disconnect) and mempool sequence number ✌️

instagibbs commented at 6:28 PM on September 1, 2020: member

@n-thumann good points, I was unfamiliar with those docs. Will update with an additional commit

instagibbs commented at 7:19 PM on September 1, 2020: member

@n-thumann can you actually get contrib/zmq/zmq_sub.py to work? I updated what I think it should look like, but I had trouble running it even with legacy notifications. Try it out?

Task exception was never retrieved
future: <Task finished coro=<ZMQHandler.handle() done, defined at contrib/zmq/zmq_sub.py:54> exception=ValueError('Invalid file object: <zmq.asyncio.Socket object at 0x7f62f0138e88>',)>
Traceback (most recent call last):
  File "contrib/zmq/zmq_sub.py", line 55, in handle
    topic, body, seq = await self.zmqSubSocket.recv_multipart()
  File "/usr/lib/python3/dist-packages/zmq/eventloop/future.py", line 170, in recv_multipart
    dict(flags=flags, copy=copy, track=track)
  File "/usr/lib/python3/dist-packages/zmq/eventloop/future.py", line 321, in _add_recv_event
    self._add_io_state(self._READ)
  File "/usr/lib/python3/dist-packages/zmq/asyncio/__init__.py", line 294, in _add_io_state
    self.io_loop.add_reader(self, self._handle_recv)
  File "/usr/lib/python3.6/asyncio/selector_events.py", line 326, in add_reader
    return self._add_reader(fd, callback, *args)
  File "/usr/lib/python3.6/asyncio/selector_events.py", line 253, in _add_reader
    key = self._selector.get_key(fd)
  File "/usr/lib/python3.6/selectors.py", line 189, in get_key
    return mapping[fileobj]
  File "/usr/lib/python3.6/selectors.py", line 70, in __getitem__
    fd = self._selector._fileobj_lookup(fileobj)
  File "/usr/lib/python3.6/selectors.py", line 224, in _fileobj_lookup
    return _fileobj_to_fd(fileobj)
  File "/usr/lib/python3.6/selectors.py", line 39, in _fileobj_to_fd
    "{!r}".format(fileobj)) from None
ValueError: Invalid file object: <zmq.asyncio.Socket object at 0x7f62f0138e88>

instagibbs force-pushed on Sep 1, 2020

in doc/zmq.md:125 in 6bc7b33008 outdated

 120 | @@ -119,3 +121,8 @@ There are several possibilities that ZMQ notification can get lost
 121 |  during transmission depending on the communication type you are
 122 |  using. Bitcoind appends an up-counting sequence number to each
 123 |  notification which allows listeners to detect lost notifications.
 124 | +
 125 | +The "sequence" publisher refers specifically to the mempool sequence

n-thumann commented at 10:21 PM on September 1, 2020:

slightly more consistent wording 😊

The `sequence` topic refers specifically to the mempool sequence

in contrib/zmq/zmq_sub.py:72 in 6bc7b33008 outdated

  67 | @@ -69,6 +68,12 @@ async def handle(self) :
  68 |          elif topic == b"rawtx":
  69 |              print('- RAW TX ('+sequence+') -')
  70 |              print(binascii.hexlify(body))
  71 | +        elif topic == b"sequence":
  72 | +            hash = body[:32].hex()

n-thumann commented at 11:08 PM on September 1, 2020:

Use bytes instead of string

            hash = binascii.hexlify(body[:32])

in contrib/zmq/zmq_sub.py:76 in 6bc7b33008 outdated

  67 | @@ -69,6 +68,12 @@ async def handle(self) :
  68 |          elif topic == b"rawtx":
  69 |              print('- RAW TX ('+sequence+') -')
  70 |              print(binascii.hexlify(body))
  71 | +        elif topic == b"sequence":
  72 | +            hash = body[:32].hex()
  73 | +            label = chr(body[32])
  74 | +            mempool_sequence = None if len(body) != 32+1+8 else struct.unpack("<Q", body[32+1:])[0]
  75 | +            print('- SEQUENCE ('+sequence+') -')
  76 | +            print("{} {} {}".format(hash, label, mempool_sequence))

n-thumann commented at 11:16 PM on September 1, 2020:

Format looks unnecessary here :)

            print(hash, label, mempool_sequence)

n-thumann commented at 11:17 PM on September 1, 2020: contributor

@n-thumann can you actually get contrib/zmq/zmq_sub.py to work?

Your updated script runs perfectly fine! Another doc related idea: Should these lines be changed? This only applies when using hashblock or rawblock and can be circumvented by using sequence topic. I think it´s also useful to briefly mention here that the body of sequence notifications differs from the others, so basically what you´ve already written here ✌️. Oh, and in case you have a little bit of spare time I would you mind having a look at my (also zmq-related) PR #18309? 😊

instagibbs force-pushed on Sep 2, 2020

instagibbs commented at 2:05 AM on September 2, 2020: member

took your suggestions @n-thumann , added some more information to docs as well as suggested

n-thumann approved

instagibbs requested review from promag on Sep 2, 2020

in test/functional/interface_zmq.py:237 in 103207ba2e outdated

 232 | +        <32-byte hash>D :                 Blockhash disconnected
 233 | +        <32-byte hash>R<8-byte LE uint> : Transactionhash removed from mempool for non-block inclusion reason
 234 | +        <32-byte hash>A<8-byte LE uint> : Transactionhash added mempool
 235 | +        """
 236 | +        self.log.info("Testing 'sequence' publisher")
 237 | +        import zmq

jonatack commented at 2:24 PM on September 3, 2020:

Perhaps import zmq once at the top rather than 4 times in the tests, e.g.:

diff --git a/test/functional/interface_zmq.py b/test/functional/interface_zmq.py
index ef4780cacb..3afab07dbf 100755
--- a/test/functional/interface_zmq.py
+++ b/test/functional/interface_zmq.py
@@ -4,6 +4,7 @@
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """Test the ZMQ notification interface."""
 import struct
+import zmq
 
 from test_framework.address import ADDRESS_BCRT1_UNSPENDABLE
 from test_framework.test_framework import BitcoinTestFramework
@@ -21,7 +22,6 @@ class ZMQSubscriber:
         self.socket = socket
         self.topic = topic
 
-        import zmq
         self.socket.setsockopt(zmq.SUBSCRIBE, self.topic)
 
     def receive(self):
@@ -43,7 +43,6 @@ class ZMQTest (BitcoinTestFramework):
         self.skip_if_no_bitcoind_zmq()
 
     def run_test(self):
-        import zmq
         self.ctx = zmq.Context()
         try:
             self.test_basic()
@@ -54,8 +53,6 @@ class ZMQTest (BitcoinTestFramework):
             self.ctx.destroy(linger=None)
 
     def test_basic(self):
-        import zmq
-
         # Invalid zmq arguments don't take down the node, see [#17185](/bitcoin-bitcoin/17185/).
         self.restart_node(0, ["-zmqpubrawtx=foo", "-zmqpubhashtx=bar"])
 
@@ -146,7 +143,6 @@ class ZMQTest (BitcoinTestFramework):
             self.log.info("Skipping reorg test because wallet is disabled")
             return
 
-        import zmq
         address = 'tcp://127.0.0.1:28333'
 
         services = [b"hashblock", b"hashtx"]

instagibbs commented at 4:12 PM on September 4, 2020:

done

instagibbs commented at 7:06 PM on September 4, 2020:

found out why: zmq-less functional test builds. Stuck it inside a pass-through try/except block.

in src/zmq/zmqnotificationinterface.cpp:208 in 103207ba2e outdated

 205 | +        for (std::list<CZMQAbstractNotifier*>::iterator i = notifiers.begin(); i!=notifiers.end(); )
 206 | +        {
 207 | +            CZMQAbstractNotifier *notifier = *i;
 208 | +            if (notifier->NotifyTransaction(*ptx))
 209 | +            {
 210 | +                i++;

jonatack commented at 2:56 PM on September 3, 2020:

here and four other places below in the new code

                ++i;

instagibbs commented at 7:19 PM on September 3, 2020:

is this in the style guide? :P

jonatack commented at 7:42 PM on September 3, 2020:

yeah, ++i is preferred over i++

instagibbs commented at 4:12 PM on September 4, 2020:

done

in contrib/zmq/zmq_sub.py:55 in 103207ba2e outdated

  53 |  
  54 |      async def handle(self) :
  55 | -        msg = await self.zmqSubSocket.recv_multipart()
  56 | -        topic = msg[0]
  57 | -        body = msg[1]
  58 | +        topic, body, seq = await self.zmqSubSocket.recv_multipart()

jonatack commented at 5:18 PM on September 3, 2020:

Nice cleanup here.

in contrib/zmq/zmq_sub.py:58 in 103207ba2e outdated

  59 |          sequence = "Unknown"
  60 | -        if len(msg[-1]) == 4:
  61 | -          msgSequence = struct.unpack('<I', msg[-1])[-1]
  62 | -          sequence = str(msgSequence)
  63 | +        if len(seq) == 4:
  64 | +          sequence = str(struct.unpack('<I', seq)[-1])

jonatack commented at 5:18 PM on September 3, 2020:

indentation

            sequence = str(struct.unpack('<I', seq)[-1])

instagibbs commented at 4:11 PM on September 4, 2020:

done

in test/functional/interface_zmq.py:12 in 103207ba2e outdated

  10 | +from test_framework.blocktools import create_block, create_coinbase, add_witness_commitment
  11 |  from test_framework.test_framework import BitcoinTestFramework
  12 | -from test_framework.messages import CTransaction, hash256
  13 | -from test_framework.util import assert_equal, connect_nodes
  14 | +from test_framework.messages import CTransaction, hash256, FromHex
  15 | +from test_framework.util import assert_equal, connect_nodes, assert_raises_rpc_error

jonatack commented at 5:21 PM on September 3, 2020:

nit, sort

from test_framework.util import (
    assert_equal,
    assert_raises_rpc_error,
    connect_nodes,
)

instagibbs commented at 4:11 PM on September 4, 2020:

done

in test/functional/interface_zmq.py:341 in 103207ba2e outdated

 336 | +            self.log.info("Evict mempool transaction by block conflict")
 337 | +            orig_txid = self.nodes[0].sendtoaddress(address=self.nodes[0].getnewaddress(), amount=1.0, replaceable=True)
 338 | +
 339 | +            # More to be simply mined
 340 | +            more_tx = []
 341 | +            for i in range(5):

jonatack commented at 6:08 PM on September 3, 2020:

            for _ in range(5):

instagibbs commented at 4:11 PM on September 4, 2020:

done

in test/functional/interface_zmq.py:417 in 103207ba2e outdated

 412 | +
 413 | +        # Some transactions have been happening but we aren't consuming zmq notifications yet
 414 | +        # or we lost a ZMQ message somehow and want to start over
 415 | +        txids = []
 416 | +        num_txs = 5
 417 | +        for i in range(num_txs):

jonatack commented at 6:09 PM on September 3, 2020:

        for _ in range(num_txs):

instagibbs commented at 4:11 PM on September 4, 2020:

done

in test/functional/interface_zmq.py:443 in 103207ba2e outdated

 438 | +            mempool_view = set(mempool_snapshot["txids"])
 439 | +            get_raw_seq = mempool_snapshot["mempool_sequence"]
 440 | +
 441 | +        # Things continue to happen in the "interim" while waiting for snapshot results
 442 | +        # We have node 0 do all these to avoid p2p races with RBF announcements
 443 | +        for i in range(num_txs):

jonatack commented at 6:09 PM on September 3, 2020:

        for _ in range(num_txs):

instagibbs commented at 4:11 PM on September 4, 2020:

done

in test/functional/interface_zmq.py:464 in 103207ba2e outdated

 459 | +
 460 | +        # 4) Moving forward, we apply the delta to our local view
 461 | +        #    remaining txs(5) + 1 rbf(A+R) + 1 block connect + 1 final tx
 462 | +        expected_sequence = get_raw_seq
 463 | +        r_gap = 0
 464 | +        for i in range(num_txs+2+1+1):

jonatack commented at 6:10 PM on September 3, 2020:

        for _ in range(num_txs + 2 + 1 + 1):

instagibbs commented at 4:11 PM on September 4, 2020:

done

jonatack commented at 7:05 PM on September 3, 2020: member

Checkpoint review feedback so far:

the debug build is clean and tests green
the contrib script seems to work well and without issues after a day of doing it's thing, though I needed to chmod it to change permissions to run it (EDIT: fixed in #19870)
the new c++ code could definitely do with some clang formatting :heart:
now or for later, there is a fair amount of duplicate-looking code, nothing blocking tho
so far the test interface_zmq.py fails in 38fbf2d8f5 at line 103
in 9b5daf74f the test fails as well; best if each commit has passing tests
EDIT: the test passes for me in commits d353bd19b, a976c97c9, and 92ca0fca0

A few minor comments follow to take or leave.

in doc/zmq.md:127 in 103207ba2e outdated

 127 | +Note that for `*block` topics when the block chain tip changes,
 128 | +a reorganisation may occur and just the tip will be notified.
 129 | +It is up to the subscriber to retrieve the chain from the last known
 130 | +block to the new tip. Also note that no notification occurs if the tip
 131 | +was in the active chain - this is the case after calling invalidateblock RPC.
 132 | +In contrast the `sequence` topic publishes all block connects and disconnects.

jonatack commented at 11:07 AM on September 4, 2020:

nit proposals, feel free to take or ignore

-Note that for `*block` topics when the block chain tip changes,
-a reorganisation may occur and just the tip will be notified.
+Note that for `*block` topics, when the block chain tip changes,
+a reorganisation may occur and only the tip will be notified.
 It is up to the subscriber to retrieve the chain from the last known
-block to the new tip. Also note that no notification occurs if the tip
-was in the active chain - this is the case after calling invalidateblock RPC.
-In contrast the `sequence` topic publishes all block connects and disconnects.
+block to the new tip. Also note that no notification will occur if the tip
+was in the active chain--as would be the case after calling the invalidateblock
+RPC. In contrast, the `sequence` topic publishes all block connections and
+disconnections.

instagibbs commented at 4:11 PM on September 4, 2020:

took most of this

in doc/zmq.md:153 in 103207ba2e outdated

 137 |  notification which allows listeners to detect lost notifications.
 138 | +
 139 | +The `sequence` topic refers specifically to the mempool sequence
 140 | +number which is also published along with all mempool events. This
 141 | +is a different sequence value than in ZMQ itself to allow a total
 142 | +ordering of mempool events to be constructed.

jonatack commented at 11:08 AM on September 4, 2020:

nit suggestions

 The `sequence` topic refers specifically to the mempool sequence
-number which is also published along with all mempool events. This
-is a different sequence value than in ZMQ itself to allow a total
+number, which is also published along with all mempool events. This
+is a different sequence value than in ZMQ itself in order to allow a total
 ordering of mempool events to be constructed.

instagibbs commented at 4:11 PM on September 4, 2020:

done

jonatack commented at 11:18 AM on September 4, 2020: member

Approach ACK on this useful feature for bitcoind mempool management that has the bonus of you and others dogfooding it. Thanks for the tests and updated contrib script and doc. It seems pretty close apart from the nits mentioned above and clean organisation of the commits wrt the test changes. I'm not sure if it's helpful for 92ca0fca04cdb6 to be a separate commit, as it partially undoes some of the changes in the previous commits, unless you wanted to document the design in some way. Otherwise LGTM, will re-review after the next update.

in src/rpc/blockchain.cpp:490 in 103207ba2e outdated

 482 | @@ -483,9 +483,12 @@ static void entryToJSON(const CTxMemPool& pool, UniValue& info, const CTxMemPool
 483 |      info.pushKV("unbroadcast", pool.IsUnbroadcastTx(tx.GetHash()));
 484 |  }
 485 |  
 486 | -UniValue MempoolToJSON(const CTxMemPool& pool, bool verbose)
 487 | +UniValue MempoolToJSON(const CTxMemPool& pool, bool verbose, bool include_mempool_sequence)
 488 |  {
 489 |      if (verbose) {
 490 | +        if (include_mempool_sequence) {
 491 | +            throw JSONRPCError(RPC_INVALID_PARAMETER, "Verbose results can not contain mempool sequence values.");

jonatack commented at 12:29 PM on September 4, 2020:

s/can not/cannot/

instagibbs commented at 4:11 PM on September 4, 2020:

done

instagibbs commented at 4:15 PM on September 4, 2020:

my string change broke the test of this. See!?!

jonatack commented at 4:31 PM on September 4, 2020:

Proof of test! 😄

in src/rpc/blockchain.cpp:538 in 103207ba2e outdated

 519 | +        if (!include_mempool_sequence) {
 520 | +            return a;
 521 | +        } else {
 522 | +            UniValue o(UniValue::VOBJ);
 523 | +            o.pushKV("txids", a);
 524 | +            o.pushKV("mempool_sequence", mempool_sequence);

jonatack commented at 12:31 PM on September 4, 2020:

The behavior of getrawmempool with the additional param and its interaction with verbose should be covered in a functional test, unless I missed the added assertion somewhere.

instagibbs commented at 3:42 PM on September 4, 2020:

There is a test line for it: assert_raises_rpc_error(-8, "Verbose results can not contain mempool sequence values.", self.nodes[0].getrawmempool, True, True)

jonatack commented at 4:01 PM on September 4, 2020:

Oops, I even tested it manually as well. More like an assert("mempool_sequence" in getrawmempool.keys()) or even checking the value too.

jonatack commented at 4:02 PM on September 4, 2020:

Never mind, it looks like you covered that.

jonatack commented at 12:31 PM on September 4, 2020: member

A couple items with the RPC changes.

instagibbs force-pushed on Sep 4, 2020

instagibbs commented at 4:12 PM on September 4, 2020: member

@jonatack I'm keeping https://github.com/bitcoin/bitcoin/pull/19572/commits/003767e7a92e8ae41f5c561a69d7be23e4753f63 in for now out of a real reason(it's changing how the legacy zmq notifications work under the hood) and laziness.

Should have addessed everything. Did some history cleanup as well.

instagibbs force-pushed on Sep 4, 2020

instagibbs commented at 7:47 PM on September 4, 2020: member

@mruddy in case you're interested(I don't know if you're on IRC)

DrahtBot added the label Needs rebase on Sep 5, 2020

instagibbs force-pushed on Sep 5, 2020

instagibbs commented at 4:17 PM on September 5, 2020: member

rebased, looks like appveyor and one of the cirrus builds is having the sads

DrahtBot removed the label Needs rebase on Sep 5, 2020

MarcoFalke commented at 5:48 PM on September 5, 2020: member

You might have to add a suppression, not sure if the tsan failue is a false positive

instagibbs commented at 9:01 PM on September 6, 2020: member

Can't seem to replicate locally with tsan.

instagibbs requested review from jonatack on Sep 8, 2020

instagibbs commented at 12:55 PM on September 8, 2020: member

@n-thumann (can't seem to re-request with your stale review, thanks github)

n-thumann commented at 5:21 PM on September 8, 2020: contributor

@n-thumann (can't seem to re-request with your stale review, thanks github)

re-ACK c2f03fd7b9090ac17299190f118511326483f23c ✌️

in src/zmq/zmqnotificationinterface.cpp:167 in c2f03fd7b9 outdated

 166 |  
 167 |      for (std::list<CZMQAbstractNotifier*>::iterator i = notifiers.begin(); i!=notifiers.end(); )
 168 |      {
 169 |          CZMQAbstractNotifier *notifier = *i;
 170 | -        if (notifier->NotifyTransaction(tx))
 171 | +        if (notifier->NotifyTransaction(tx) && notifier->NotifyTransactionAcceptance(tx, mempool_sequence))

jonatack commented at 7:57 PM on September 8, 2020:

error here while building 712f214

zmq/zmqnotificationinterface.cpp: In member function ‘virtual void CZMQNotificationInterface::TransactionAddedToMempool(const CTransactionRef&, uint64_t)’:
zmq/zmqnotificationinterface.cpp:167:106: error: no matching function for call to ‘CZMQAbstractNotifier::NotifyTransactionAcceptance(const CTransaction&, uint64_t&)’
  167 |         if (notifier->NotifyTransaction(tx) && notifier->NotifyTransactionAcceptance(tx, mempool_sequence))
      |                                                                                                          ^
In file included from ./zmq/zmqpublishnotifier.h:8,
                 from zmq/zmqnotificationinterface.cpp:6:
./zmq/zmqabstractnotifier.h:50:18: note: candidate: ‘virtual bool CZMQAbstractNotifier::NotifyTransactionAcceptance(const CTransaction&)’
   50 |     virtual bool NotifyTransactionAcceptance(const CTransaction &transaction);
      |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
./zmq/zmqabstractnotifier.h:50:18: note:   candidate expects 1 argument, 2 provided

in src/validation.cpp:1061 in c2f03fd7b9 outdated

1051 | @@ -1052,7 +1052,7 @@ bool MemPoolAccept::AcceptSingleTransaction(const CTransactionRef& ptx, ATMPArgs
1052 |  
1053 |      if (!Finalize(args, workspace)) return false;
1054 |  
1055 | -    GetMainSignals().TransactionAddedToMempool(ptx);
1056 | +    GetMainSignals().TransactionAddedToMempool(ptx, m_pool.GetAndIncrementSequence());

jonatack commented at 7:59 PM on September 8, 2020:

error here while building 712f214

validation.cpp: In member function ‘bool {anonymous}::MemPoolAccept::AcceptSingleTransaction(const CTransactionRef&, {anonymous}::MemPoolAccept::ATMPArgs&)’:
validation.cpp:1055:60: error: ‘class CTxMemPool’ has no member named ‘GetAndIncrementSequence’
 1055 |     GetMainSignals().TransactionAddedToMempool(ptx, m_pool.GetAndIncrementSequence());
      |                                                            ^~~~~~~~~~~~~~~~~~~~~~~
make[2]: *** [Makefile:12086: libbitcoin_server_a-validation.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[2]: Leaving directory '/home/jon/projects/bitcoin/bitcoin/src'
make[1]: *** [Makefile:18955: all-recursive] Error 1
make[1]: Leaving directory '/home/jon/projects/bitcoin/bitcoin/src'
make: *** [Makefile:799: all-recursive] Error 1

jonatack commented at 8:01 PM on September 8, 2020: member

A couple of errors while building 712f214.

instagibbs force-pushed on Sep 8, 2020

instagibbs commented at 8:53 PM on September 8, 2020: member

collapsed the two logic commits since it's not that much code and lots of lines changed twice in a row

in test/functional/interface_zmq.py:8 in 4e1cc1d888 outdated

   4 | @@ -5,13 +5,24 @@
   5 |  """Test the ZMQ notification interface."""
   6 |  import struct
   7 |  
   8 | -from test_framework.address import ADDRESS_BCRT1_UNSPENDABLE
   9 | +from test_framework.address import ADDRESS_BCRT1_UNSPENDABLE, ADDRESS_BCRT1_P2WSH_OP_TRUE

jonatack commented at 7:50 AM on September 9, 2020:

bb5e3a34 nit sort

from test_framework.address import ADDRESS_BCRT1_P2WSH_OP_TRUE, ADDRESS_BCRT1_UNSPENDABLE

in src/rpc/blockchain.cpp:567 in 4e1cc1d888 outdated

 543 | @@ -529,6 +544,15 @@ static UniValue getrawmempool(const JSONRPCRequest& request)
 544 |                          {
 545 |                              {RPCResult::Type::OBJ, "transactionid", "", MempoolEntryDescription()},
 546 |                          }},
 547 | +                    RPCResult{"for verbose = false and mempool_sequence = true",
 548 | +                        RPCResult::Type::OBJ, "", "",
 549 | +                        {
 550 | +                            {RPCResult::Type::NUM, "mempool_sequence", "The mempool sequence value."},
 551 | +                            {RPCResult::Type::ARR, "txids", "",

jonatack commented at 8:06 AM on September 9, 2020:

885212e could you please inverse the order of txids and mempool_sequence fields here in the help to be the same as the rpc output

                         {
-                            {RPCResult::Type::NUM, "mempool_sequence", "The mempool sequence value."},
                             {RPCResult::Type::ARR, "txids", "",
                             {
                                 {RPCResult::Type::STR_HEX, "", "The transaction id"},
                             }},
+                            {RPCResult::Type::NUM, "mempool_sequence", "The mempool sequence value."},
                         }},

instagibbs commented at 2:17 PM on September 9, 2020:

done

in test/functional/interface_zmq.py:9 in 4e1cc1d888 outdated

   4 | @@ -5,13 +5,24 @@
   5 |  """Test the ZMQ notification interface."""
   6 |  import struct
   7 |  
   8 | -from test_framework.address import ADDRESS_BCRT1_UNSPENDABLE
   9 | +from test_framework.address import ADDRESS_BCRT1_UNSPENDABLE, ADDRESS_BCRT1_P2WSH_OP_TRUE
  10 | +from test_framework.blocktools import create_block, create_coinbase, add_witness_commitment

jonatack commented at 8:10 AM on September 9, 2020:

99a94fe

from test_framework.blocktools import add_witness_commitment, create_block, create_coinbase

in test/functional/interface_zmq.py:15 in 4e1cc1d888 outdated

  13 | -from test_framework.util import assert_equal, connect_nodes
  14 | +from test_framework.messages import CTransaction, hash256, FromHex
  15 | +from test_framework.util import (
  16 | +    assert_equal,
  17 | +    connect_nodes,
  18 | +    assert_raises_rpc_error,

jonatack commented at 8:11 AM on September 9, 2020:

99a94fe nit sort

jonatack commented at 8:24 AM on September 9, 2020: member

Tested ACK 4e1cc1d88889784e962e4393091c445b6561df9d

Aligning the rpc output and help would be good. Feel free to ignore the test import order nits.

instagibbs force-pushed on Sep 9, 2020

jonatack commented at 2:15 PM on September 10, 2020: member

re-ACK 4baaa89d29057e524a38e120512664ffacce413d per git range-diff 564e1ab 4e1cc1d 4baaa89, manually re-ran getrawmempool and its help for the fun

new help section

$ bitcoin-cli -regtest help getrawmempool

Result (for verbose = false and mempool_sequence = true):
{                            (json object)
  "txids" : [                (json array)
    "hex",                   (string) The transaction id
    ...
  ],
  "mempool_sequence" : n     (numeric) The mempool sequence value.
}

getrawmempool with mempool_sequence

$ bitcoin-cli -regtest getrawmempool false true
{
  "txids": [
  ],
  "mempool_sequence": 1
}

getrawmempool with bad params combination

$ bitcoin-cli -regtest getrawmempool true true
error code: -8
error message:
Verbose results cannot contain mempool sequence values.

fanquake referenced this in commit dffefda21d on Sep 11, 2020

sidhujag referenced this in commit 27c6cdde45 on Sep 11, 2020

in src/wallet/wallet.cpp:1237 in 4baaa89d29 outdated

1226 | @@ -1227,7 +1227,7 @@ void CWallet::blockConnected(const CBlock& block, int height)
1227 |      m_last_block_processed = block_hash;
1228 |      for (size_t index = 0; index < block.vtx.size(); index++) {
1229 |          SyncTransaction(block.vtx[index], {CWalletTx::Status::CONFIRMED, height, block_hash, (int)index});
1230 | -        transactionRemovedFromMempool(block.vtx[index], MemPoolRemovalReason::BLOCK);
1231 | +        transactionRemovedFromMempool(block.vtx[index], MemPoolRemovalReason::BLOCK, 0 /* mempool_sequence */);

0xB10C commented at 6:35 AM on September 14, 2020:

Why is mempool_sequence == 0 here?

instagibbs commented at 1:55 PM on September 14, 2020:

it's a required chain interface argument, but the wallet doesn't care/have access to about the mempool sequence value.

0xB10C commented at 7:20 AM on September 14, 2020: member

Concept ACK for using a ZMQ feed for tracking mempool and chain changes. The sequence number allows to order and detect missing mempool changes. This seems helpful.

This change allows Bitcoin Core users to keep track of their mempool in an external service connected via ZMQ. It makes the external detection of reorgs easier as every block connection is notified (not only changes to the block tip as its the case with the other *block topics).

On the approach: IMO the sequence notifier could use proper ZMQ multipart messages instead of in-lining a new protocol. https://github.com/bitcoin/bitcoin/pull/19572/files#diff-d8e6fe13399f13c42a93ec8326c60614R94-R97

now:
| topic | <32-byte hash>R<8-byte LE uint> | zmq sequence num |

multipart:
| topic | 32-byte-hash | char | (optional) mempool sequence | zmq sequence |

I mentioned something like this in #17878 (review) as a concept alternative as well. However, I think adding proper ZMQ multipart message support to CZMQAbstractPublishNotifier::SendMessage could be a change in its own. This could, for example, look like this SendMessage with this zmq_send_multipart. Let me know what you think.

0xB10C commented at 9:38 AM on September 14, 2020: member

Had a quick look at the Bitcoin Core is restarted and transactions are added to the mempool from mempool.dat case. These are correctly notified, but don't necessary have the same mempool_sequence (that's expected). With a mempool.dat containing multiple thousand transactions the ZMQ buffer could fill up when using the DEFAULT_ZMQ_SNDHWM of 1000 resulting in ZMQ messages being dropped. This is, however, nothing new and can happen with the existing *tx ZMQ publishers as well.

Here, the ZMQ buffers could fill up when multiple thousand transactions are removed from the mempool (this excludes confirmations). This would result in some removal notifications not reaching the external service. But I believe these events to be rare. It's more something a user of the new ZMQ sequence endpoint should keep in mind. Not really specific to this PR.

instagibbs commented at 1:53 PM on September 14, 2020: member

@0xB10C thanks for the review!

Discussion about dropped messages

Yes, this is a general problem that can be mostly avoided, and if not the endpoint logic should basically toss what it thinks it knows and restart its tracking.

Multipart messages

I'm not sure if the change alone is worth it for current messages. I think it really shines when there are optional messages, of which we currently have none. Is this a fair assessment? A potential application would be for example we publish optional mempool eviction reasons. I'll review/pocket https://github.com/0xB10C/bitcoin/commit/a06f5c7db6c0e491f150fe5dafb864c15326bb71# for now but if we decide to break format again, I think it's worth inclusion.

DrahtBot added the label Needs rebase on Sep 19, 2020

instagibbs force-pushed on Sep 21, 2020

instagibbs commented at 4:01 PM on September 21, 2020: member

rebased. No functional changes.

DrahtBot removed the label Needs rebase on Sep 21, 2020

in src/zmq/zmqnotificationinterface.cpp:144 in 877f511d7f outdated

 140 | @@ -140,31 +141,53 @@ void CZMQNotificationInterface::UpdatedBlockTip(const CBlockIndex *pindexNew, co
 141 |      });
 142 |  }
 143 |  
 144 | -void CZMQNotificationInterface::TransactionAddedToMempool(const CTransactionRef& ptx)
 145 | +void CZMQNotificationInterface::TransactionAddedToMempool(const CTransactionRef& ptx,  uint64_t mempool_sequence)

jonatack commented at 8:15 PM on September 21, 2020:

pico-nit 68b1e612 extra space in last rebase, ignore unless you have to retouch again

instagibbs commented at 3:21 PM on September 22, 2020:

you're in luck, rebasing again. done.

jonatack commented at 8:16 PM on September 21, 2020: member

Code review re-ACK 877f511d7f1f3c37a86c421e21fc0694c387e69f per git range-diff c0c409d 4baaa89 877f511, looked at the new function TryForEachAndRemoveFailed() that caused most of the rebase, verified debug build is clean and the changed test interface_zmq.py passes.

instagibbs requested review from luke-jr on Sep 21, 2020

n-thumann commented at 9:29 PM on September 21, 2020: contributor

re-ACK https://github.com/bitcoin/bitcoin/commit/877f511d7f1f3c37a86c421e21fc0694c387e69f

zmq test: Actually make reorg occur 1b615e61bf

instagibbs force-pushed on Sep 22, 2020

Add 'sequence' zmq publisher to track all block (dis)connects, mempool deltas

Using the zmq notifications to avoid excessive mempool polling can be difficult
given the current notifications available. It announces all transactions
being added to mempool or included in blocks, but announces no evictions
and gives no indication if the transaction is in the mempool or a block.

Block notifications for zmq are also substandard, in that it only announces
block tips, while all block transactions are still announced.

This commit adds a unified stream which can be used to closely track mempool:

1) getrawmempool to fill out mempool knowledge
2) if txhash is announced, add or remove from set
based on add/remove flag
3) if blockhash is announced, get block txn list,
remove from those transactions local view of mempool
4) if we drop a sequence number, go to (1)

The mempool sequence number starts at the value 1, and
increments each time a transaction enters the mempool,
or is evicted from the mempool for any reason, including
block inclusion. The mempool sequence number is published
via ZMQ for any transaction-related notification.

These features allow for ZMQ/RPC consumer to track mempool
state in a more exacting way, without unnecesarily polling
getrawmempool. See interface_zmq.py::test_mempool_sync for
example usage.

e76fc2b84d

Add functional tests for zmq sequence topic and mempool sequence logic 68c3c7e1bd

Update zmq notification documentation and sample consumer 759d94e70f

instagibbs force-pushed on Sep 22, 2020

instagibbs commented at 3:36 PM on September 22, 2020: member

rebased again

laanwj commented at 11:53 AM on September 23, 2020: member

Code review ACK 759d94e70f6844443106404882c7b105f3a4dba7

I see a potential confusion between sequence numbers of the individual packets and the "mempool sequence" but this is a matter of documentation, I don't have a suggestion for better naming.

laanwj merged this on Sep 23, 2020

laanwj closed this on Sep 23, 2020

instagibbs commented at 11:58 AM on September 23, 2020: member

I see a potential confusion between sequence numbers of the individual packets and the "mempool sequence" but this is a matter of documentation, I don't have a suggestion for better naming.

Willing to entertain any renaming if people think of something better!

jonatack commented at 2:02 PM on September 23, 2020: member

re-ACK 759d94e70f6844443106404882c7b105f3a4dba7

shuckc commented at 2:34 PM on September 23, 2020: none

Lovely! Very much look forward to using this.

sidhujag referenced this in commit e38523bda0 on Sep 23, 2020

domob1812 referenced this in commit c8d0274012 on Sep 28, 2020

PastaPastaPasta referenced this in commit bc18d5d458 on Jun 27, 2021

PastaPastaPasta referenced this in commit 9762ada89e on Jun 28, 2021

PastaPastaPasta referenced this in commit 36237fe794 on Jun 29, 2021

PastaPastaPasta referenced this in commit 66c1ad151a on Jul 1, 2021

PastaPastaPasta referenced this in commit b17d32b26f on Jul 1, 2021

PastaPastaPasta referenced this in commit 6ef9992a26 on Jul 15, 2021

PastaPastaPasta referenced this in commit 788e3d31b4 on Jul 16, 2021

Fabcien referenced this in commit 3cf2c996d8 on Oct 14, 2021

Fabcien referenced this in commit 18db6565c0 on Oct 14, 2021

Fabcien referenced this in commit 5ec80435a5 on Oct 14, 2021

Fabcien referenced this in commit 0f144de39f on Oct 14, 2021

DrahtBot locked this on Feb 15, 2022

ZMQ: Create "sequence" notifier, enabling client-side mempool tracking #19572

Conflicts

I think you could extract CValidationInterface changes to a different PR.

oops 😅 good catch.

@luke-jr commented on this pull request.