Repository: bitcoin/bitcoin (language: cpp)

Issue:
Return block hash with wallet calls
Some wallet calls (like getbalances) need to lock the chainstate to make sure they are caught up on the latest block (which may have been reported to the users in getblockchaininfo or similar).

Those calls could benefit if the block hash they were valid on was included in the response. For example, immature balance can only change when the chainstate changes. (Assuming no keys are imported)

It seems those calls can include the block hash they are caught up with for free in the response. They could simply include `m_last_block_processed` in the response.

I haven't checked back with other developers to see if they like this approach, but to start simple only a single call could be updated for now. E.g. `getbalances`.

#### Useful skills:

Basic understanding of threads, the Bitcoin Core wallet and the Bitcoin Core RPC interface.

#### Want to work on this issue?

The purpose of the `good first issue` label is to highlight which issues are suitable for a new contributor without a deep understanding of the codebase.

You do not need to request permission to start working on this. You are encouraged to comment on the issue if you are planning to work on it. This will help other contributors monitor which issues are actively being addressed and is also an effective way to request assistance if and when you need it.

For guidance on contributing, please read [CONTRIBUTING.md](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md) before opening your pull request.



Discussion / hints:
Will look into it.
> Those calls could benefit if the block hash they were valid on was included in the response. For example, immature balance can only change when the chainstate changes.

@MarcoFalke but what's the idea? Next time calling `getbalances` cliend would also send last received `m_last_block_processed` so that the server would reply "no change"? Or just because this is valuable information?

In this particular case, an import+rescan makes things worse right?
> Or just because this is valuable information?

Yes

> an import+rescan makes things worse right?

What things? `m_last_block_processed` is a wallet member and including it in the reply is free.
> What things? `m_last_block_processed` is a wallet member and including it in the reply is free.

It should be fine to add that to RPC that return objects. I was asking other thing but you already said in OP:

> immature balance can only change when the chainstate changes. (Assuming no keys are imported)

Fix patch:
diff --git a/doc/release-notes-26094.md b/doc/release-notes-26094.md
new file mode 100644
index 0000000000000..ba73f2707e72f
--- /dev/null
+++ b/doc/release-notes-26094.md
@@ -0,0 +1,6 @@
+- The `getbalances` RPC now returns a `lastprocessedblock` JSON object which contains the wallet's last processed block
+  hash and height at the time the balances were calculated. This result shouldn't be cached because importing new keys could invalidate it.
+- The `gettransaction` RPC now returns a `lastprocessedblock` JSON object which contains the wallet's last processed block
+  hash and height at the time the transaction information was generated.
+- The `getwalletinfo` RPC now returns a `lastprocessedblock` JSON object which contains the wallet's last processed block
+  hash and height at the time the wallet information was generated.
\ No newline at end of file
diff --git a/src/wallet/rpc/coins.cpp b/src/wallet/rpc/coins.cpp
index 4c386789f1d70..750ef69f6eaa0 100644
--- a/src/wallet/rpc/coins.cpp
+++ b/src/wallet/rpc/coins.cpp
@@ -448,6 +448,7 @@ RPCHelpMan getbalances()
                     {RPCResult::Type::STR_AMOUNT, "untrusted_pending", "untrusted pending balance (outputs created by others that are in the mempool)"},
                     {RPCResult::Type::STR_AMOUNT, "immature", "balance from immature coinbase outputs"},
                 }},
+                RESULT_LAST_PROCESSED_BLOCK,
             }
             },
         RPCExamples{
@@ -488,6 +489,8 @@ RPCHelpMan getbalances()
         balances_watchonly.pushKV("immature", ValueFromAmount(bal.m_watchonly_immature));
         balances.pushKV("watchonly", balances_watchonly);
     }
+
+    AppendLastProcessedBlock(balances, wallet);
     return balances;
 },
     };
diff --git a/src/wallet/rpc/transactions.cpp b/src/wallet/rpc/transactions.cpp
index eb4f4c87ae897..c34391e6e867c 100644
--- a/src/wallet/rpc/transactions.cpp
+++ b/src/wallet/rpc/transactions.cpp
@@ -731,6 +731,7 @@ RPCHelpMan gettransaction()
                         {
                             {RPCResult::Type::ELISION, "", "Equivalent to the RPC decoderawtransaction method, or the RPC getrawtransaction method when `verbose` is passed."},
                         }},
+                        RESULT_LAST_PROCESSED_BLOCK,
                     })
                 },
                 RPCExamples{
@@ -791,6 +792,7 @@ RPCHelpMan gettransaction()
         entry.pushKV("decoded", decoded);
     }
 
+    AppendLastProcessedBlock(entry, *pwallet);
     return entry;
 },
     };
diff --git a/src/wallet/rpc/util.cpp b/src/wallet/rpc/util.cpp
index 4d82e0a41f326..4ff44b84b0791 100644
--- a/src/wallet/rpc/util.cpp
+++ b/src/wallet/rpc/util.cpp
@@ -177,4 +177,14 @@ void HandleWalletError(const std::shared_ptr<CWallet> wallet, DatabaseStatus& st
         throw JSONRPCError(code, error.original);
     }
 }
+
+void AppendLastProcessedBlock(UniValue& entry, const CWallet& wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)
+{
+    AssertLockHeld(wallet.cs_wallet);
+    UniValue lastprocessedblock{UniValue::VOBJ};
+    lastprocessedblock.pushKV("hash", wallet.GetLastBlockHash().GetHex());
+    lastprocessedblock.pushKV("height", wallet.GetLastBlockHeight());
+    entry.pushKV("lastprocessedblock", lastprocessedblock);
+}
+
 } // namespace wallet
diff --git a/src/wallet/rpc/util.h b/src/wallet/rpc/util.h
index d5d6ac0dfa8ae..2fdba04352255 100644
--- a/src/wallet/rpc/util.h
+++ b/src/wallet/rpc/util.h
@@ -5,7 +5,9 @@
 #ifndef BITCOIN_WALLET_RPC_UTIL_H
 #define BITCOIN_WALLET_RPC_UTIL_H
 
+#include <rpc/util.h>
 #include <script/script.h>
+#include <wallet/wallet.h>
 
 #include <any>
 #include <memory>
@@ -17,13 +19,17 @@ class UniValue;
 struct bilingual_str;
 
 namespace wallet {
-class CWallet;
 class LegacyScriptPubKeyMan;
 enum class DatabaseStatus;
 struct WalletContext;
 
 extern const std::string HELP_REQUIRING_PASSPHRASE;
 
+static const RPCResult RESULT_LAST_PROCESSED_BLOCK { RPCResult::Type::OBJ, "lastprocessedblock", "hash and height of the block this information was generated on",{
+    {RPCResult::Type::STR_HEX, "hash", "hash of the block this information was generated on"},
+    {RPCResult::Type::NUM, "height", "height of the block this information was generated on"}}
+};
+
 /**
  * Figures out what wallet, if any, to use for a JSONRPCRequest.
  *
@@ -45,8 +51,8 @@ std::string LabelFromValue(const UniValue& value);
 void PushParentDescriptors(const CWallet& wallet, const CScript& script_pubkey, UniValue& entry);
 
 void HandleWalletError(const std::shared_ptr<CWallet> wallet, DatabaseStatus& status, bilingual_str& error);
-
 int64_t ParseISO8601DateTime(const std::string& str);
+void AppendLastProcessedBlock(UniValue& entry, const CWallet& wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
 } //  namespace wallet
 
 #endif // BITCOIN_WALLET_RPC_UTIL_H
diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp
index ea3507bc75177..a3a2a7b89c76b 100644
--- a/src/wallet/rpc/wallet.cpp
+++ b/src/wallet/rpc/wallet.cpp
@@ -68,6 +68,7 @@ static RPCHelpMan getwalletinfo()
                         }, /*skip_type_check=*/true},
                         {RPCResult::Type::BOOL, "descriptors", "whether this wallet uses descriptors for scriptPubKey management"},
                         {RPCResult::Type::BOOL, "external_signer", "whether this wallet is configured to use an external signer such as a hardware wallet"},
+                        RESULT_LAST_PROCESSED_BLOCK,
                     }},
                 },
                 RPCExamples{
@@ -129,6 +130,8 @@ static RPCHelpMan getwalletinfo()
     }
     obj.pushKV("descriptors", pwallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS));
     obj.pushKV("external_signer", pwallet->IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER));
+
+    AppendLastProcessedBlock(obj, *pwallet);
     return obj;
 },
     };


Test patch:
diff --git a/test/functional/wallet_balance.py b/test/functional/wallet_balance.py
index 9ed2caefb7562..af9270a321992 100755
--- a/test/functional/wallet_balance.py
+++ b/test/functional/wallet_balance.py
@@ -11,6 +11,7 @@
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_equal,
+    assert_is_hash_string,
     assert_raises_rpc_error,
 )
 
@@ -183,8 +184,13 @@ def test_balances(*, fee_node_1=0):
                                                  'untrusted_pending': Decimal('30.0') - fee_node_1}}  # Doesn't include output of node 0's send since it was spent
             if self.options.descriptors:
                 del expected_balances_0["watchonly"]
-            assert_equal(self.nodes[0].getbalances(), expected_balances_0)
-            assert_equal(self.nodes[1].getbalances(), expected_balances_1)
+            balances_0 = self.nodes[0].getbalances()
+            balances_1 = self.nodes[1].getbalances()
+            # remove lastprocessedblock keys (they will be tested later)
+            del balances_0['lastprocessedblock']
+            del balances_1['lastprocessedblock']
+            assert_equal(balances_0, expected_balances_0)
+            assert_equal(balances_1, expected_balances_1)
             # getbalance without any arguments includes unconfirmed transactions, but not untrusted transactions
             assert_equal(self.nodes[0].getbalance(), Decimal('9.99'))  # change from node 0's send
             assert_equal(self.nodes[1].getbalance(), Decimal('0'))  # node 1's send had an unsafe input
@@ -309,5 +315,30 @@ def test_balances(*, fee_node_1=0):
             assert_equal(self.nodes[0].getbalances()['mine']['untrusted_pending'], Decimal('0.1'))
 
 
+        # Tests the lastprocessedblock JSON object in getbalances, getwalletinfo
+        # and gettransaction by checking for valid hex strings and by comparing
+        # the hashes & heights between generated blocks.
+        self.log.info("Test getbalances returns expected lastprocessedblock json object")
+        prev_hash = self.nodes[0].getbestblockhash()
+        prev_height = self.nodes[0].getblock(prev_hash)['height']
+        self.generatetoaddress(self.nodes[0], 5, self.nodes[0].get_deterministic_priv_key().address)
+        lastblock = self.nodes[0].getbalances()['lastprocessedblock']
+        assert_is_hash_string(lastblock['hash'])
+        assert_equal((prev_hash == lastblock['hash']), False)
+        assert_equal(lastblock['height'], prev_height + 5)
+
+        prev_hash = self.nodes[0].getbestblockhash()
+        prev_height = self.nodes[0].getblock(prev_hash)['height']
+        self.log.info("Test getwalletinfo returns expected lastprocessedblock json object")
+        walletinfo = self.nodes[0].getwalletinfo()
+        assert_equal(walletinfo['lastprocessedblock']['height'], prev_height)
+        assert_equal(walletinfo['lastprocessedblock']['hash'], prev_hash)
+
+        self.log.info("Test gettransaction returns expected lastprocessedblock json object")
+        txid = self.nodes[1].sendtoaddress(self.nodes[1].getnewaddress(), 0.01)
+        tx_info = self.nodes[1].gettransaction(txid)
+        assert_equal(tx_info['lastprocessedblock']['height'], prev_height)
+        assert_equal(tx_info['lastprocessedblock']['hash'], prev_hash)
+
 if __name__ == '__main__':
     WalletTest().main()
diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py
index 53ac01686a407..6f33e8159dd31 100755
--- a/test/functional/wallet_basic.py
+++ b/test/functional/wallet_basic.py
@@ -666,7 +666,7 @@ def run_test(self):
                                  "category": baz["category"],
                                  "vout":     baz["vout"]}
         expected_fields = frozenset({'amount', 'bip125-replaceable', 'confirmations', 'details', 'fee',
-                                     'hex', 'time', 'timereceived', 'trusted', 'txid', 'wtxid', 'walletconflicts'})
+                                     'hex', 'lastprocessedblock', 'time', 'timereceived', 'trusted', 'txid', 'wtxid', 'walletconflicts'})
         verbose_field = "decoded"
         expected_verbose_fields = expected_fields | {verbose_field}
 
diff --git a/test/functional/wallet_orphanedreward.py b/test/functional/wallet_orphanedreward.py
index d8931fa62023d..451f8abb0a535 100755
--- a/test/functional/wallet_orphanedreward.py
+++ b/test/functional/wallet_orphanedreward.py
@@ -65,7 +65,10 @@ def run_test(self):
         assert_equal(self.nodes[0].getbestblockhash(), orig_chain_tip)
         self.generate(self.nodes[0], 3)
 
-        assert_equal(self.nodes[1].getbalances(), pre_reorg_conf_bals)
+        balances = self.nodes[1].getbalances()
+        del balances["lastprocessedblock"]
+        del pre_reorg_conf_bals["lastprocessedblock"]
+        assert_equal(balances, pre_reorg_conf_bals)
         assert_equal(self.nodes[1].gettransaction(txid)["details"][0]["abandoned"], True)