wallet: Remove ISMINE_WATCHONLY and watchonly from RPCs #32618

pull achow101 wants to merge 7 commits into bitcoin:master from achow101:delete-ismine-watchonly changing 23 files +169 โˆ’266
  1. achow101 commented at 7:33 pm on May 26, 2025: member

    Descriptor wallets do not use the watchonly behavior as it is not possible to mix watchonly and non-watchonly in a descriptor wallet. With legacy wallets now removed, all of the watchonly handling and reporting code is no longer needed. This PR removes watchonly options and results from the RPCs and the handling of watchonly things from the wallet’s internals.

    With all of the watchonly things removed, ISMINE_WATCH_ONLY is removed as well.

    Split from #32523

    Depends on #32594 for tests that are easier to read

  2. DrahtBot commented at 7:33 pm on May 26, 2025: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32618.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK Eunovo, maflcko, furszy, rkrux
    Stale ACK BrandonOdiwuor

    If your review is incorrectly listed, please react with ๐Ÿ‘Ž to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #32857 (wallet: allow skipping script paths by Sjors)
    • #30972 (Wallet: “listreceivedby*” fix by BrandonOdiwuor)
    • #27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
    • #25269 (wallet: re-activate “AmountWithFeeExceedsBalance” error by furszy)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

    LLM Linter (โœจ experimental)

    Possible typos and grammar issues:

    • In coincontrol.h: โ€œwhile requires all selected inputs be usedโ€ -> โ€œwhile requiring all selected inputs be usedโ€ [corrects the gerund for grammatical parallelism]

    No other typos were found.

    drahtbot_id_4_m

  3. DrahtBot added the label Wallet on May 26, 2025
  4. achow101 force-pushed on May 26, 2025
  5. achow101 force-pushed on May 27, 2025
  6. DrahtBot added the label Needs rebase on Jun 4, 2025
  7. achow101 force-pushed on Jun 4, 2025
  8. achow101 force-pushed on Jun 4, 2025
  9. DrahtBot removed the label Needs rebase on Jun 5, 2025
  10. in src/wallet/rpc/coins.cpp:174 in 69c6a2abc3 outdated
    170@@ -171,7 +171,7 @@ RPCHelpMan getbalance()
    171                 {
    172                     {"dummy", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "Remains for backward compatibility. Must be excluded or set to \"*\"."},
    173                     {"minconf", RPCArg::Type::NUM, RPCArg::Default{0}, "Only include transactions confirmed at least this many times."},
    174-                    {"include_watchonly", RPCArg::Type::BOOL, RPCArg::DefaultHint{"true for watch-only wallets, otherwise false"}, "Also include balance in watch-only addresses (see 'importaddress')"},
    175+                    {"include_watchonly", RPCArg::Type::BOOL, RPCArg::Default{false}, "No longer used"},
    


    rkrux commented at 11:35 am on June 12, 2025:
    Does this argument include_watchonly not require the DEPRECATED flag now in the RPCResult description?

    achow101 commented at 6:47 pm on June 12, 2025:
    I suppose so, added to this and all others in the RPC docs.
  11. achow101 force-pushed on Jun 12, 2025
  12. achow101 force-pushed on Jun 14, 2025
  13. achow101 marked this as ready for review on Jun 14, 2025
  14. achow101 commented at 0:56 am on June 14, 2025: member
    Ready for review
  15. DrahtBot added the label Needs rebase on Jun 19, 2025
  16. achow101 force-pushed on Jun 20, 2025
  17. DrahtBot removed the label Needs rebase on Jun 20, 2025
  18. maflcko commented at 3:55 pm on June 24, 2025: member

    thx for the test

    review ACK 003a3cdb29dcc1e3e1a53f8227de73389071fbd1 ๐Ÿ“

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
    1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
    2trusted comment: review ACK 003a3cdb29dcc1e3e1a53f8227de73389071fbd1 ๐Ÿ“
    3W/H75G0Njq3xUMblb1LYeqKLIsDTB7AbxYK6n8ESm5k84ss0X35FSu3GEVPcDpVGWOancqnM5G43AeA9J97pCw==
    
  19. BrandonOdiwuor commented at 8:18 am on July 1, 2025: contributor

    Tested ACK 003a3cdb29dcc1e3e1a53f8227de73389071fbd1

    Confirmed that all watch-only related functionality has been removed from the wallet and relevant RPCs, in line with the deprecation of legacy wallets.

    • Verified that watch only has been removed from balances and getbalance RPC.
    • Tested that include_watchonly option and iswatchonly flag was removed from all RPCs including getaddressinfo, fundrawtransaction, send, sendall, walletcreatefundedpsbt, listreceivedbyaddress, listreceivedbylabel, listtransactions, listsinceblock, simulaterawtransaction
  20. Eunovo commented at 9:26 am on July 1, 2025: contributor

    Consider cleaning up wallet_balance.py too, like this:

     0diff --git a/test/functional/wallet_balance.py b/test/functional/wallet_balance.py
     1index 4efc0169eb..ac0374c3fa 100755
     2--- a/test/functional/wallet_balance.py
     3+++ b/test/functional/wallet_balance.py
     4@@ -80,9 +80,9 @@ class WalletTest(BitcoinTestFramework):
     5         assert_equal(self.nodes[0].getbalance("*"), 50)
     6         assert_equal(self.nodes[0].getbalance("*", 1), 50)
     7         assert_equal(self.nodes[0].getbalance(minconf=1), 50)
     8-        assert_equal(self.nodes[0].getbalance(minconf=0, include_watchonly=True), 50)
     9-        assert_equal(self.nodes[0].getbalance("*", 1, True), 50)
    10-        assert_equal(self.nodes[1].getbalance(minconf=0, include_watchonly=True), 50)
    11+        assert_equal(self.nodes[0].getbalance(minconf=0), 50)
    12+        assert_equal(self.nodes[0].getbalance("*", 1), 50)
    13+        assert_equal(self.nodes[1].getbalance(minconf=0), 50)
    14 
    15         # Send 40 BTC from 0 to 1 and 60 BTC from 1 to 0.
    16         txs = create_transactions(self.nodes[0], self.nodes[1].getnewaddress(), 40, [Decimal('0.01')])
    17@@ -142,14 +142,10 @@ class WalletTest(BitcoinTestFramework):
    18             # getbalances
    19             expected_balances_0 = {'mine':      {'immature':          Decimal('0E-8'),
    20                                                  'trusted':           Decimal('9.99'),  # change from node 0's send
    21-                                                 'untrusted_pending': Decimal('60.0')},
    22-                                   'watchonly': {'immature':          Decimal('5000'),
    23-                                                 'trusted':           Decimal('50.0'),
    24-                                                 'untrusted_pending': Decimal('0E-8')}}
    25+                                                 'untrusted_pending': Decimal('60.0')}}
    26             expected_balances_1 = {'mine':      {'immature':          Decimal('0E-8'),
    27                                                  'trusted':           Decimal('0E-8'),  # node 1's send had an unsafe input
    28                                                  'untrusted_pending': Decimal('30.0') - fee_node_1}}  # Doesn't include output of node 0's send since it was spent
    29-            del expected_balances_0["watchonly"]
    30             balances_0 = self.nodes[0].getbalances()
    31             balances_1 = self.nodes[1].getbalances()
    32             # remove lastprocessedblock keys (they will be tested later)
    
  21. Eunovo commented at 9:35 am on July 1, 2025: contributor

    Here is a diff removing include_watchonly=True from other functional tests, except wallet_migration.py

     0diff --git a/test/functional/wallet_importprunedfunds.py b/test/functional/wallet_importprunedfunds.py
     1index cb052e4511..d45be4aa18 100755
     2--- a/test/functional/wallet_importprunedfunds.py
     3+++ b/test/functional/wallet_importprunedfunds.py
     4@@ -89,7 +89,7 @@ class ImportPrunedFundsTest(BitcoinTestFramework):
     5         wwatch = self.nodes[1].get_wallet_rpc('wwatch')
     6         wwatch.importdescriptors([{"desc": self.nodes[0].getaddressinfo(address2)["desc"], "timestamp": "now"}])
     7         wwatch.importprunedfunds(rawtransaction=rawtxn2, txoutproof=proof2)
     8-        assert [tx for tx in wwatch.listtransactions(include_watchonly=True) if tx['txid'] == txnid2]
     9+        assert [tx for tx in wwatch.listtransactions() if tx['txid'] == txnid2]
    10 
    11         # Import with private key with no rescan
    12         w1 = self.nodes[1].get_wallet_rpc(self.default_wallet_name)
    13@@ -109,13 +109,13 @@ class ImportPrunedFundsTest(BitcoinTestFramework):
    14 
    15         # Remove transactions
    16         assert_raises_rpc_error(-4, f'Transaction {txnid1} does not belong to this wallet', w1.removeprunedfunds, txnid1)
    17-        assert not [tx for tx in w1.listtransactions(include_watchonly=True) if tx['txid'] == txnid1]
    18+        assert not [tx for tx in w1.listtransactions() if tx['txid'] == txnid1]
    19 
    20         wwatch.removeprunedfunds(txnid2)
    21-        assert not [tx for tx in wwatch.listtransactions(include_watchonly=True) if tx['txid'] == txnid2]
    22+        assert not [tx for tx in wwatch.listtransactions() if tx['txid'] == txnid2]
    23 
    24         w1.removeprunedfunds(txnid3)
    25-        assert not [tx for tx in w1.listtransactions(include_watchonly=True) if tx['txid'] == txnid3]
    26+        assert not [tx for tx in w1.listtransactions() if tx['txid'] == txnid3]
    27 
    28         # Check various RPC parameter validation errors
    29         assert_raises_rpc_error(-22, "TX decode failed", w1.importprunedfunds, b'invalid tx'.hex(), proof1)
    30diff --git a/test/functional/wallet_listreceivedby.py b/test/functional/wallet_listreceivedby.py
    31index d6219af17c..91dc1a9528 100755
    32--- a/test/functional/wallet_listreceivedby.py
    33+++ b/test/functional/wallet_listreceivedby.py
    34@@ -27,7 +27,7 @@ class ReceivedByTest(BitcoinTestFramework):
    35 
    36     def run_test(self):
    37         # save the number of coinbase reward addresses so far
    38-        num_cb_reward_addresses = len(self.nodes[1].listreceivedbyaddress(minconf=0, include_empty=True, include_watchonly=True))
    39+        num_cb_reward_addresses = len(self.nodes[1].listreceivedbyaddress(minconf=0, include_empty=True))
    40 
    41         self.log.info("listreceivedbyaddress Test")
    42 
    43@@ -67,7 +67,7 @@ class ReceivedByTest(BitcoinTestFramework):
    44         # Test Address filtering
    45         # Only on addr
    46         expected = {"address": addr, "label": "", "amount": Decimal("0.1"), "confirmations": 10, "txids": [txid, ]}
    47-        res = self.nodes[1].listreceivedbyaddress(minconf=0, include_empty=True, include_watchonly=True, address_filter=addr)
    48+        res = self.nodes[1].listreceivedbyaddress(minconf=0, include_empty=True, address_filter=addr)
    49         assert_array_result(res, {"address": addr}, expected)
    50         assert_equal(len(res), 1)
    51         # Test for regression on CLI calls with address string (#14173)
    52@@ -75,7 +75,7 @@ class ReceivedByTest(BitcoinTestFramework):
    53         assert_array_result(cli_res, {"address": addr}, expected)
    54         assert_equal(len(cli_res), 1)
    55         # Error on invalid address
    56-        assert_raises_rpc_error(-4, "address_filter parameter was invalid", self.nodes[1].listreceivedbyaddress, minconf=0, include_empty=True, include_watchonl
    57y=True, address_filter="bamboozling")
    58+        assert_raises_rpc_error(-4, "address_filter parameter was invalid", self.nodes[1].listreceivedbyaddress, minconf=0, include_empty=True, address_filter="
    59bamboozling")
    60         # Another address receive money
    61         res = self.nodes[1].listreceivedbyaddress(0, True, True)
    62         assert_equal(len(res), 2 + num_cb_reward_addresses)  # Right now 2 entries
    
  22. DrahtBot added the label Needs rebase on Jul 1, 2025
  23. test: Watchonly wallets should estimate larger size
    Test that when a watchonly wallet and the wallet with private keys fund
    the same tx, the watchonly wallet should use a higher fee since it
    should be estimating the size to be larger as it assumes the signer
    cannot grind the R value.
    9991f49c38
  24. wallet: Wallets without private keys cannot grind R d20dc9c6aa
  25. achow101 force-pushed on Jul 1, 2025
  26. wallet: Remove watchonly balances
    Descriptor wallets do not have mixed mine and watchonly, so there is no
    need to report a watchonly balance.
    e81d95d435
  27. wallet, rpc: Remove watchonly from RPCs
    Descriptor wallets don't have a conception of watchonly within a wallet,
    so remove all of these options and results from the RPCs
    1337c72198
  28. wallet, spend: Remove fWatchOnly from CCoinControl
    Descriptor wallets do not have a concept of watchonly within a wallet,
    so remove the watchonly option from coin control.
    4439bf4b41
  29. wallet: Remove ISMINE_WATCH_ONLY
    ISMINE_WATCH_ONLY has been removed from all places it was being used,
    and migration does not need ISMINE_WATCH_ONLY, so remove the enum.
    15710869e1
  30. doc: Release note for removed watchonly parameters and results b1a8ac07e9
  31. achow101 force-pushed on Jul 1, 2025
  32. achow101 commented at 6:27 pm on July 1, 2025: member
    Removed the include_watchonly=True.
  33. DrahtBot removed the label Needs rebase on Jul 1, 2025
  34. DrahtBot requested review from BrandonOdiwuor on Jul 2, 2025
  35. DrahtBot requested review from maflcko on Jul 2, 2025
  36. maflcko commented at 8:29 am on July 2, 2025: member

    re-ACK b1a8ac07e91dd1d305fcbc16ea931d60e46c0055 ๐ŸŒˆ

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
    1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
    2trusted comment: re-ACK b1a8ac07e91dd1d305fcbc16ea931d60e46c0055 ๐ŸŒˆ
    3tZimYRLhaIkbxWPb9bFw84mEs8fM0kMiwUZjFXeBP0hYZUEFy8jrAvayLEjmA/01c/AUJfMl9Ob/d0yijdVmAw==
    
  37. in test/functional/wallet_migration.py:365 in 1337c72198 outdated
    361@@ -363,7 +362,7 @@ def test_other_watchonly(self):
    362         assert_raises_rpc_error(-5, "Invalid or non-wallet transaction id", imports0.gettransaction, received_watchonly_txid)
    363         assert_raises_rpc_error(-5, "Invalid or non-wallet transaction id", imports0.gettransaction, received_sent_watchonly_utxo['txid'])
    364         assert_raises_rpc_error(-5, "Invalid or non-wallet transaction id", imports0.gettransaction, sent_watchonly_txid)
    365-        assert_equal(len(imports0.listtransactions(include_watchonly=True)), 2)
    366+        assert_equal(len(imports0.listtransactions()), 2)
    


    rkrux commented at 11:10 am on July 2, 2025:

    In 1337c72198a7d32935431d64e9e58c12f9003abc “wallet, rpc: Remove watchonly from RPCs”

    Similar to this, on line 386 include_watchonly can be removed for the watchonly wallet because that, too, is asserted after migration on the master node. I tested on local that it works fine.

    0- assert_equal(len(watchonly.listtransactions(include_watchonly=True)), 4)
    1+ assert_equal(len(watchonly.listtransactions()), 4)
    

    achow101 commented at 8:14 pm on July 2, 2025:
    Will do if I need to retouch
  38. in src/wallet/spend.cpp:55 in 4439bf4b41 outdated
    53@@ -54,7 +54,7 @@ static bool IsSegwit(const Descriptor& desc) {
    54 static bool UseMaxSig(const std::optional<CTxIn>& txin, const CCoinControl* coin_control) {
    55     // Use max sig if watch only inputs were used or if this particular input is an external input
    


    rkrux commented at 11:13 am on July 2, 2025:

    In 4439bf4b41a6997d4d965f00a8c40efa9cf6895b “wallet, spend: Remove fWatchOnly from CCoinControl”

    0-  // Use max sig if watch only inputs were used or if this particular input is an external input
    1+ // Use max sig if this particular input is an external input
    

    achow101 commented at 8:14 pm on July 2, 2025:
    Will do if I need to retouch
  39. in src/wallet/spend.cpp:57 in 4439bf4b41 outdated
    53@@ -54,7 +54,7 @@ static bool IsSegwit(const Descriptor& desc) {
    54 static bool UseMaxSig(const std::optional<CTxIn>& txin, const CCoinControl* coin_control) {
    55     // Use max sig if watch only inputs were used or if this particular input is an external input
    56     // to ensure a sufficient fee is attained for the requested feerate.
    57-    return coin_control && (coin_control->fAllowWatchOnly || (txin && coin_control->IsExternalSelected(txin->prevout)));
    58+    return coin_control && txin && coin_control->IsExternalSelected(txin->prevout);
    


    rkrux commented at 11:22 am on July 2, 2025:

    In 4439bf4b41a6997d4d965f00a8c40efa9cf6895b “wallet, spend: Remove fWatchOnly from CCoinControl”

    The UseMaxSig function now tests only for the input being externally selected. The only usage of this function is inside MaxInputWeight function. Maybe the following diff so that this function still remains generic (instead of the above suggestion #32618 (review))? Also aligns well with the function doc.

     0@@ -51,10 +51,10 @@ static bool IsSegwit(const Descriptor& desc) {
     1 }
     2 
     3 /** Whether to assume ECDSA signatures' will be high-r. */
     4-static bool UseMaxSig(const std::optional<CTxIn>& txin, const CCoinControl* coin_control) {
     5-    // Use max sig if watch only inputs were used or if this particular input is an external input
     6+static bool UseMaxSig(const std::optional<CTxIn>& txin, const CCoinControl* coin_control, const bool can_grind_r) {
     7+    // Use max sig if the wallet can't grind R or if this particular input is an external input
     8     // to ensure a sufficient fee is attained for the requested feerate.
     9-    return coin_control && txin && coin_control->IsExternalSelected(txin->prevout);
    10+    return (!can_grind_r || (coin_control && txin && coin_control->IsExternalSelected(txin->prevout)));
    11 }
    12 
    13 /** Get the size of an input (in witness units) once it's signed.
    14@@ -68,7 +68,7 @@ static bool UseMaxSig(const std::optional<CTxIn>& txin, const CCoinControl* coin
    15 static std::optional<int64_t> MaxInputWeight(const Descriptor& desc, const std::optional<CTxIn>& txin,
    16                                              const CCoinControl* coin_control, const bool tx_is_segwit,
    17                                              const bool can_grind_r) {
    18-    if (const auto sat_weight = desc.MaxSatisfactionWeight(!can_grind_r || UseMaxSig(txin, coin_control))) {
    19+    if (const auto sat_weight = desc.MaxSatisfactionWeight(UseMaxSig(txin, coin_control, can_grind_r))) {
    20         if (const auto elems_count = desc.MaxSatisfactionElems()) {
    21             const bool is_segwit = IsSegwit(desc);
    22             // Account for the size of the scriptsig and the number of elements on the witness stack. Note
    

    achow101 commented at 8:14 pm on July 2, 2025:
    Will do if I need to retouch
  40. in src/wallet/rpc/addresses.cpp:383 in 1337c72198 outdated
    379@@ -380,7 +380,7 @@ RPCHelpMan getaddressinfo()
    380                         {RPCResult::Type::STR, "address", "The bitcoin address validated."},
    381                         {RPCResult::Type::STR_HEX, "scriptPubKey", "The hex-encoded output script generated by the address."},
    382                         {RPCResult::Type::BOOL, "ismine", "If the address is yours."},
    383-                        {RPCResult::Type::BOOL, "iswatchonly", "If the address is watchonly."},
    384+                        {RPCResult::Type::BOOL, "iswatchonly", "(DEPRECATED) Always false."},
    


    rkrux commented at 11:29 am on July 2, 2025:

    In 1337c72198a7d32935431d64e9e58c12f9003abc “wallet, rpc: Remove watchonly from RPCs”

    I find it unsettling that this field is shown to be false even for watch-only wallets. Can’t we remove this field and mark this commit a breaking change? Reference: Conventional Commits - Breaking Change.-,BREAKING%20CHANGE,-%3A%20a%20commit)


    achow101 commented at 6:26 pm on July 2, 2025:

    It’s already false for existing descriptor wallets.

    The purpose of marking it deprecated is so that it can be removed in the future. Otherwise, it may be surprising to users that this field suddenly disappeared.


    rkrux commented at 6:20 am on July 3, 2025:
    Got it. Generally how long do we wait to remove a field from the RPC interface after marking it deprecated?
  41. rkrux commented at 12:13 pm on July 2, 2025: contributor

    Concept ACK b1a8ac07e91dd1d305fcbc16ea931d60e46c0055

    Thanks for splitting this PR out of the other one, it’s easier to review now. I have left few minor suggestions.

    Descriptor wallets do not use the watchonly behavior as it is not possible to mix watchonly and non-watchonly in a descriptor wallet.

    For this^, I verified that descriptors lacking all the private keys can’t be imported in non-watch-only (descriptor) wallets.

    https://github.com/bitcoin/bitcoin/blob/7fa9b58bd9078809037dea306e8a00c3bc9c9d46/src/wallet/rpc/backup.cpp#L251-L259

  42. furszy commented at 8:10 pm on July 2, 2025: member
    light code review ACK b1a8ac07e91dd1d305fcbc16ea931d60e46c0055
  43. DrahtBot requested review from rkrux on Jul 2, 2025
  44. rkrux approved
  45. rkrux commented at 6:14 am on July 3, 2025: contributor

    ACK b1a8ac07e91dd1d305fcbc16ea931d60e46c0055

    This is a necessary and timely cleanup.

  46. glozow merged this on Jul 7, 2025
  47. glozow closed this on Jul 7, 2025


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-07-25 18:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me