Add zmq notifications for evicted transactions, tests #19462

instagibbs · 2020-07-07T20:11:23Z

Currently the only zmq notifications that exist for transactions are for:

Entry into mempool
Appearance in blocks
Disconnect from block

For mempool-tracking purposes, it's useful to know when transactions are being dropped from the local mempool without relying on a tight polling loop. This notification publishes anytime a transaction is removed from the mempool, aside from the case where it is confirmed in the new incoming block(already published in 2 above).

Based on #19507

Additional test cases that could be added:

block conflict
mempool timeout
mempool trim to size

andrewtoth · 2020-07-07T20:32:01Z

Concept ACK.

Definitely useful since there's no easy way to currently track replacement for systems listening to bitcoind.

DrahtBot · 2020-07-07T21:05:19Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

zmq: Add support to listen on multiple interfaces #18309 (zmq: Add support to listen on multiple interfaces by n-thumann)
ZMQ: Small cleanups in the ZMQ code #13686 (ZMQ: Small cleanups in the ZMQ code by domob1812)

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

Concept ACK.

src/init.cpp

instagibbs · 2020-07-08T02:00:10Z

Pushed some fixes, and documentation.

instagibbs · 2020-07-08T13:21:14Z

Single run failure unrelated:

  File "/home/travis/build/bitcoin/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/feature_backwards_compatibility.py", line 106, in run_test
    self.nodes[1].abandontransaction(tx3_id)

restarted

laanwj · 2020-07-09T13:00:56Z

Concept ACK

src/zmq/zmqabstractnotifier.cpp

src/zmq/zmqabstractnotifier.h

src/zmq/zmqpublishnotifier.cpp

shuckc · 2020-07-09T13:51:54Z

At the moment, the way that I synchronise with mempool contents is to poll getrawmempool and listen out for hashtx notifications, with the caveat that evictions are only detected by the poll. Since you are looking to address that here, Concept Ack from me.

One consideration is that it's not currently possible to perfectly synchronise the poll of getrawmempool with the zmq announcements, as they share no common sequence number. This can cause you to interpret an addition as a removal and visa-versa when checking for a tx hash in your local mempool transaction set. This get slightly worse with each additional zmq notifier you have to listen to, since they are typically (see lnd) configured on separate TCP connections, so TCP packet loss (causing delays on one stream) and reconnections can cause ordering issues. The configuration on separate connections seems to be an (now unnecessary) piece of inherited wisdom from before the publishing buffer size was tuneable with the High Water Mark value.

Since this implementation adds another new zmq notifier, it might be worthwhile to take this opportunity to design the notifier such that it can be used exclusively to maintain mempool synchronisation, then you would only need to query getrawmempool at startup and after seeing a gap on the zmq channel sequence number. This would require a message format that is something slightly more complex.

Could I suggest adding a new notifier called mempoolsync where the messages are:

C <block hash>              connect block hash
D <block hash>              disconnect block hash
A <txn hash>                add new transaction into mempool (ie. via rpc)
R <txn hash> <txn hash>     replace transaction hash with another
X <txn hash>                mempool eviction

Since this new notifier has all the relevant events for mempool synchonisation strongly sequenced by a single notifier sequence number, you could then (when zmq is compiled in) add an additional field in the return value for getrawmempool that is the last published notification value at the time the result was prepared. Of course to actually maintain mempool synchronisation, several of these events require calls to be made to bitcoind to decode various things depending on your use case, but that is relatively straightforward.

luke-jr · 2020-07-09T19:34:54Z

Couldn't this just be a new message for the existing endpoints?

instagibbs · 2020-07-09T19:41:10Z

@luke-jr I thought I'd break someone's integration perhaps if I did so since there's no "options" or anything in current messages, just txids AFAICT. (maybe I'm wrong!)

instagibbs · 2020-07-10T17:46:42Z

pushed fixups

fjahr

Concept ACK

src/zmq/zmqnotificationinterface.cpp

fjahr · 2020-07-10T14:29:43Z

src/zmq/zmqnotificationinterface.cpp

@@ -177,6 +179,25 @@ void CZMQNotificationInterface::TransactionAddedToMempool(const CTransactionRef&
    }
 }

+void CZMQNotificationInterface::TransactionRemovedFromMempool(const CTransactionRef& ptx, MemPoolRemovalReason reason)


Also a some code duplication here with the two functions above.

fjahr · 2020-07-10T18:18:35Z

src/zmq/zmqpublishnotifier.cpp

@@ -178,6 +180,16 @@ bool CZMQPublishHashTransactionNotifier::NotifyTransaction(const CTransaction &t
    return SendMessage(MSG_HASHTX, data, 32);
 }

+bool CZMQPublishHashTransactionEvictionNotifier::NotifyTransactionEviction(const CTransaction &transaction, MemPoolRemovalReason reason)


