Add -walletnotify to call an external script on wallet transactions #1974

Merged
merged 1 commit into from Feb 22, 2013

Projects

None yet

8 participants

kjj2 commented Nov 3, 2012

Exactly like -blocknotify, except that it gives the TxID of transactions that hit the wallet.

Note that this is NOT a payment notification. It will trigger on outgoing transactions too, and can trigger multiple times on the same transaction.

Contributor
freewil commented Nov 3, 2012

This is repetitive and doesn't add any new functionality. You can check for new incoming transactions after -blocknotify and obviously you will already know about outgoing transactions.

kjj2 commented Nov 3, 2012

No, this triggers when a transaction comes in or goes out in any way, not just when blocks hit. And now that we have the raw transaction API and offline signing and transmission is relatively easy, it is no longer obvious when transactions go out.

Owner
sipa commented Nov 5, 2012

This is probably useful for a lot of people who would otherwise depend on polling (or using centralized services to know when to poll), but it hopefully doesn't inspire people to do 0-conf accepts.

ACK.

@gavinandresen gavinandresen commented on the diff Nov 5, 2012
src/wallet.cpp
@@ -470,6 +471,16 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn)
// Notify UI of new or updated transaction
NotifyTransactionChanged(this, hash, fInsertedNew ? CT_NEW : CT_UPDATED);
+
+ // notify an external script when a wallet transaction comes in or is updated
+ std::string strCmd = GetArg("-walletnotify", "");
+
+ if ( !strCmd.empty())
+ {
+ boost::replace_all(strCmd, "%s", wtxIn.GetHash().GetHex());
+ boost::thread t(runCommand, strCmd); // thread runs free
gavinandresen
gavinandresen Nov 5, 2012 Contributor

This opens up a potential DoS, I can send you a gazillion tiny transactions to try to get you to spawn gazillions of threads.

I'd suggest not notifying transactions that are less than MIN_TX_FEE/-mintxfee (see code after // Fee-per-kilobyte amount considered the same as "free" in main.cpp)

gmaxwell
gmaxwell Nov 5, 2012 Member

Uh. I'd worry that callers wouldn't be aware of that subtle detail and would have applications which missed small transactions.

If the API worked in a way that it could simply be rate limited to not notify more than X times per second then that would be a preferable option, but the 1:1 behavior doesn't seem to work for that.

kjj2
kjj2 Nov 5, 2012

Since this triggers when a transaction updates the wallet, a potential DOS attacker would need to know both your IP (because no one will relay), and one of your addresses (pubkeys) to do their dust attack. In my opinion, there are easier ways for them to DOS you at that point.

Also, the fee calculations are not trivial. Adding that would require either duplicating a lot of code, or moving those calculations into their own function.

Rate limiting would be a bit easier, but the best way to do it would be open to question. Make a queue to toss them into, and then have a thread poll and dump that queue every 30 seconds or so?

I made this after two people popped into IRC, within an hour or two of each other, both having problems figuring out the "right" way to spot new transactions coming in to their wallets. This solves their problems quickly and simply. I'd rather see this get out quickly for those people, along with a note or something about the potential danger (and how to mitigate it for critical nodes) than make them keep waiting while we figure out how to make it safer under circumstances that may or may not ever happen.

Then again, no one comes bitching to me when things don't work, so I defer to the judgment of the more experienced developers.

gavinandresen
gavinandresen Nov 6, 2012 Contributor

I'd be curious to see what happens if you run this experiment on testnet:

  • Create 100 transactions, each with 1 well-aged input and (oh, I dunno) 100 output that all go to the same wallet (100 different addresses, though).
  • Now send them all at the same time.

If the inputs are old enough and large enough, you could perform the above attack paying minimal transaction fees.

I predict Bad Things will happen as the bitcoin process forks 10,000 times. -blocknotify doesn't have to worry about these issues because we've got one block on average every 10 minutes.

-walletnotify=/path/to/named_pipe would be safer. Actually, -blocknotify=/path/to/named_pipe might be useful, too...

kjj2
kjj2 Nov 6, 2012

I only have one live wallet with RPC access at the moment, so I wasn't able to test your setup exactly. But I did create 10 new keys in my wallet, and I made a sendtomany raw transaction that sent something to each of them, then signed it offline and relayed it through a different node.

http://blockexplorer.com/tx/7650e6a4ef61485a3f6c44fbeaa1063c6ea0bbe4bf040fb91c6fd71888a81677

That transaction triggered walletnotify exactly twice on my box, once when it entered the memory pool, and once when the block containing it arrived. It doesn't trigger for each input or each output of a transaction, just for each transaction-event.

I like the named pipe thing too. I'll look into it.

Member
jgarzik commented Nov 16, 2012

ACK... with maybe some doc describing some usage issues like what Gavin raised.

Contributor

I've had a need for this kind of alert in applications before and as a result ended up writing a custom bitcoin client with its own internal filtering. I would love to be able to attach a listener to the mempool (perhaps SyncWithWallets could be extended to this end) which can perform filtering beyond just CWallet::IsMine(). Perhaps we can also filter out transactions below a certain size threshold, for instance.

The need for being able to monitor transactions actually motivated #2121, which combined with -walletnotify would allow realtime tracking of payments without having to store any private keys at all.

Contributor
rebroad commented Dec 24, 2012

How about instead an option to output transaction information to a file instead? That way, a separate program can deal with the problem of flooding, etc, and triggering commands from output as it appears in the file.

Contributor

Couple of ACKS... this is definitely better than what we have now. I'm going to pull.

@gavinandresen gavinandresen merged commit 49e332f into bitcoin:master Feb 22, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment