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

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
@somdoron
Copy link

somdoron commented Jun 8, 2017

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.

@ryanofsky
Copy link
Contributor

ryanofsky left a comment

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;

This comment has been minimized.

@ryanofsky

ryanofsky Jun 8, 2017

Contributor

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;

This comment has been minimized.

@ryanofsky

ryanofsky Jun 8, 2017

Contributor

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.

This comment has been minimized.

@ryanofsky

ryanofsky Jun 8, 2017

Contributor

Maybe wrap these lines for consistency with rest of the file.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Jun 8, 2017

Concept ACK - however some comments:

  • This should respect ENABLE_WALLET, for compiling without wallet support
  • It should respect runtime -wallet=0
  • Wallet events should not go through CValidationInterface, it is for validation events only. To do this correctly, zmq should subscribe to wallet events through CWallet directly.

@somdoron somdoron force-pushed the somdoron:zmq_wallet_tx branch Jun 8, 2017

@somdoron

This comment has been minimized.

Copy link
Author

somdoron commented Jun 8, 2017

@ryanofsky fixed the type, wrapped the docs and using 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.

@somdoron somdoron force-pushed the somdoron:zmq_wallet_tx branch 4 times, most recently from f4f1370 Jun 10, 2017

@somdoron

This comment has been minimized.

Copy link
Author

somdoron commented Jun 10, 2017

@ryanofsky @laanwj rebased the pull request on top of @jnewbery pull request #10555 and now all tests are passing

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Jun 12, 2017

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.

@somdoron

This comment has been minimized.

Copy link
Author

somdoron commented Jun 13, 2017

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.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Jun 13, 2017

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.

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Jun 13, 2017

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.

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Jun 18, 2017

Needs rebase.

@somdoron somdoron force-pushed the somdoron:zmq_wallet_tx branch Jun 20, 2017

@somdoron

This comment has been minimized.

Copy link
Author

somdoron commented Jun 20, 2017

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

This comment has been minimized.

@MarcoFalke

MarcoFalke Jun 20, 2017

Member

nit: missing braces { and } for the if block, also this need adjustment for multiwallet.

This comment has been minimized.

@somdoron

somdoron Jun 20, 2017

Author

fixed both

@somdoron somdoron force-pushed the somdoron:zmq_wallet_tx branch Jun 20, 2017

@somdoron

This comment has been minimized.

Copy link
Author

somdoron commented Jun 20, 2017

@MarcoFalke fixed and rebased

@ryanofsky
Copy link
Contributor

ryanofsky left a comment

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,

This comment has been minimized.

@ryanofsky

ryanofsky Jun 21, 2017

Contributor

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 in CZMQNotificationInterface::Initialize consistent with you how currently disconnect in CZMQNotificationInterface::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.

This comment has been minimized.

@somdoron

somdoron Jun 21, 2017

Author

thanks, will make the change

@somdoron somdoron force-pushed the somdoron:zmq_wallet_tx branch Jun 22, 2017

@ryanofsky
Copy link
Contributor

ryanofsky left a comment

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,

This comment has been minimized.

@ryanofsky

ryanofsky Jun 22, 2017

Contributor

You should be able to shorten this by getting rid of the assignment (it will just call the default constructor which is fine).

This comment has been minimized.

@somdoron

somdoron Jun 22, 2017

Author

fixed and rebased last commit. Do you want me to rebase everything into one commit?

This comment has been minimized.

@ryanofsky

ryanofsky Jun 22, 2017

Contributor

fixed and rebased last commit. Do you want me to rebase everything into one commit?

No preference, either way seems fine.

@somdoron somdoron force-pushed the somdoron:zmq_wallet_tx branch 2 times, most recently Jun 22, 2017

@ryanofsky
Copy link
Contributor

ryanofsky left a comment

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

This comment has been minimized.

@luke-jr

luke-jr Sep 2, 2017

Member

No documentation on how to determine which wallet this transaction involves (is it even possible?)

This comment has been minimized.

@somdoron

somdoron Sep 3, 2017

Author

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;

This comment has been minimized.

@luke-jr

