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

instagibbs · 2020-07-23T15:06:31Z

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 · 2020-07-23T15:57:33Z

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 · 2020-07-23T16:57:51Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

test: Replace (dis)?connect_nodes globals with TestFramework methods #19967 (test: Replace (dis)?connect_nodes globals with TestFramework methods by robot-dreams)
Avoid locking CTxMemPool::cs recursively in Mempool{Info}ToJSON() #19652 (Avoid locking CTxMemPool::cs recursively in Mempool{Info}ToJSON() by hebasto)
test: Mypy next steps #19532 (test: Mypy next steps by Ghorbanian)
rpc: remove deprecated CRPCCommand constructor #18531 (rpc: Assert that RPCArg names are equal to CRPCCommand ones by MarcoFalke)
zmq: Add support to listen on multiple interfaces #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.

promag

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.

promag · 2020-07-23T22:23:00Z

test/functional/interface_zmq.py

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

oops 😅 good catch.

src/txmempool.h

instagibbs · 2020-07-23T22:57:13Z

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 ***@***.***> wrote: ***@***.**** commented on this pull request. I still think #19476 <#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 <#19572 (comment)>: > @@ -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 <951507c> oops 😅 good catch. ------------------------------ In src/txmempool.h <#19572 (comment)>: > @@ -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 <#19572 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABMAFU6BN2AS2HWUGITFY7LR5C3UFANCNFSM4PF2HZKQ> .

promag · 2020-07-26T10:53:57Z

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

I wasn't suggesting otherwise 👍

jonasschnelli · 2020-07-27T07:50:19Z

Concept ACK

instagibbs · 2020-07-27T12:52:34Z

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 · 2020-07-27T15:14:54Z

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

instagibbs · 2020-07-27T16:12:59Z

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.

luke-jr · 2020-07-31T00:32:23Z

src/init.cpp

@@ -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"?

n-thumann · 2020-09-21T21:29:22Z

re-ACK 877f511

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

instagibbs · 2020-09-22T15:36:21Z

rebased again

laanwj · 2020-09-23T11:53:27Z

Code review ACK 759d94e

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.

instagibbs · 2020-09-23T11:58:08Z

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 · 2020-09-23T14:02:11Z

re-ACK 759d94e

shuckc · 2020-09-23T14:34:52Z

Lovely! Very much look forward to using this.

…-side mempool tracking 759d94e Update zmq notification documentation and sample consumer (Gregory Sanders) 68c3c7e Add functional tests for zmq sequence topic and mempool sequence logic (Gregory Sanders) e76fc2b Add 'sequence' zmq publisher to track all block (dis)connects, mempool deltas (Gregory Sanders) 1b615e6 zmq test: Actually make reorg occur (Gregory Sanders) Pull request description: 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 bitcoin#19462 (comment) Alternative to bitcoin#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) ACKs for top commit: laanwj: Code review ACK 759d94e Tree-SHA512: 9daf0d7d996190f3a68ff40340a687519323d7a6c51dcb26be457fbc013217ea7b62fbd0700b74b654433d2e370704feb61e5584399290692464fcfcb72ce3b7