Code between this function and NotifyTransaction is almost identical, could be de-dublicated, maybe also together with NotifyBlock but it's not blocking IMO

Deduplicated the txn publishing portion, left block stuff alone for now.

instagibbs · 2020-07-14T17:15:58Z

Oddly enough I'm going to concept NACK my own PR.

After spending some time trying to write a "mempool tracker" via ZMQ with mempool sequence numbers, this plus legacy zmq notifications are insufficient.

The hashtx feed really isn't a mempool feed, it's a feed of "transactions I saw somewhere", which is pretty obnoxious and hard to reason about and be very useful, especially in the presence of reorgs.
The "eviction" feed here doesn't get told about transactions removed via block inclusion. This means relying on the hashtx feed, but there are no mempool sequence numbers attached to those because chainstate has no clue what a mempool is.

In my estimation, these transaction feeds can be subsumed by a single feed that is mempooltx, which includes both inclusion and exclusion from the mempool for any reason, with the appropriate label on the report. This way we don't have to touch older notifications at all instead of adding a bunch of options here and there to achieve the goal.

mempooltx allows you to track exactly what is in the mempool(assuming no dropped messages), and blockhash is used to trigger block transaction fetching(to show inclusion, exclusion via reog, etc).

…empool 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 #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) ACKs for top commit: laanwj: Code review ACK 759d94e Tree-SHA512: 9daf0d7d996190f3a68ff40340a687519323d7a6c51dcb26be457fbc013217ea7b62fbd0700b74b654433d2e370704feb61e5584399290692464fcfcb72ce3b7

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

DrahtBot added RPC/REST/ZMQ Tests labels Jul 7, 2020

promag reviewed Jul 7, 2020

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Jul 8, 2020

zmq: Add support to listen on multiple interfaces #18309

Merged

instagibbs force-pushed the notify_eviction branch from 678d449 to b9f4e8d Compare July 8, 2020 01:59

DrahtBot mentioned this pull request Jul 8, 2020

ZMQ: Small cleanups in the ZMQ code #13686

Merged

maflcko removed the Tests label Jul 8, 2020

laanwj requested changes Jul 9, 2020

View reviewed changes

src/zmq/zmqabstractnotifier.cpp Outdated Show resolved Hide resolved

src/zmq/zmqabstractnotifier.h Outdated Show resolved Hide resolved

src/zmq/zmqpublishnotifier.cpp Outdated Show resolved Hide resolved

instagibbs force-pushed the notify_eviction branch from b9f4e8d to 432a333 Compare July 10, 2020 17:45

fjahr reviewed Jul 10, 2020

View reviewed changes

instagibbs force-pushed the notify_eviction branch 2 times, most recently from ee86391 to 0686a96 Compare July 13, 2020 14:28

instagibbs added 3 commits July 13, 2020 14:18

Make ordering of zmq consumption irrelevant to functional test

e70512a

Add test case for mempool->block zmq notification

2399a06

Add zmq test for transaction pub during reorg

a0f4f9c

DrahtBot mentioned this pull request Jul 14, 2020

Expand functional zmq transaction tests #19507

Merged

instagibbs added 3 commits July 14, 2020 10:14

Have zmq reorg test cover mempool txns

7356292

Add zmq notifications for evicted transactions, tests

544441e

Deduplicate zmq txn message code

78a9583

instagibbs force-pushed the notify_eviction branch from 0686a96 to 78a9583 Compare July 14, 2020 14:18

instagibbs closed this Jul 14, 2020

instagibbs mentioned this pull request Jul 23, 2020

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

Merged

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

Add zmq notifications for evicted transactions, tests #19462

Add zmq notifications for evicted transactions, tests #19462

Uh oh!

Conversation

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

Uh oh!

Uh oh!

andrewtoth commented Jul 7, 2020

Uh oh!

DrahtBot commented Jul 7, 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!

Uh oh!

instagibbs commented Jul 8, 2020

Uh oh!

instagibbs commented Jul 8, 2020

Uh oh!

laanwj commented Jul 9, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

luke-jr commented Jul 9, 2020

Uh oh!

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

Uh oh!

Uh oh!

instagibbs commented Jul 10, 2020

Uh oh!

fjahr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fjahr Jul 10, 2020

Choose a reason for hiding this comment

Uh oh!

fjahr Jul 10, 2020

Choose a reason for hiding this comment

Uh oh!

instagibbs Jul 13, 2020

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Jul 14, 2020

Uh oh!

Uh oh!

instagibbs commented Jul 7, 2020 •

edited

Loading

DrahtBot commented Jul 7, 2020 •

edited

Loading

shuckc commented Jul 9, 2020 •

edited

Loading

instagibbs commented Jul 9, 2020 •

edited

Loading