Add zmq notifications for evicted transactions, tests

instagibbs commented at 8:11 pm on July 7, 2020: member

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 commented at 8:32 pm on July 7, 2020: contributor

Concept ACK.

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

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

DrahtBot added the label Tests on Jul 7, 2020

in src/init.cpp:484 in 678d449d65 outdated

479@@ -480,20 +480,26 @@ void SetupServerArgs(NodeContext& node)
480 
481 #if ENABLE_ZMQ
482     gArgs.AddArg("-zmqpubhashblock=<address>", "Enable publish hash block in <address>", ArgsManager::ALLOW_ANY, OptionsCategory::ZMQ);
483-    gArgs.AddArg("-zmqpubhashtx=<address>", "Enable publish hash transaction in <address>", ArgsManager::ALLOW_ANY, OptionsCategory::ZMQ);
484+    gArgs.AddArg("-zmqpubhashtx=<address>", "Enable publish hash transaction in <address> on entry into mempool, and appearance in blocks", ArgsManager::ALLOW_ANY, OptionsCategory::ZMQ);
485+    gArgs.AddArg("-zmqpubhashtxevict=<address>", "Enable publish hash transaction in <address>  upon mempool eviction other than through block inclusion", ArgsManager::ALLOW_ANY, OptionsCategory::ZMQ);

promag commented at 8:40 pm on July 7, 2020:

nit, double space.

instagibbs commented at 1:59 am on July 8, 2020:

done

DrahtBot commented at 9:05 pm on July 7, 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:

#18309 (zmq: Add support to listen on multiple interfaces by n-thumann)
#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 commented at 9:45 pm on July 7, 2020: member

Concept ACK.

instagibbs force-pushed on Jul 8, 2020

instagibbs commented at 2:00 am on July 8, 2020: member

Pushed some fixes, and documentation.

instagibbs commented at 1:21 pm on July 8, 2020: member

Single run failure unrelated:

0  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
1    self.nodes[1].abandontransaction(tx3_id)

restarted

MarcoFalke removed the label Tests on Jul 8, 2020

laanwj commented at 1:00 pm on July 9, 2020: member

Concept ACK

in src/zmq/zmqabstractnotifier.cpp:24 in b9f4e8dbe6 outdated

19@@ -20,3 +20,8 @@ bool CZMQAbstractNotifier::NotifyTransaction(const CTransaction &/*transaction*/
20 {
21     return true;
22 }
23+
24+bool CZMQAbstractNotifier::NotifyTransactionEviction(const CTransaction &/*transaction*/, MemPoolRemovalReason &reason)

laanwj commented at 1:28 pm on July 9, 2020:

I don’t think it makes sense to pass reason as a reference here. Let alone a mutable one!

instagibbs commented at 5:46 pm on July 10, 2020:

Done.

in src/zmq/zmqabstractnotifier.h:46 in b9f4e8dbe6 outdated

42@@ -42,6 +43,7 @@ class CZMQAbstractNotifier
43 
44     virtual bool NotifyBlock(const CBlockIndex *pindex);
45     virtual bool NotifyTransaction(const CTransaction &transaction);
46+    virtual bool NotifyTransactionEviction(const CTransaction &transaction, MemPoolRemovalReason &reason);

laanwj commented at 1:28 pm on July 9, 2020:

Same comment re: use of reference parameter.

in src/zmq/zmqpublishnotifier.cpp:188 in b9f4e8dbe6 outdated

179@@ -178,6 +180,16 @@ bool CZMQPublishHashTransactionNotifier::NotifyTransaction(const CTransaction &t
180     return SendMessage(MSG_HASHTX, data, 32);
181 }
182 
183+bool CZMQPublishHashTransactionEvictionNotifier::NotifyTransactionEviction(const CTransaction &transaction, MemPoolRemovalReason &reason)
184+{
185+    uint256 hash = transaction.GetHash();
186+    LogPrint(BCLog::ZMQ, "zmq: Publish hashtxevict %s\n", hash.GetHex());
187+    char data[32];
188+    for (unsigned int i = 0; i < 32; i++)

laanwj commented at 1:31 pm on July 9, 2020:

Do we want to report the reason here? No strong reason here. On one hand, reasons are an implementation detail, on the other, for statistics it could be useful to report it (I think back in the day when I had a patch to add this functionality, I did add the reason, though converted to a interface-specific enum).

instagibbs commented at 1:35 pm on July 9, 2020:

Just a hunch but I think in general “no”. Anyone who wants to do statistics on mempool drops can look at logs after the fact? The aim of this PR is primarily for timely notification of transaction changes in “known” mempool.

laanwj changes_requested

shuckc commented at 1:51 pm on July 9, 2020: none

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 commented at 7:34 pm on July 9, 2020: member

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

instagibbs commented at 7:41 pm on July 9, 2020: member

@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!)

in src/zmq/zmqnotificationinterface.cpp:184 in b9f4e8dbe6 outdated

178@@ -177,6 +179,25 @@ void CZMQNotificationInterface::TransactionAddedToMempool(const CTransactionRef&
179     }
180 }
181 
182+void CZMQNotificationInterface::TransactionRemovedFromMempool(const CTransactionRef& ptx, MemPoolRemovalReason reason)
183+{
184+     const CTransaction& tx = *ptx;

fjahr commented at 2:23 pm on July 10, 2020:

nit: looks like there is an extra space here

in src/zmq/zmqnotificationinterface.cpp:182 in b9f4e8dbe6 outdated

178@@ -177,6 +179,25 @@ void CZMQNotificationInterface::TransactionAddedToMempool(const CTransactionRef&
179     }
180 }
181 
182+void CZMQNotificationInterface::TransactionRemovedFromMempool(const CTransactionRef& ptx, MemPoolRemovalReason reason)

fjahr commented at 2:29 pm on July 10, 2020:

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

instagibbs force-pushed on Jul 10, 2020

instagibbs commented at 5:46 pm on July 10, 2020: member

pushed fixups

in src/zmq/zmqpublishnotifier.cpp:195 in 432a3339d8 outdated

179@@ -178,6 +180,16 @@ bool CZMQPublishHashTransactionNotifier::NotifyTransaction(const CTransaction &t
180     return SendMessage(MSG_HASHTX, data, 32);
181 }
182 
183+bool CZMQPublishHashTransactionEvictionNotifier::NotifyTransactionEviction(const CTransaction &transaction, MemPoolRemovalReason reason)

fjahr commented at 6:18 pm on July 10, 2020:

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

instagibbs commented at 2:26 pm on July 13, 2020:

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

fjahr commented at 6:19 pm on July 10, 2020: member

Concept ACK

instagibbs force-pushed on Jul 13, 2020

Make ordering of zmq consumption irrelevant to functional test e70512a83c

Add test case for mempool->block zmq notification 2399a0600c

Add zmq test for transaction pub during reorg a0f4f9c983

Have zmq reorg test cover mempool txns 7356292e1d

Add zmq notifications for evicted transactions, tests 544441e94f

Deduplicate zmq txn message code 78a958372e

instagibbs force-pushed on Jul 14, 2020

instagibbs commented at 5:15 pm on July 14, 2020: member

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

instagibbs closed this on Jul 14, 2020

laanwj referenced this in commit 9e217f5a6f on Sep 23, 2020

sidhujag referenced this in commit e38523bda0 on Sep 23, 2020

DrahtBot locked this on Feb 15, 2022

Add zmq notifications for evicted transactions, tests #19462

Conflicts