-
Notifications
You must be signed in to change notification settings - Fork 36.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ZMQ: add publishers for wallet transactions. #10554
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 76d3e3bb9e2d5fd5e0c570489cc377ce95cc370f. Change looks good. Left some minor comments.
src/validationinterface.h
Outdated
@@ -52,6 +53,8 @@ struct CMainSignals { | |||
boost::signals2::signal<void (const CBlockIndex *, const CBlockIndex *, bool fInitialDownload)> UpdatedBlockTip; | |||
/** Notifies listeners of a transaction having been added to mempool. */ | |||
boost::signals2::signal<void (const CTransactionRef &)> TransactionAddedToMempool; | |||
/** Notifies listeners of a transaction having been added to the wallet. */ | |||
boost::signals2::signal<void (const CTransactionRef &, uint256 hashBlock)> TransactionAddedWallet; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably s/TransactionAddedWallet/TransactionAddedToWallet/ for consistency
src/validationinterface.h
Outdated
@@ -52,6 +53,8 @@ struct CMainSignals { | |||
boost::signals2::signal<void (const CBlockIndex *, const CBlockIndex *, bool fInitialDownload)> UpdatedBlockTip; | |||
/** Notifies listeners of a transaction having been added to mempool. */ | |||
boost::signals2::signal<void (const CTransactionRef &)> TransactionAddedToMempool; | |||
/** Notifies listeners of a transaction having been added to the wallet. */ | |||
boost::signals2::signal<void (const CTransactionRef &, uint256 hashBlock)> TransactionAddedWallet; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Throughout the PR, should use const uint256&
instead of uint256
arg type to avoid unnecessary copies.
doc/zmq.md
Outdated
@@ -75,6 +77,11 @@ notification `-zmqpubhashtx` the topic is `hashtx` (no null | |||
terminator) and the body is the hexadecimal transaction hash (32 | |||
bytes). | |||
|
|||
For wallet transaction notifications (both hash and tx), the topic also indicate if the transaction came from a block or mempool. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe wrap these lines for consistency with rest of the file.
Concept ACK - however some comments:
|
@ryanofsky fixed the type, wrapped the docs and using |
f4f1370
to
09c6899
Compare
@ryanofsky @laanwj rebased the pull request on top of @jnewbery pull request #10555 and now all tests are passing |
Does ZMQ have any authentication? I believe originally nothing wallet-related was exposed through it, as there may at least by privacy issues from publishing this. |
zeromq does have authentication, but it is not being used within bitcoind. But if you feel that it is needed I can add authentication for the wallet publishers. |
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.
I'd insist on that only when adding control of the wallet to the ZMQ interface. |
I have no strong opinion on the need for authentication; I just wanted to bring up that I believed that was the reason for not having wallet specific notifications in ZMQ before. |
Needs rebase. |
@MarcoFalke rebased |
src/zmq/zmqnotificationinterface.cpp
Outdated
@@ -110,6 +116,12 @@ bool CZMQNotificationInterface::Initialize() | |||
void CZMQNotificationInterface::Shutdown() | |||
{ | |||
LogPrint(BCLog::ZMQ, "zmq: Shutdown notification interface\n"); | |||
|
|||
#ifdef ENABLE_WALLET | |||
if (pwalletMain) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: missing braces {
and }
for the if block, also this need adjustment for multiwallet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed both
@MarcoFalke fixed and rebased |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/wallet/wallet.h
Outdated
@@ -1069,6 +1069,9 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface | |||
boost::signals2::signal<void (CWallet *wallet, const uint256 &hashTx, | |||
ChangeType status)> NotifyTransactionChanged; | |||
|
|||
boost::signals2::signal<void (const CTransactionRef &ptxn, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
- It will let you get rid of the
ConnectToWalletSignals
method and all the ifdefed code around it and simply connect to the signal inCZMQNotificationInterface::Initialize
consistent with you how currently disconnect inCZMQNotificationInterface::Shutdown
- If will make it possible in the future for new wallets to be added to ::vpwallets at runtime without having to modify zmq code and make it register for new notifications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, will make the change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 123c3e75e96e56a78387353d10dd05b8d73e51c1. Change since last review was a new commit making the wallet signal static.
src/wallet/wallet.cpp
Outdated
* Signal when transactions are added to wallet | ||
*/ | ||
boost::signals2::signal<void (const CTransactionRef &ptxn, | ||
const uint256 &blockHash)> CWallet::TransactionAddedToWallet = boost::signals2::signal<void (const CTransactionRef &ptxn, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to shorten this by getting rid of the assignment (it will just call the default constructor which is fine).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed and rebased last commit. Do you want me to rebase everything into one commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed and rebased last commit. Do you want me to rebase everything into one commit?
No preference, either way seems fine.
da7ff89
to
d358230
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK d358230d10b00ef7e1a284d1b855fa1604bf1d28. Only change since last review was removing unneeded init assignment.
be added. Because zeromq is using prefix matching for topics | ||
you can subscribe to `rawwallettx` (or `hashwallettx`) to get | ||
both notifications. If you only want one type of notification | ||
subscribe to either `rawwallettx-mempool` or `rawwallettx-block`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No documentation on how to determine which wallet this transaction involves (is it even possible?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was actually submitted before the multi-wallet was merged. Wallet name (or identifier) can be part of the topic.
/* | ||
* Signal when transactions are added to wallet | ||
*/ | ||
boost::signals2::signal<void (const CTransactionRef &ptxn, const uint256 &blockHash)> CWallet::TransactionAddedToWallet; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO blockHash
should be nullptr
when not in a block.
/* | ||
* Signal when transactions are added to wallet | ||
*/ | ||
boost::signals2::signal<void (const CTransactionRef &ptxn, const uint256 &blockHash)> CWallet::TransactionAddedToWallet; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably CWalletTx
should be passed instead of CTransactionRef
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually if CWalletTX is used the blockhash is not needed anymore. I will take a look.
@somdoron are you still working on this? IIUC, luke's improvements could be implemented later without breaking backwards compatibility, if they are what's holding this up. |
@ryanofsky yes, will rebase today and send a PR |
d358230
to
6b4224a
Compare
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
6b4224a
to
ed4fd26
Compare
@ryanofsky rebased and all tests passed. Doesn't yet include @luke-jr comments. |
uint256 hash = transaction.GetHash(); | ||
LogPrint(BCLog::ZMQ, "zmq: Publish hashwallettx %s\n", hash.GetHex()); | ||
char data[32]; | ||
for (unsigned int i = 0; i < 32; i++) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
memcpy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind. I see we do reverse the byte order here...
I'm still unsure about this, if wtxs (protected by http auth) should be something we broadcast via ZMQ. Also, how does this handle multiwallet? Should we somehow integrate the wallet identifier in the notifications? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
- add documentation saying that multiwallet is supported but that notifications will not indicate which wallet the notification is for; or
- add the wallet name to the notification topic
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:
- zmq is unauthenticated
- notifications are received for all wallets and can't be enabled/disabled on a per-wallet basis.
@@ -10,6 +10,10 @@ | |||
#include "streams.h" | |||
#include "util.h" | |||
|
|||
#ifdef ENABLE_WALLET | |||
#include "../wallet/wallet.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There only seem to be a couple of other relative includes in the codebase. Other files use:
#include "wallet/wallet.h"
for (std::list<CZMQAbstractNotifier*>::iterator i = notifiers.begin(); i!=notifiers.end(); ) | ||
{ | ||
CZMQAbstractNotifier *notifier = *i; | ||
if (notifier->NotifyWalletTransaction(tx, hashBlock)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style nit: braces on same line please.
{ | ||
i++; | ||
} | ||
else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style nit:
} else {
|
||
const char *command; | ||
|
||
if (!hashBlock.IsNull()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: braces please.
uint256 hash = transaction.GetHash(); | ||
LogPrint(BCLog::ZMQ, "zmq: Publish hashwallettx %s\n", hash.GetHex()); | ||
char data[32]; | ||
for (unsigned int i = 0; i < 32; i++) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: braces please
@@ -20,3 +20,7 @@ bool CZMQAbstractNotifier::NotifyTransaction(const CTransaction &/*transaction*/ | |||
{ | |||
return true; | |||
} | |||
|
|||
bool CZMQAbstractNotifier::NotifyWalletTransaction(const CTransaction &transaction, const uint256 &hashBlock){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: function brace on newline please.
@@ -22,16 +23,17 @@ def __init__(self, socket, topic): | |||
import zmq | |||
self.socket.setsockopt(zmq.SUBSCRIBE, self.topic) | |||
|
|||
def receive(self): | |||
def receive(self, specific_topic = None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: no spaces around the =
for named arguments.
@@ -37,6 +37,10 @@ def assert_equal(thing1, thing2, *args): | |||
if thing1 != thing2 or any(thing1 != arg for arg in args): | |||
raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args)) | |||
|
|||
def assert_not_equal(thing1, thing2): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've added this but not used it. If it's not being used, can you remove it from this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK ed4fd26. Only change since last review is rebase. Agree with John it'd be good to document the multiwallet limitation if you don't want to add wallet names to the notifications right now. Also I don't see any reason not to squash the two commits. Would make understanding the PR a little simpler.
Github-Pull: bitcoin#10554 Rebased-From: d6448d4
For some reason travis is failing with |
I tend to agree with @jonasschnelli alternative. |
What about this PR? Is it going to be added? |
You can close. I actually used the code in production as part of payments solution. |
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.