wallet: Add new format string placeholders for walletnotify

maayank commented at 12:29 pm on February 10, 2021: contributor

This patch includes two new format placeholders for walletnotify: %b - the hash of the block containting the transaction (zeroed if a mempool transaction) %h - the height of the block containing the transaction (zero if a mempool transaction)

I’ve included test suite changes to check and validate the above functional requirements as well as doc/help description changes.

Motivation The walletnotify option is used to be notified of new transactions relevant to the wallet of the node. A common usage pattern is to perform afterwards additional RPC calls to determine:

If this is a mempool transaction or not (i.e. are there any confirmations?)
What block was it included in?
Did this transaction was seen before and is now seen again because of a fork?

All of these questions can be answered with the current features, but the resulting RPC calls may be expensive in a heavily used node. As this information is readily available when calling the walletnotify callback, it makes sense to save expensive round trips by optionally sending this information at that point in time. I can definitely say we would like to use it in Fireblocks, my employer.

Please let me know of any questions and suggestions.

maayank renamed this:
~~Add new format string placeholders for walletnotify~~
wallet: Add new format string placeholders for walletnotify
on Feb 10, 2021

promag commented at 12:48 pm on February 10, 2021: member

This seems fine, but keep in mind that -walletnotify scripts are called async, so you don’t really know if you are processing in the right order. Not so sure if it’s a good approach to avoid RPC calls.

maayank force-pushed on Feb 10, 2021

maayank commented at 12:58 pm on February 10, 2021: contributor

Thank you for the comment @promag . It’s an important comment especially for future readers intending to use it for fork detection.

(not related) I’ve changed the code to use ToString instead of std::to_string to avoid locale linter errors.

What do you say on changing the start of the description “Execute command when a wallet transaction changes” to “Asynchronously execute command when a wallet transaction changes”? It’s true also before this commit. Would help to highlight potential tradeoffs for future users.

fanquake added the label Wallet on Feb 10, 2021

laanwj commented at 1:35 pm on February 10, 2021: member

Concept ACK, I don’t think adding this information to notifications can hurt and it’s only very little more code.

maayank force-pushed on Feb 10, 2021

maayank commented at 8:14 am on February 11, 2021: contributor

I’ve noticed in the CI checks some linter errors as well as that on some systems the notification test case failed due to the concatenation of wallet, txid, block height and block hash being too long of a filename. I’ve changed the test case so it now checks not only the filename (for wallet and txid) but contents (for block height and block hash). Now the request passes all the CI tests and is ready to merge.

meshcollider commented at 10:03 pm on February 16, 2021: contributor

Concept ACK

laanwj approved

laanwj commented at 6:34 pm on February 18, 2021: member

Code review ACK 3d0ee35031f7f81b3f333c3ae7175af8591a9099

in src/wallet/init.cpp:70 in 3d0ee35031 outdated

66@@ -67,7 +67,7 @@ void WalletInit::AddWalletOptions(ArgsManager& argsman) const
67     argsman.AddArg("-walletbroadcast",  strprintf("Make the wallet broadcast transactions (default: %u)", DEFAULT_WALLETBROADCAST), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
68     argsman.AddArg("-walletdir=<dir>", "Specify directory to hold wallets (default: <datadir>/wallets if it exists, otherwise <datadir>)", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::WALLET);
69 #if HAVE_SYSTEM
70-    argsman.AddArg("-walletnotify=<cmd>", "Execute command when a wallet transaction changes. %s in cmd is replaced by TxID and %w is replaced by wallet name. %w is not currently implemented on windows. On systems where %w is supported, it should NOT be quoted because this would break shell escaping used to invoke the command.", ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
71+    argsman.AddArg("-walletnotify=<cmd>", "Execute command when a wallet transaction changes. %s in cmd is replaced by TxID, %w is replaced by wallet name, %b is replaced by the hash of the block including the transaction (zeroed if the transaction is not included) and %h is replaced by the block height (0 if not included). %w is not currently implemented on windows. On systems where %w is supported, it should NOT be quoted because this would break shell escaping used to invoke the command.", ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);

luke-jr commented at 1:52 am on February 24, 2021:

Height 0 is the genesis block. Suggest -1 for unconfirmed txs instead.

luke-jr commented at 1:53 am on February 24, 2021:

Would prefer the fixed string “unconfirmed” for “%b” when not a block hash.

maayank commented at 2:02 pm on March 8, 2021:

Thank you for you comments Luke. Made the requested changes

luke-jr changes_requested

maayank force-pushed on Mar 8, 2021

maayank commented at 3:51 pm on March 11, 2021: contributor

Thank you for your comments @luke-jr , made the changes you suggested. FYI @laanwj @promag

laanwj commented at 7:29 am on March 15, 2021: member

Agree with @luke-jr ’s suggestions, ACK after squashing commits.

maayank force-pushed on Mar 15, 2021

Add new format string placeholders for walletnotify to include relevant block information for transactions 06e1fb0b17

maayank force-pushed on Mar 15, 2021

laanwj commented at 6:51 pm on March 15, 2021: member

ACK 06e1fb0b170a69996a7ce1ef5203785a7bc6b278

laanwj merged this on Mar 15, 2021

laanwj closed this on Mar 15, 2021

in src/wallet/wallet.cpp:948 in 06e1fb0b17

943@@ -944,6 +944,14 @@ CWalletTx* CWallet::AddToWallet(CTransactionRef tx, const CWalletTx::Confirmatio
944     if (!strCmd.empty())
945     {
946         boost::replace_all(strCmd, "%s", hash.GetHex());
947+        if (confirm.status == CWalletTx::Status::CONFIRMED)
948+        {

jonatack commented at 7:10 pm on March 15, 2021:

0-        if (confirm.status == CWalletTx::Status::CONFIRMED)
1-        {
2+        if (confirm.status == CWalletTx::Status::CONFIRMED) {

laanwj commented at 7:28 pm on March 15, 2021:

Oops, noticed this too late, sorry

jonatack commented at 7:10 pm on March 15, 2021: member

Concept ACK, looks like a helpful nice addition.

sidhujag referenced this in commit edb5298277 on Mar 16, 2021

luke-jr referenced this in commit 8f711d565d on Jun 27, 2021

DrahtBot locked this on Aug 16, 2022

wallet: Add new format string placeholders for walletnotify #21141