zmq: add rawmempooltx publisher

0xB10C commented at 9:17 am on November 29, 2021: contributor

This adds a rawmempooltx publisher that only notifies raw transactions that are added to the mempool. The existing rawtx publisher notifies of transactions added to mempool and transactions appearing in blocks, causing raw transactions to often be published multiple times.

This caused confusion in #23471 (review), Ali Sherief [asked] for such a patch on the bitcoin-dev mailing list, and I’ve needed a ZMQ publisher for only mempool transactions multiple times. Also partially fixes #16180 from what I can tell.

I’ll leave this as a draft for now until:

I’ve gotten a few Concept ACKs that we want to add another publisher only for tx added to the mempool. (An alternative could be to extend the current rawtx publisher to include information about the reason for a transaction notification (mempool/block)).
I’ll add functional tests
I’ve gotten feedback on the publisher name rawmempooltx

I’d additionally find it useful to have an ZMQ multipart message part containing the transaction fee for mempool transactions. This requires us to change the interface though. I think this is out of scope for this PR.

zmq: add rawmempooltx publisher

The rawtx ZMQ publisher publishes transactions added to the mempool
and transactions in blocks. Often transactions are published multiple
times. Some applications only want to subscribe to mempool
transactions.

52c477e97f

fanquake added the label RPC/REST/ZMQ on Nov 29, 2021

promag commented at 9:46 am on November 29, 2021: member

Have you seen #7753?

ZenulAbidin commented at 10:54 am on November 29, 2021: contributor

For now, a workaround I found is to patch CZMQNotificationInterface::BlockDisconnected before building Bitcoin Core and comment out the loop that fires NotifyTransaction() like so:

 0void CZMQNotificationInterface::BlockDisconnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindexDisconnected)
 1{
 2/* ----- this part is deleted - causes Core to *not* publish confirmed transactions
 3    for (const CTransactionRef& ptx : pblock->vtx) {
 4        const CTransaction& tx = *ptx;
 5        TryForEachAndRemoveFailed(notifiers, [&tx](CZMQAbstractNotifier* notifier) {
 6            return notifier->NotifyTransaction(tx);
 7        });
 8    }
 9--------- */
10    // Next we notify BlockDisconnect listeners for *all* blocks
11    TryForEachAndRemoveFailed(notifiers, [pindexDisconnected](CZMQAbstractNotifier* notifier) {
12        return notifier->NotifyBlockDisconnect(pindexDisconnected);
13    });
14}

For reading only confirmed transactions (and throwing away the unconfirmed ones from the mempool), I believe a similar patch can be made to CZMQNotificationInterface::BlockDisconnected or CZMQNotificationInterface::TransactionAddedToMempool, but I have not tested this. But the procedure is similar.

Of course, I know that such a modification cannot be merged, I’m just leaving this here for anyone else who finds themselves in a similar situation in the future.

laanwj commented at 11:27 am on November 29, 2021: member

Concept ACK, I think having notifications to monitor the mempool makes sense, there has been interest in this in the past, I tried to do this at some point but lost interest.

drewx2 commented at 2:57 pm on November 29, 2021: none

The mempool only contains unconfirmed transactions and I don’t see that changing, therefore is the addition of the word “raw” to the flag/topic necessary?

ghost commented at 3:19 pm on November 29, 2021: none

Have you seen #7753?

According to the description of this PR:

Add notifications when transactions enter or leave the mempool. It also makes it possible to keep statistics on why transactions disappear from the mempool.

Isn’t this already possible with #19572 ?

benthecarman commented at 3:19 pm on November 29, 2021: contributor

The mempool only contains unconfirmed transactions and I don’t see that changing, therefore is the addition of the word “raw” to the flag/topic necessary?

I think so, it tells you that it will send the raw bytes of the tx rather than the txid

benthecarman commented at 5:33 pm on November 29, 2021: contributor

Concept ACK

drewx2 commented at 8:09 pm on November 29, 2021: none

The mempool only contains unconfirmed transactions and I don’t see that changing, therefore is the addition of the word “raw” to the flag/topic necessary?

I think so, it tells you that it will send the raw bytes of the tx rather than the txid

Good point. Though I think the use of “tx” and “txid” already provide context.

0xB10C commented at 1:29 pm on December 14, 2021: contributor

This topic needs documentation and a rebase with #23471 merged.

jb55 commented at 8:14 pm on December 26, 2021: contributor

Concept ACK

DrahtBot commented at 9:24 am on December 31, 2021: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17783 (util: Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior by ryanofsky)
#17581 (refactor: Remove settings merge reverse precedence code by ryanofsky)
#17580 (refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags by ryanofsky)
#17493 (util: Forbid ambiguous multiple assignments in config file by ryanofsky)

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.

0xB10C commented at 10:37 am on October 18, 2022: contributor

I haven’t gotten around to pick this up again and will close it for now. It has a few Concept ACKs so might be worth adding an up-for-grabs label.

0xB10C closed this on Oct 18, 2022

fanquake added the label Up for grabs on Oct 18, 2022

bitcoin locked this on Oct 18, 2023

zmq: add rawmempooltx publisher #23624

Conflicts