luke-jr Sep 2, 2017

Member

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;

This comment has been minimized.

@luke-jr

luke-jr Sep 2, 2017

Member

Probably CWalletTx should be passed instead of CTransactionRef?

This comment has been minimized.

@somdoron

somdoron Sep 4, 2017

Author

actually if CWalletTX is used the blockhash is not needed anymore. I will take a look.

@ryanofsky

This comment has been minimized.

Copy link
Contributor

ryanofsky commented Oct 12, 2017

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

@somdoron

This comment has been minimized.

Copy link
Author

somdoron commented Oct 17, 2017

@ryanofsky yes, will rebase today and send a PR

@somdoron somdoron force-pushed the somdoron:zmq_wallet_tx branch to 6b4224a Oct 19, 2017

somdoron added some commits Jun 8, 2017

ZMQ: add publishers of wallet tx
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.
ZMQ: Making CWallet::TransactionAddedToWallet static
In order to avoid the registration for wallet signals.
Also enable dynamic addition of wallets without register them with ZMQ

@somdoron somdoron force-pushed the somdoron:zmq_wallet_tx branch from 6b4224a to ed4fd26 Oct 19, 2017

@somdoron

This comment has been minimized.

Copy link
Author

somdoron commented Oct 22, 2017

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

This comment has been minimized.

@jonasschnelli

jonasschnelli Oct 23, 2017

Member

memcpy?

This comment has been minimized.

@jonasschnelli

jonasschnelli Oct 23, 2017

Member

Nevermind. I see we do reverse the byte order here...

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Oct 23, 2017

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?

@jnewbery
Copy link
Member

jnewbery left a comment

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:

  1. add documentation saying that multiwallet is supported but that notifications will not indicate which wallet the notification is for; or
  2. 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"

This comment has been minimized.

@jnewbery

jnewbery Oct 23, 2017

Member

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

This comment has been minimized.

@jnewbery

jnewbery Oct 23, 2017

Member

style nit: braces on same line please.

{
i++;
}
else

This comment has been minimized.

@jnewbery

jnewbery Oct 23, 2017

Member

style nit:

} else {


const char *command;

if (!hashBlock.IsNull())

This comment has been minimized.

@jnewbery

jnewbery Oct 23, 2017

Member

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

This comment has been minimized.

@jnewbery

jnewbery Oct 23, 2017

Member

nit: braces please

@@ -20,3 +20,7 @@ bool CZMQAbstractNotifier::NotifyTransaction(const CTransaction &/*transaction*/
{
return true;
}

bool CZMQAbstractNotifier::NotifyWalletTransaction(const CTransaction &transaction, const uint256 &hashBlock){

This comment has been minimized.

@jnewbery

jnewbery Oct 23, 2017

Member

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

This comment has been minimized.

@jnewbery

jnewbery Oct 23, 2017

Member

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

This comment has been minimized.

@jnewbery

jnewbery Oct 23, 2017

Member

You've added this but not used it. If it's not being used, can you remove it from this PR?

@ryanofsky
Copy link
Contributor

ryanofsky left a comment

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.

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Nov 6, 2017

@ryanofsky

This comment has been minimized.

Copy link
Contributor

ryanofsky commented Jan 9, 2018

For some reason travis is failing with contrib/devtools/lint-python.sh: 10: contrib/devtools/lint-python.sh: flake8: not found. It's possible a rebase might fix this.

@promag

This comment has been minimized.

Copy link
Member

promag commented Jan 10, 2018

Alternative would be HTTP based long polling (#7949).

I tend to agree with @jonasschnelli alternative.

@breitsmiley

This comment has been minimized.

Copy link

breitsmiley commented May 4, 2018

What about this PR? Is it going to be added?

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented May 4, 2018

No update from PR author @somdoron since October 2017. Are you still working on this @somdoron ?

If not, I suggest we close this as 'Up for grabs'

@somdoron

This comment has been minimized.

Copy link
Author

somdoron commented May 5, 2018

You can close.

I actually used the code in production as part of payments solution.

@jnewbery jnewbery closed this May 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment