There is no way to only get real time notifications of transaction that affect the wallet. You have to do that manually by enabling zmqrawtx and filter out transactions.
I’m suggesting adding two new publisers, both for hash and raw wallet transactions.
Also topic will indicate if transaction came from mempool or block so developers can handle the transaction accordingly without a RPC round trip to bitcoind.
Tests and documentation are updated.
52@@ -52,6 +53,8 @@ struct CMainSignals {
53 boost::signals2::signal<void (const CBlockIndex *, const CBlockIndex *, bool fInitialDownload)> UpdatedBlockTip;
54 /** Notifies listeners of a transaction having been added to mempool. */
55 boost::signals2::signal<void (const CTransactionRef &)> TransactionAddedToMempool;
56+ /** Notifies listeners of a transaction having been added to the wallet. */
57+ boost::signals2::signal<void (const CTransactionRef &, uint256 hashBlock)> TransactionAddedWallet;
const uint256& instead of uint256 arg type to avoid unnecessary copies.
76@@ -75,6 +77,11 @@ notification `-zmqpubhashtx` the topic is `hashtx` (no null
77 terminator) and the body is the hexadecimal transaction hash (32
78 bytes).
79
80+For wallet transaction notifications (both hash and tx), the topic also indicate if the transaction came from a block or mempool.
Concept ACK - however some comments:
ENABLE_WALLET, for compiling without wallet support-wallet=0CValidationInterface, it is for validation events only. To do this correctly, zmq should subscribe to wallet events through CWallet directly.
const uint256 &hashBlock
@laanwj using wallet directly (without going through CValidationInterface) and respecting ENABLE_WALLET. Respecting -wallet=0 by checking if pwalletMain is not null.
zeromq does have authentication, but it is not being used within bitcoind. Do you think it is needed here? my only counter argument is that it should be exposed to internal network and trusted peers only.
But if you feel that it is needed I can add authentication for the wallet publishers.
Do you think it is needed here? my only counter argument is that it should be exposed to internal network and trusted peers only.
We could just add a warning (to the option help enabling this) that the API is unauthenticated, and thus wallet notifications should not be used when the zmq endpoints are accessible to other users - they can be restricted by other means, e.g. binding locally, binding to UNIX socket, firewall, etc.
But if you feel that it is needed I can add authentication for the wallet publishers.
I’d insist on that only when adding control of the wallet to the ZMQ interface.
115@@ -110,6 +116,12 @@ bool CZMQNotificationInterface::Initialize()
116 void CZMQNotificationInterface::Shutdown()
117 {
118 LogPrint(BCLog::ZMQ, "zmq: Shutdown notification interface\n");
119+
120+#ifdef ENABLE_WALLET
121+ if (pwalletMain)
{ and } for the if block, also this need adjustment for multiwallet.
1068@@ -1069,6 +1069,9 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
1069 boost::signals2::signal<void (CWallet *wallet, const uint256 &hashTx,
1070 ChangeType status)> NotifyTransactionChanged;
1071
1072+ boost::signals2::signal<void (const CTransactionRef &ptxn,
I think you should make this a static variable to make the zmq code simpler and less fragile and remove the dependency on ::vpwallets. Advantages of making this static:
ConnectToWalletSignals method and all the ifdefed code around it and simply connect to the signal in CZMQNotificationInterface::Initialize consistent with you how currently disconnect in CZMQNotificationInterface::Shutdown
utACK 2c393441b8dc55322625670a7aaf37e2df583e29, but I definitely think you should consider making CWallet::TransactionAddedToWallet static as described below to make init and shutdown less fragile.
Changes since previous review were moving the signal from validation interface to wallet, passing txids by const reference, wrapping markdown documentation, rebasing the test.
55@@ -56,6 +56,12 @@ CFeeRate CWallet::minTxFee = CFeeRate(DEFAULT_TRANSACTION_MINFEE);
56 * Override with -fallbackfee
57 */
58 CFeeRate CWallet::fallbackFee = CFeeRate(DEFAULT_FALLBACK_FEE);
59+/*
60+ * Signal when transactions are added to wallet
61+ */
62+boost::signals2::signal<void (const CTransactionRef &ptxn,
63+const uint256 &blockHash)> CWallet::TransactionAddedToWallet = boost::signals2::signal<void (const CTransactionRef &ptxn,
fixed and rebased last commit. Do you want me to rebase everything into one commit?
No preference, either way seems fine.
82+or mempool. If originated from mempool `-mempool` postfix
83+will be added to the topic, for block `-block` postfix will
84+be added. Because zeromq is using prefix matching for topics
85+you can subscribe to `rawwallettx` (or `hashwallettx`) to get
86+both notifications. If you only want one type of notification
87+subscribe to either `rawwallettx-mempool` or `rawwallettx-block`.
55@@ -56,6 +56,10 @@ CFeeRate CWallet::minTxFee = CFeeRate(DEFAULT_TRANSACTION_MINFEE);
56 * Override with -fallbackfee
57 */
58 CFeeRate CWallet::fallbackFee = CFeeRate(DEFAULT_FALLBACK_FEE);
59+/*
60+ * Signal when transactions are added to wallet
61+ */
62+boost::signals2::signal<void (const CTransactionRef &ptxn, const uint256 &blockHash)> CWallet::TransactionAddedToWallet;
blockHash should be nullptr when not in a block.
CWalletTx should be passed instead of CTransactionRef?
There is no way to get real time notification of transactions that only affect the wallet.
You have to do that manually by enabling zmqrawtx and filter out transactions.
I'm suggesting adding two new publisers, both for hash and raw wallet transactions.
Also topic will indicate if transaction came from mempool or block so developers can handle the transaction accordingly without a RPC round trip to bitcoind.
In order to avoid the registration for wallet signals.
Also enable dynamic addition of wallets without register them with ZMQ
169@@ -166,6 +170,23 @@ bool CZMQPublishHashTransactionNotifier::NotifyTransaction(const CTransaction &t
170 return SendMessage(MSG_HASHTX, data, 32);
171 }
172
173+bool CZMQPublishHashWalletTransactionNotifier::NotifyWalletTransaction(const CTransaction &transaction, const uint256 &hashBlock){
174+ uint256 hash = transaction.GetHash();
175+ LogPrint(BCLog::ZMQ, "zmq: Publish hashwallettx %s\n", hash.GetHex());
176+ char data[32];
177+ for (unsigned int i = 0; i < 32; i++)
I’m still unsure about this, if wtxs (protected by http auth) should be something we broadcast via ZMQ. Alternative would be HTTP based long polling (#7949).
Also, how does this handle multiwallet? Should we somehow integrate the wallet identifier in the notifications?
9@@ -10,6 +10,10 @@
10 #include "streams.h"
11 #include "util.h"
12
13+#ifdef ENABLE_WALLET
14+#include "../wallet/wallet.h"
There only seem to be a couple of other relative includes in the codebase. Other files use:
#include "wallet/wallet.h"
200+ const CTransaction& tx = *ptx;
201+
202+ for (std::list<CZMQAbstractNotifier*>::iterator i = notifiers.begin(); i!=notifiers.end(); )
203+ {
204+ CZMQAbstractNotifier *notifier = *i;
205+ if (notifier->NotifyWalletTransaction(tx, hashBlock))
204+ CZMQAbstractNotifier *notifier = *i;
205+ if (notifier->NotifyWalletTransaction(tx, hashBlock))
206+ {
207+ i++;
208+ }
209+ else
style nit:
} else {
177+ for (unsigned int i = 0; i < 32; i++)
178+ data[31 - i] = hash.begin()[i];
179+
180+ const char *command;
181+
182+ if (!hashBlock.IsNull())
19@@ -20,3 +20,7 @@ bool CZMQAbstractNotifier::NotifyTransaction(const CTransaction &/*transaction*/
20 {
21 return true;
22 }
23+
24+bool CZMQAbstractNotifier::NotifyWalletTransaction(const CTransaction &transaction, const uint256 &hashBlock){
22@@ -22,16 +23,17 @@ def __init__(self, socket, topic):
23 import zmq
24 self.socket.setsockopt(zmq.SUBSCRIBE, self.topic)
25
26- def receive(self):
27+ def receive(self, specific_topic = None):
= for named arguments.
36@@ -37,6 +37,10 @@ def assert_equal(thing1, thing2, *args):
37 if thing1 != thing2 or any(thing1 != arg for arg in args):
38 raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
39
40+def assert_not_equal(thing1, thing2):
Apologies for all the style nits. Please see https://github.com/bitcoin/bitcoin/blob/6157e8ce3937af3f46d3e7dd922d19d6dc272145/doc/developer-notes.md for the full style guide.
I agree with the other reviewers that this needs a bit more work for multiwallet. Can you either:
Both are fine. You could do (1) now and a future PR could add the name of the wallet to the notification.
Can you also update the zmq_test.py to verify that this notifications are received for all wallets when multiwallet is being used? I have a branch here that does that: https://github.com/jnewbery/bitcoin/tree/pr10554.1 . Feel free to take that and squash into your commit if you like it.
Also, I think this could use some extra documentation warning users about security implications:
contrib/devtools/lint-python.sh: 10: contrib/devtools/lint-python.sh: flake8: not found. It’s possible a rebase might fix this.
Alternative would be HTTP based long polling (#7949).
I tend to agree with @jonasschnelli alternative.
You can close.
I actually used the code in production as part of payments solution.