Merged the Xaya-specific ZMQ notifications with the upstream extension for sequence (bitcoin/bitcoin#19572).

…sub.py file permissions 062e669 script: fix zmq_sub.py file permissions (Jon Atack) 36f8e0c doc: update PyZMQ installation instructions, ZeroMQ link (Jon Atack) Pull request description: Seen while reviewing bitcoin#19572. ACKs for top commit: theStack: ACK 062e669 🧷 fanquake: ACK 062e669 Tree-SHA512: 2210d92385377d066984d0a83882c3ece9f0f41c901b7eb375af9cdb57296f50f227c68193ccf35926073c2b788d58976442791a9fce2fc0f76452804d5cee6a

Summary: > 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. > > 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) This is a backport of [[bitcoin/bitcoin#19572 | core#19572]] [1/4] bitcoin/bitcoin@1b615e6 Note: I wasn't able to reproduce the issue that this commit is supposed to fix. I tried adding an extra line after generating the 2 new blocks on node 1, without being able to get an identical block: `assert disconnect_block not in connect_blocks` Test Plan: `test/functional/test_runner.py interface_zmq` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10303

Summary: This is a backport of [[bitcoin/bitcoin#19572 | core#19572]] [3/4] bitcoin/bitcoin@68c3c7e The main change from the original PR is the removal of the RBF tests. Depends on D10304 Test Plan: `test/functional/test_runner.py interface_zmq` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D10306

Summary: This is a backport of [[bitcoin/bitcoin#19572 | core#19572]] [4/4] bitcoin/bitcoin@759d94e Depends on D10306 I had to revert the IP address change from D1120 to make the zmq_sub.py script work. Test Plan: ``` $ src/bitcoind -daemon -zmqpubrawtx=tcp://127.0.0.1:28332 -zmqpubrawblock=tcp://127.0.0.1:28332 -zmqpubhashtx=tcp://127.0.0.1:28332 -zmqpubhashblock=tcp://127.0.0.1:28332 -zmqpubsequence=tcp://127.0.0.1:28332 $ python ../contrib/zmq/zmq_sub.py ``` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10307

Summary: Add a test to generate a "R" notification by: - sending a tx on node0 while the nodes are disconnected - verifying that this transaction is added to node0's mempool - generating a conflicting tx on node1 - mining the conflicting tx into a block on node1 - reconnecting the nodes - checking that the first tx is rejected on node0 Also add `noban` permission to all nodes to avoid the tx relay delays, because there are a lot of calls to `sync_all` and this diff adds more of them. This reduces the test run time from 45s to 14s. This replaces the RBF tests that were in [[bitcoin/bitcoin#19572 | [[bitcoin/bitcoin#19572 | core#19572]]]] backported in D10306. Test Plan: `test/functional/test_runner.py interface_zmq` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D10311

instagibbs force-pushed the zmq_sequence_all branch from c89b73a to 606a49b Compare July 23, 2020 15:15

DrahtBot added Mempool RPC/REST/ZMQ Tests Validation Wallet labels Jul 23, 2020

instagibbs force-pushed the zmq_sequence_all branch from 606a49b to 0e302f6 Compare July 23, 2020 17:08

promag reviewed Jul 23, 2020

View reviewed changes

This was referenced Jul 23, 2020

Log RPC parameters (arguments) if -debug=rpcparams #16365

Closed

Wallet passive startup #15719

Closed

instagibbs force-pushed the zmq_sequence_all branch from f3ec8d1 to b5de7c1 Compare July 27, 2020 19:59

This was referenced Jul 27, 2020

Add fee_est tool for debugging fee estimation code #10443

Closed

refactor: Pass ArgsManager into functions that register args #19561

Merged

refactor: Replace RecursiveMutex with Mutex in CTxMemPool #19306

Closed

DrahtBot added the Needs rebase label Jul 30, 2020

luke-jr reviewed Jul 31, 2020

View reviewed changes

zmq test: Actually make reorg occur

1b615e6

instagibbs force-pushed the zmq_sequence_all branch from 877f511 to 6a8c921 Compare September 22, 2020 15:23

instagibbs added 3 commits September 22, 2020 11:34

Add functional tests for zmq sequence topic and mempool sequence logic

68c3c7e

Update zmq notification documentation and sample consumer

759d94e

instagibbs force-pushed the zmq_sequence_all branch from 6a8c921 to 759d94e Compare September 22, 2020 15:34

laanwj merged commit 9e217f5 into bitcoin:master Sep 23, 2020

domob1812 added a commit to xaya/xaya that referenced this pull request Sep 28, 2020

Merge branch 'namecoin'

c8d0274

Merged the Xaya-specific ZMQ notifications with the upstream extension for sequence (bitcoin/bitcoin#19572).

ryanofsky mentioned this pull request Sep 28, 2020

Multiprocess bitcoin #10102

Draft

ghost mentioned this pull request Nov 18, 2020

ZMQ rawblock doesn't publish new block in case of chainsplit. #20376

Closed

ghost mentioned this pull request Nov 29, 2021

zmq: add rawmempooltx publisher #23624

Closed

3 tasks

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

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

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

Uh oh!

Conversation

instagibbs commented Jul 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

instagibbs commented Jul 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jul 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Jul 23, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

instagibbs commented Jul 23, 2020 via email

Uh oh!

promag commented Jul 26, 2020

Uh oh!

jonasschnelli commented Jul 27, 2020

Uh oh!

instagibbs commented Jul 27, 2020

Uh oh!

instagibbs commented Jul 27, 2020

Uh oh!

instagibbs commented Jul 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr Jul 31, 2020

Choose a reason for hiding this comment

Uh oh!

n-thumann commented Sep 21, 2020

Uh oh!

instagibbs commented Sep 22, 2020

Uh oh!

laanwj commented Sep 23, 2020

Uh oh!

instagibbs commented Sep 23, 2020

Uh oh!

jonatack commented Sep 23, 2020

Uh oh!

shuckc commented Sep 23, 2020

Uh oh!

Uh oh!

instagibbs commented Jul 23, 2020 •

edited

Loading

instagibbs commented Jul 23, 2020 •

edited

Loading

DrahtBot commented Jul 23, 2020 •

edited

Loading

instagibbs commented Jul 27, 2020 •

edited

Loading