Merge #21141: wallet: Add new format string placeholders for walletnotify

06e1fb0 Add new format string placeholders for walletnotify to include relevant block information for transactions (Maayan Keshet)

Pull request description:

  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:
  1. If this is a mempool transaction or not (i.e. are there any confirmations?)
  2. What block was it included in?
  3. 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.

ACKs for top commit:
  laanwj:
    ACK 06e1fb0

Tree-SHA512: d2744e2a7a883f9c3a9fd32237110e048c4b6b11fea8221c33d10b74157f65bbc4351211f441e8c1a4af5d5d38e2ba6b1943a7673dc18860c0553d7b41e00775

Author: laanwj

src/wallet/init.cpp

@@ -70,7 +70,7 @@ void WalletInit::AddWalletOptions(ArgsManager& argsman) const
     argsman.AddArg("-walletbroadcast",  strprintf("Make the wallet broadcast transactions (default: %u)", DEFAULT_WALLETBROADCAST), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
     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);
 #if HAVE_SYSTEM
-    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);
+    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 (set to 'unconfirmed' if the transaction is not included) and %h is replaced by the block height (-1 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);
 #endif
     argsman.AddArg("-walletrbf", strprintf("Send transactions with full-RBF opt-in enabled (RPC only, default: %u)", DEFAULT_WALLET_RBF), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
 

src/wallet/wallet.cpp

@@ -944,6 +944,14 @@ CWalletTx* CWallet::AddToWallet(CTransactionRef tx, const CWalletTx::Confirmatio
     if (!strCmd.empty())
     {
         boost::replace_all(strCmd, "%s", hash.GetHex());
+        if (confirm.status == CWalletTx::Status::CONFIRMED)
+        {
+            boost::replace_all(strCmd, "%b", confirm.hashBlock.GetHex());
+            boost::replace_all(strCmd, "%h", ToString(confirm.block_height));
+        } else {
+            boost::replace_all(strCmd, "%b", "unconfirmed");
+            boost::replace_all(strCmd, "%h", "-1");
+        }
 #ifndef WIN32
         // Substituting the wallet name isn't currently supported on windows
         // because windows shell escaping has not been implemented yet:

test/functional/feature_notifications.py

@@ -17,7 +17,7 @@
 FILE_CHAR_START = 32 if os.name == 'nt' else 1
 FILE_CHAR_END = 128
 FILE_CHARS_DISALLOWED = '/\\?%*:|"<>' if os.name == 'nt' else '/'
-
+UNCONFIRMED_HASH_STRING = 'unconfirmed'
 
 def notify_outputname(walletname, txid):
     return txid if os.name == 'nt' else '{}_{}'.format(walletname, txid)
@@ -43,7 +43,7 @@ def setup_network(self):
             "-blocknotify=echo > {}".format(os.path.join(self.blocknotify_dir, '%s')),
         ], [
             "-rescan",
-            "-walletnotify=echo > {}".format(os.path.join(self.walletnotify_dir, notify_outputname('%w', '%s'))),
+            "-walletnotify=echo %h_%b > {}".format(os.path.join(self.walletnotify_dir, notify_outputname('%w', '%s'))),
         ]]
         self.wallet_names = [self.default_wallet_name, self.wallet]
         super().setup_network()
@@ -90,11 +90,9 @@ def run_test(self):
             self.wait_until(lambda: len(os.listdir(self.walletnotify_dir)) == block_count, timeout=10)
 
             # directory content should equal the generated transaction hashes
-            txids_rpc = list(map(lambda t: notify_outputname(self.wallet, t['txid']), self.nodes[1].listtransactions("*", block_count)))
-            assert_equal(sorted(txids_rpc), sorted(os.listdir(self.walletnotify_dir)))
+            tx_details = list(map(lambda t: (t['txid'], t['blockheight'], t['blockhash']), self.nodes[1].listtransactions("*", block_count)))
             self.stop_node(1)
-            for tx_file in os.listdir(self.walletnotify_dir):
-                os.remove(os.path.join(self.walletnotify_dir, tx_file))
+            self.expect_wallet_notify(tx_details)
 
             self.log.info("test -walletnotify after rescan")
             # restart node to rescan to force wallet notifications
@@ -104,10 +102,8 @@ def run_test(self):
             self.wait_until(lambda: len(os.listdir(self.walletnotify_dir)) == block_count, timeout=10)
 
             # directory content should equal the generated transaction hashes
-            txids_rpc = list(map(lambda t: notify_outputname(self.wallet, t['txid']), self.nodes[1].listtransactions("*", block_count)))
-            assert_equal(sorted(txids_rpc), sorted(os.listdir(self.walletnotify_dir)))
-            for tx_file in os.listdir(self.walletnotify_dir):
-                os.remove(os.path.join(self.walletnotify_dir, tx_file))
+            tx_details = list(map(lambda t: (t['txid'], t['blockheight'], t['blockhash']), self.nodes[1].listtransactions("*", block_count)))
+            self.expect_wallet_notify(tx_details)
 
             # Conflicting transactions tests.
             # Generate spends from node 0, and check notifications
@@ -122,7 +118,7 @@ def run_test(self):
             tx1 = self.nodes[0].sendtoaddress(address=ADDRESS_BCRT1_UNSPENDABLE, amount=1, replaceable=True)
             assert_equal(tx1 in self.nodes[0].getrawmempool(), True)
             self.sync_mempools()
-            self.expect_wallet_notify([tx1])
+            self.expect_wallet_notify([(tx1, -1, UNCONFIRMED_HASH_STRING)])
 
             # Generate bump transaction, sync mempools, and check for bump1
             # notification. In the future, per
@@ -131,39 +127,57 @@ def run_test(self):
             bump1 = self.nodes[0].bumpfee(tx1)["txid"]
             assert_equal(bump1 in self.nodes[0].getrawmempool(), True)
             self.sync_mempools()
-            self.expect_wallet_notify([bump1])
+            self.expect_wallet_notify([(bump1, -1, UNCONFIRMED_HASH_STRING)])
 
             # Add bump1 transaction to new block, checking for a notification
             # and the correct number of confirmations.
-            self.nodes[0].generatetoaddress(1, ADDRESS_BCRT1_UNSPENDABLE)
+            blockhash1 = self.nodes[0].generatetoaddress(1, ADDRESS_BCRT1_UNSPENDABLE)[0]
+            blockheight1 = self.nodes[0].getblockcount()
             self.sync_blocks()
-            self.expect_wallet_notify([bump1])
+            self.expect_wallet_notify([(bump1, blockheight1, blockhash1)])
             assert_equal(self.nodes[1].gettransaction(bump1)["confirmations"], 1)
 
             # Generate a second transaction to be bumped.
             tx2 = self.nodes[0].sendtoaddress(address=ADDRESS_BCRT1_UNSPENDABLE, amount=1, replaceable=True)
             assert_equal(tx2 in self.nodes[0].getrawmempool(), True)
             self.sync_mempools()
-            self.expect_wallet_notify([tx2])
+            self.expect_wallet_notify([(tx2, -1, UNCONFIRMED_HASH_STRING)])
 
             # Bump tx2 as bump2 and generate a block on node 0 while
             # disconnected, then reconnect and check for notifications on node 1
             # about newly confirmed bump2 and newly conflicted tx2.
             self.disconnect_nodes(0, 1)
             bump2 = self.nodes[0].bumpfee(tx2)["txid"]
-            self.nodes[0].generatetoaddress(1, ADDRESS_BCRT1_UNSPENDABLE)
+            blockhash2 = self.nodes[0].generatetoaddress(1, ADDRESS_BCRT1_UNSPENDABLE)[0]
+            blockheight2 = self.nodes[0].getblockcount()
             assert_equal(self.nodes[0].gettransaction(bump2)["confirmations"], 1)
             assert_equal(tx2 in self.nodes[1].getrawmempool(), True)
             self.connect_nodes(0, 1)
             self.sync_blocks()
-            self.expect_wallet_notify([bump2, tx2])
+            self.expect_wallet_notify([(bump2, blockheight2, blockhash2), (tx2, -1, UNCONFIRMED_HASH_STRING)])
             assert_equal(self.nodes[1].gettransaction(bump2)["confirmations"], 1)
 
         # TODO: add test for `-alertnotify` large fork notifications
 
-    def expect_wallet_notify(self, tx_ids):
-        self.wait_until(lambda: len(os.listdir(self.walletnotify_dir)) >= len(tx_ids), timeout=10)
-        assert_equal(sorted(notify_outputname(self.wallet, tx_id) for tx_id in tx_ids), sorted(os.listdir(self.walletnotify_dir)))
+    def expect_wallet_notify(self, tx_details):
+        self.wait_until(lambda: len(os.listdir(self.walletnotify_dir)) >= len(tx_details), timeout=10)
+        # Should have no more and no less files than expected
+        assert_equal(sorted(notify_outputname(self.wallet, tx_id) for tx_id, _, _ in tx_details), sorted(os.listdir(self.walletnotify_dir)))
+        # Should now verify contents of each file
+        for tx_id, blockheight, blockhash in tx_details:
+            fname = os.path.join(self.walletnotify_dir, notify_outputname(self.wallet, tx_id))
+            with open(fname, 'rt', encoding='utf-8') as f:
+                text = f.read()
+                # Universal newline ensures '\n' on 'nt'
+                assert_equal(text[-1], '\n')
+                text = text[:-1]
+                if os.name == 'nt':
+                    # On Windows, echo as above will append a whitespace
+                    assert_equal(text[-1], ' ')
+                    text = text[:-1]
+                expected = str(blockheight) + '_' + blockhash
+                assert_equal(text, expected)
+
         for tx_file in os.listdir(self.walletnotify_dir):
             os.remove(os.path.join(self.walletnotify_dir, tx_file))