rpc: Added ability to remove watch only addresses

benthecarman commented at 2:41 pm on January 8, 2019: contributor

I added a new rpc call that allows you to remove a watch only address

Format: removeaddress address/script (p2sh) (purgetransactions)

fanquake added the label Wallet on Jan 8, 2019

fanquake added the label RPC/REST/ZMQ on Jan 8, 2019

in src/wallet/wallet.cpp:892 in c6e9bca890 outdated

886+
887+    // Notify UI of new or updated transaction
888+    NotifyTransactionChanged(this, tx.GetHash(), CT_DELETED);
889+
890+    // Notify an external script when a wallet transaction comes in or is updated
891+    std::string strCmd = gArgs.GetArg("-walletnotify", "");

promag commented at 2:50 pm on January 8, 2019:

I don’t think this should use the same script.

benthecarman commented at 4:01 pm on January 8, 2019:

Yeah, that makes a lot of sense now that you point that out, however, I have no idea what I’d name that. Any ideas?

promag commented at 4:36 pm on January 8, 2019:

Maybe remove notification? This is only happens after the RPC so the client doesn’t need a notification.

benthecarman commented at 4:57 pm on January 8, 2019:

Yeah that’s true

in src/wallet/rpcdump.cpp:349 in c6e9bca890 outdated

342@@ -343,6 +343,89 @@ UniValue importaddress(const JSONRPCRequest& request)
343     return NullUniValue;
344 }
345 
346+static void RemoveAddress(CWallet*, const CTxDestination& dest);
347+static void RemoveScript(CWallet* const pwallet, const CScript& script) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)
348+{
349+    if (::IsMine(*pwallet, script) == ISMINE_SPENDABLE)

promag commented at 2:50 pm on January 8, 2019:

Add braces here and throughout.

in src/wallet/wallet.cpp:969 in c6e9bca890 outdated

970@@ -947,7 +971,10 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const CBlockI
971             if (pIndex != nullptr)
972                 wtx.SetMerkleBranch(pIndex, posInBlock);
973 
974-            return AddToWallet(wtx, false);
975+            if (IsMine(tx) || IsFromMe(tx))
976+                return AddToWallet(wtx, false);
977+
978+            return RemoveFromWallet(tx);

promag commented at 2:52 pm on January 8, 2019:

Maybe submit this change in a separate PR.

promag commented at 2:56 pm on January 8, 2019: member

Please add some tests.

benthecarman force-pushed on Jan 8, 2019

in src/wallet/wallet.cpp:969 in a3a1233526 outdated

962@@ -947,7 +963,10 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const CBlockI
963             if (pIndex != nullptr)
964                 wtx.SetMerkleBranch(pIndex, posInBlock);
965 
966-            return AddToWallet(wtx, false);
967+            if (IsMine(tx) || IsFromMe(tx))
968+                return AddToWallet(wtx, false);
969+
970+            return RemoveFromWallet(tx);

jonasschnelli commented at 11:35 pm on January 8, 2019:

This looks a bit invasive. I haven’t looked closer at it, but would it not be possible to remove transaction with the removed addresses during LoadToWallet() rather then an expansive rescan?

benthecarman commented at 3:04 am on January 10, 2019:

Without this, it will remove the balance and addresses from the wallets but it will leave all the transactions with their ids in the wallet so with this function it will remove them from the wallet during the rescan. I am not sure if there is a faster way to do it.

jonasschnelli commented at 6:15 am on January 10, 2019:

I don’t see the reason why a rescan would be required. IMO it’s only required after adding a script where its creation-timestamp lies in the past (expected historical transactions). You don’t need to go through the whole (or part of) the blockchain history just to know that a transaction in your wallet doesn’t belong to you anymore. Also, don’t forget that rescans are incompatible for pruned peers.

It should be sufficient to loop though mapWallet and remove transactions that are no longer IsMine(tx) || IsFromMe(tx) (after the deletion of one or many scripts/addresses).

jonasschnelli commented at 11:36 pm on January 8, 2019: contributor

Not opposed against removing scripts. But maybe combining with importmulti for more advanced script removing and batches would eventually make sense.

DrahtBot commented at 10:47 pm on January 9, 2019: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22788 (scripted-diff: Use generate* from TestFramework by MarcoFalke)
#21206 (refactor: Make CWalletTx sync state type-safe by ryanofsky)
#20591 (wallet, bugfix: fix ComputeTimeSmart function during rescanning process. by BitcoinTsunami)
#20096 (wallet: Remove WalletDatabase refcounting and enforce only one Batch access the database at a time by achow101)
#17526 (Add Single Random Draw as an additional coin selection algorithm by achow101)

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.

benthecarman force-pushed on Jan 10, 2019

benthecarman force-pushed on Jan 12, 2019

benthecarman commented at 0:08 am on January 12, 2019: contributor

Added functionality to remove scripts as well

jonasschnelli commented at 0:15 am on January 12, 2019: contributor

Looks much better! Will test soon.

hebasto commented at 8:48 pm on January 16, 2019: member

@benthecarman could you address Travis linter failure?

benthecarman force-pushed on Jan 17, 2019

benthecarman commented at 7:52 pm on January 17, 2019: contributor

Added release notes

DrahtBot added the label Needs rebase on Jan 30, 2019

benthecarman force-pushed on Feb 4, 2019

benthecarman commented at 11:42 am on February 4, 2019: contributor

Rebased and fixed release notes

hebasto commented at 11:55 am on February 4, 2019: member

Could trailing whitespaces be removed? See Travis log: https://travis-ci.org/bitcoin/bitcoin/jobs/488455534

DrahtBot removed the label Needs rebase on Feb 4, 2019

benthecarman force-pushed on Feb 4, 2019

benthecarman commented at 12:15 pm on February 4, 2019: contributor

@hebasto Sorry about that, fixed

in src/wallet/rpcdump.cpp:406 in 4c7d2b68b3 outdated

401+        throw std::runtime_error(
402+            RPCHelpMan{"removeaddress",
403+                "\nRemoves an address or script (in hex) that was being watched as if it were in your wallet but was not being used to spend. Requires a new wallet backup.\n",
404+                {
405+                    {"address", RPCArg::Type::STR, /* opt */ false, /* default_val */ "", "The Bitcoin address (or hex-encoded script)"},
406+                    {"p2sh", RPCArg::Type::BOOL, /* opt */ true, /* default_val */ "false", "Add the P2SH version of the script as well"},

Sjors commented at 4:35 pm on August 15, 2019:

Did you mean “Remove” instead of “Add”?

Sjors commented at 4:39 pm on August 15, 2019:

Suggest adding a third argument purge_transactions (default true) about removing existing transactions from the wallet.

in src/wallet/wallet.cpp:948 in 4c7d2b68b3 outdated

943+    AssertLockHeld(cs_wallet);
944+
945+    std::vector<CWalletTx> vWtx;
946+    for (std::map<uint256, CWalletTx>::iterator it = mapWallet.begin(); it != mapWallet.end(); ++it) {
947+        CWalletTx tx = it->second;
948+        if (!IsFromMe(*tx.tx) && !IsMine(*tx.tx)) {

Sjors commented at 4:38 pm on August 15, 2019:

I think this is too broad. Suggest adding an additional condition that the to be deleted transaction is to or from the specific watch-only address we’re removing.

Sjors commented at 4:43 pm on August 15, 2019: member

Concept ACK

bitcoin deleted a comment on Aug 18, 2019

Sjors commented at 5:54 pm on September 3, 2019: member

This has a silent merge conflict with master because RPCHelpMan syntax changed a bit.

DrahtBot added the label Needs rebase on Sep 5, 2019

adamjonas commented at 0:57 am on May 1, 2020: member

@benthecarman - it looks like Sjor’s comments are still unaddressed and a rebase is needed. Is this still something you intend to continue with?

benthecarman force-pushed on May 3, 2020

DrahtBot removed the label Needs rebase on May 3, 2020

benthecarman commented at 3:23 am on May 4, 2020: contributor

Fixed for @Sjors’s comments and rebased

hebasto commented at 4:28 am on May 4, 2020: member

c7e72e88b40f97a7c3a4c5c07a978bc2f54e6db9: The lint-assertions.sh and lint-python.sh linters do not pass. Please fix.

in src/wallet/wallet.cpp:929 in c7e72e88b4 outdated

923@@ -924,6 +924,42 @@ void CWallet::LoadToWallet(CWalletTx& wtxIn)
924     }
925 }
926 
927+void CWallet::RemoveOldTransactions(const CScript& script)
928+{
929+    // AssertLockHeld(cs_wallet);

Sjors commented at 9:33 am on May 4, 2020:

Why is this entire function commented out?

benthecarman commented at 2:15 pm on May 4, 2020:

woops, pushed wrong commit. Should be good now

benthecarman force-pushed on May 4, 2020

in src/rpc/client.cpp:131 in c91732604e outdated

127@@ -128,6 +128,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
128     { "importprivkey", 2, "rescan" },
129     { "importaddress", 2, "rescan" },
130     { "importaddress", 3, "p2sh" },
131+    { "removeaddress", 2, "p2sh" },

adamjonas commented at 4:45 pm on May 14, 2020:

This is getting caught in the linter. Be sure the position of the argument is consistent with the dispatch table in rpcwallet.cpp (it isn’t – the index starts at 0).

in src/wallet/rpcdump.cpp:335 in c91732604e outdated

330+    if (!isRedeemScript && ismine == ISMINE_SPENDABLE) {
331+        throw JSONRPCError(RPC_WALLET_ERROR, "The wallet contains the private key for this address");
332+    } else if (!isRedeemScript && ismine == ISMINE_NO) {
333+        throw JSONRPCError(RPC_WALLET_ERROR, "The wallet does not contain this address or script");
334+    }
335+    assert(ismine == ISMINE_WATCH_ONLY || isRedeemScript);

adamjonas commented at 4:57 pm on May 14, 2020:

This will be caught too as CHECK_NONFATAL is recommended instead of assert for rpc code.

adamjonas commented at 4:58 pm on May 14, 2020: member

@benthecarman A couple of linter issues to work out before this is ready for further review.

in src/wallet/wallet.cpp:927 in c91732604e outdated

923@@ -924,6 +924,45 @@ void CWallet::LoadToWallet(CWalletTx& wtxIn)
924     }
925 }
926 
927+void CWallet::RemoveOldTransactions(const CScript& script)

kristapsk commented at 11:16 am on May 15, 2020:

I think RemoveTransactions() would be a better name, as this can remove any transactions, being “old” is not a requirement.

kristapsk commented at 11:20 am on May 15, 2020: contributor

This would be a very usefull addition for me, I have a wallet with watch-only addresses imported that I would like to remove.

benthecarman force-pushed on May 15, 2020

kristapsk commented at 3:19 pm on May 15, 2020: contributor

RemoveOldTransactions() got renamed to RemoveTransactions() but calling plance didn’t get changed, so it doesn’t build now.

0wallet/rpcdump.cpp: In function ‘void RemoveScript(CWallet*, const CScript&, bool, bool)’:
1wallet/rpcdump.cpp:356:18: error: ‘class CWallet’ has no member named ‘RemoveOldTransactions’; did you mean ‘RemoveTransactions’?
2  356 |         pwallet->RemoveOldTransactions(script);
3      |                  ^~~~~~~~~~~~~~~~~~~~~
4      |                  RemoveTransactions

benthecarman force-pushed on May 15, 2020

benthecarman commented at 3:22 pm on May 15, 2020: contributor

RemoveOldTransactions() got renamed to RemoveTransactions() but calling plance didn’t get changed, so it doesn’t build now.

0wallet/rpcdump.cpp: In function ‘void RemoveScript(CWallet*, const CScript&, bool, bool)’:
1wallet/rpcdump.cpp:356:18: error: ‘class CWallet’ has no member named ‘RemoveOldTransactions’; did you mean ‘RemoveTransactions’?
2  356 |         pwallet->RemoveOldTransactions(script);
3      |                  ^~~~~~~~~~~~~~~~~~~~~
4      |                  RemoveTransactions

woops, fixed now :)

benthecarman force-pushed on May 15, 2020

in src/wallet/rpcdump.cpp:387 in 15ee301110 outdated

382+                    {"p2sh", RPCArg::Type::BOOL, /* default */ "false", "Remove the P2SH version of the script as well"},
383+                    {"purgetransactions", RPCArg::Type::BOOL, /* default_val */ "false", "Remove existing transactions from the wallet"},
384+                },
385+                RPCResult{RPCResult::Type::NONE, "", ""},
386+                RPCExamples{
387+            "\nExamples:\n"

kristapsk commented at 4:22 pm on May 15, 2020:

This line is not needed, results in a duplicate “Examples:” in help output.

kristapsk changes_requested

benthecarman force-pushed on May 15, 2020

kristapsk approved

kristapsk commented at 9:41 pm on May 15, 2020: contributor

ACK ad0de4c639c215e334bc076c4bc2f6340f8f3a67

adamjonas commented at 3:33 pm on May 19, 2020: member

@benthecarman CIs are still red since ad0de4c.

benthecarman force-pushed on May 20, 2020

benthecarman commented at 1:23 am on May 20, 2020: contributor

@benthecarman CIs are still red since ad0de4c.

Sorry, everything was passing locally, looks like it had silent merge conflicts with master again

in src/wallet/rpcdump.cpp:337 in 8af899113f outdated

332+    } else if (!isRedeemScript && ismine == ISMINE_NO) {
333+        throw JSONRPCError(RPC_WALLET_ERROR, "The wallet does not contain this address or script");
334+    }
335+    CHECK_NONFATAL(ismine == ISMINE_WATCH_ONLY || isRedeemScript);
336+
337+    auto spk_man = pwallet->GetLegacyScriptPubKeyMan();

Sjors commented at 9:01 am on May 28, 2020:

Try this instead, to guarantee it only works for legacy wallets:

0    LegacyScriptPubKeyMan& spk_man = EnsureLegacyScriptPubKeyMan(*pwallet);

Sjors commented at 9:03 am on May 28, 2020: member

Haven’t had a chance to review, but I left a note to better handle descriptor wallets. Keep an eye on #18788 as well…

benthecarman force-pushed on May 28, 2020

achow101 commented at 3:59 pm on June 11, 2020: member

There are a few import scenarios where some scripts and pubkeys get left behind and that could cause IsMine issues. There will also be leftover CKeyMetadata entries. I think these will all need to be cleaned up.

Importing any P2SH or P2WSH address with a redeemScript will add that redeemScript to mapScripts. If pubkeys were imported to the keypool, then those pubkeys are still in the keypool after removeaddress. Those need to be removed otherwise users could be sending money to an address that the wallet is no longer watching.

benthecarman commented at 0:02 am on June 12, 2020: contributor

There are a few import scenarios where some scripts and pubkeys get left behind and that could cause IsMine issues. There will also be leftover CKeyMetadata entries. I think these will all need to be cleaned up.

Importing any P2SH or P2WSH address with a redeemScript will add that redeemScript to mapScripts. If pubkeys were imported to the keypool, then those pubkeys are still in the keypool after removeaddress. Those need to be removed otherwise users could be sending money to an address that the wallet is no longer watching.

Sounds like most of this needs to be fixed, however, do note this is only for watch only addresses. So, I don’t believe we need to worry about the key pool.

achow101 commented at 1:25 am on June 12, 2020: member

however, do note this is only for watch only addresses. So, I don’t believe we need to worry about the key pool.

importmulti allows people to import address with their pubkeys and add the pubkeys into the keypool. The scriptPubKeys still end up in setWatchOnly and the pubkeys in mapWatchKeys, but also those pubkeys go in the keypool so that getnewaddress can work. Anything imported with those and then used with removeaddress would still have the pubkey in the keypool.

benthecarman force-pushed on Jun 14, 2020

benthecarman commented at 9:57 pm on June 14, 2020: contributor

There are a few import scenarios where some scripts and pubkeys get left behind and that could cause IsMine issues. There will also be leftover CKeyMetadata entries. I think these will all need to be cleaned up.

Importing any P2SH or P2WSH address with a redeemScript will add that redeemScript to mapScripts. If pubkeys were imported to the keypool, then those pubkeys are still in the keypool after removeaddress. Those need to be removed otherwise users could be sending money to an address that the wallet is no longer watching.

I believe this is addressed now

benthecarman force-pushed on Jun 19, 2020

benthecarman commented at 0:28 am on June 19, 2020: contributor

Fixed rebase issues

benthecarman force-pushed on Aug 1, 2020

benthecarman commented at 0:30 am on August 1, 2020: contributor

Rebased

in src/wallet/rpcdump.cpp:364 in b3b9d8fafb outdated

359+    if (IsValidDestination(dest)) {
360+        pwallet->DelAddressBook(dest);
361+    }
362+}
363+
364+UniValue removeaddress(const JSONRPCRequest& request)

maflcko commented at 12:13 pm on August 14, 2020:

Might be good to use the non-deprecated constructor (see #19528)

benthecarman commented at 0:36 am on August 15, 2020:

Done.

in src/wallet/rpcdump.cpp:384 in b3b9d8fafb outdated

379+                },
380+            }.Check(request);
381+
382+    std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);
383+    if (!wallet) return NullUniValue;
384+    CWallet* const pwallet = wallet.get();

maflcko commented at 12:15 pm on August 14, 2020:

0    const CWallet& wallet = *pwallet.get();

As the previous line checks for nullptr, might be good to just take a reference instead of keeping to treat it as pointer that can be nullptr.

benthecarman commented at 0:36 am on August 15, 2020:

Done.

maflcko commented at 12:16 pm on August 14, 2020: member

didn’t review, but left two style nits (feel free to ignore)

benthecarman force-pushed on Aug 15, 2020

benthecarman commented at 0:36 am on August 15, 2020: contributor

Applied @MarcoFalke’s suggestions

DrahtBot added the label Needs rebase on Aug 31, 2020

benthecarman force-pushed on Aug 31, 2020

benthecarman commented at 8:48 pm on August 31, 2020: contributor

Rebased

DrahtBot removed the label Needs rebase on Aug 31, 2020

kristapsk approved

kristapsk commented at 1:47 am on November 5, 2020: contributor

re-ACK 96040c834543bfcfb1ce86182294dd5e0985fc7f

benthecarman force-pushed on Jan 27, 2021

benthecarman commented at 11:05 am on January 27, 2021: contributor

Rebased with latest master, fixed issues that came up from that

in src/wallet/wallet.cpp:986 in 06a33cc47e outdated

980@@ -981,6 +981,45 @@ bool CWallet::LoadToWallet(const uint256& hash, const UpdateWalletTxFn& fill_wtx
981     return true;
982 }
983 
984+void CWallet::RemoveTransactions(const CScript& script)
985+{
986+    AssertLockHeld(cs_wallet);

adamjonas commented at 3:43 pm on January 27, 2021:

After the rebase, I’m getting compile warnings for this line consistent with the CI:

 0wallet/wallet.cpp:986:5: warning: calling function 'AssertLockHeldInternal<AnnotatedMixin<std::__1::recursive_mutex> >' requires holding mutex 'cs_wallet' exclusively [-Wthread-safety-analysis]
 1    AssertLockHeld(cs_wallet);
 2    ^
 3./sync.h:81:28: note: expanded from macro 'AssertLockHeld'
 4#define AssertLockHeld(cs) AssertLockHeldInternal(#cs, __FILE__, __LINE__, &cs)
 5                           ^
 6wallet/wallet.cpp:989:54: warning: reading variable 'mapWallet' requires holding mutex 'cs_wallet' [-Wthread-safety-analysis]
 7    for (std::map<uint256, CWalletTx>::iterator it = mapWallet.begin(); it != mapWallet.end(); ++it) {
 8                                                     ^
 9wallet/wallet.cpp:989:79: warning: reading variable 'mapWallet' requires holding mutex 'cs_wallet' [-Wthread-safety-analysis]
10    for (std::map<uint256, CWalletTx>::iterator it = mapWallet.begin(); it != mapWallet.end(); ++it) {
11                                                                              ^
12wallet/wallet.cpp:1001:67: warning: reading variable 'mapWallet' requires holding mutex 'cs_wallet' [-Wthread-safety-analysis]
13                std::map<uint256, CWalletTx>::const_iterator mi = mapWallet.find(txin.prevout.hash);
14                                                                  ^
15wallet/wallet.cpp:1002:27: warning: reading variable 'mapWallet' requires holding mutex 'cs_wallet' [-Wthread-safety-analysis]
16                if (mi != mapWallet.end()) {
17                          ^
18wallet/wallet.cpp:1011:56: warning: calling function 'IsMine' requires holding mutex 'cs_wallet' exclusively [-Wthread-safety-analysis]
19        if (txContainsScript && !IsFromMe(*wTx.tx) && !IsMine(*wTx.tx)) {
20                                                       ^
21wallet/wallet.cpp:1017:9: warning: reading variable 'mapWallet' requires holding mutex 'cs_wallet' [-Wthread-safety-analysis]
22        mapWallet.erase(wtx->GetHash());
23        ^
247 warnings generated.

benthecarman commented at 6:26 pm on January 27, 2021:

Wasn’t able to reproduce myself but seems I was able to fix it

benthecarman force-pushed on Jan 27, 2021

DrahtBot added the label Needs rebase on Jan 28, 2021

benthecarman force-pushed on Jan 29, 2021

benthecarman commented at 10:05 am on January 29, 2021: contributor

Rebased

DrahtBot removed the label Needs rebase on Jan 29, 2021

in doc/release-notes-15129.md:4 in d3a1077c85 outdated

0@@ -0,0 +1,4 @@
1+New RPCs
2+--------
3+- A new `removeaddress` RPC will remove a watch only address or script
4+  from the current wallet.

jonatack commented at 12:15 pm on January 29, 2021:

0  from the current wallet. (#15129)

in src/wallet/rpcdump.cpp:378 in d3a1077c85 outdated

373+        "removeaddress",
374+        "\nRemoves an address or script (in hex) that was being watched as if it were in your wallet but was not being used to spend. Requires a new wallet backup.\n",
375+        {
376+            {"address", RPCArg::Type::STR, RPCArg::Optional::NO, "The Bitcoin address (or hex-encoded script)"},
377+            {"p2sh", RPCArg::Type::BOOL, /* default */ "false", "Remove the P2SH version of the script as well"},
378+            {"purgetransactions", RPCArg::Type::BOOL, /* default_val */ "false", "Remove existing transactions from the wallet"},

jonatack commented at 12:21 pm on January 29, 2021:

ISTM current practice is to use snake case for the field name

0            {"purge_transactions", RPCArg::Type::BOOL, /* default_val */ "false", "Whether to remove existing transactions from the wallet"},

in src/wallet/rpcdump.cpp:377 in d3a1077c85 outdated

372+    return RPCHelpMan{
373+        "removeaddress",
374+        "\nRemoves an address or script (in hex) that was being watched as if it were in your wallet but was not being used to spend. Requires a new wallet backup.\n",
375+        {
376+            {"address", RPCArg::Type::STR, RPCArg::Optional::NO, "The Bitcoin address (or hex-encoded script)"},
377+            {"p2sh", RPCArg::Type::BOOL, /* default */ "false", "Remove the P2SH version of the script as well"},

jonatack commented at 12:22 pm on January 29, 2021:

0            {"p2sh", RPCArg::Type::BOOL, /* default */ "false", "Whether to remove the P2SH version of the script as well"},

in src/wallet/rpcdump.cpp:381 in d3a1077c85 outdated

375+        {
376+            {"address", RPCArg::Type::STR, RPCArg::Optional::NO, "The Bitcoin address (or hex-encoded script)"},
377+            {"p2sh", RPCArg::Type::BOOL, /* default */ "false", "Remove the P2SH version of the script as well"},
378+            {"purgetransactions", RPCArg::Type::BOOL, /* default_val */ "false", "Remove existing transactions from the wallet"},
379+        },
380+        RPCResult{RPCResult::Type::NONE, "", ""},

jonatack commented at 12:25 pm on January 29, 2021:

I believe a JSON object is currently preferred for the output (see other wallet rpcs, e.g. upgradewallet, for examples)

benthecarman commented at 1:01 am on February 6, 2021:

What should be returned?

ryanofsky commented at 5:20 pm on September 1, 2021:

What should be returned?

It does seem ok to me to return null like this PR is currently doing. But if you wanted to extend it you could return lists of scripts and addresses removed by the recursive removescript/removeaddress implementation (maybe in a similar format similar to the importmulti requests parameter). There could also be a “warnings” field for example if you remove a known P2SH redeem script without removing the associated address.

in src/wallet/rpcdump.cpp:395 in d3a1077c85 outdated

390+
391+            // Whether to remove the p2sh version too
392+            bool fP2SH = false;
393+            if (!request.params[1].isNull()) {
394+                fP2SH = request.params[1].get_bool();
395+            }

jonatack commented at 12:30 pm on January 29, 2021:

can replace these lines (and have constness and follow variable naming style per doc/developer-notes.md) with

0const bool is_p2sh{request.params[1].isNull() ? false : request.params[1].get_bool()};

in src/wallet/rpcdump.cpp:401 in d3a1077c85 outdated

396+
397+            // Whether to remove the wallet transactions too
398+            bool fPurgeTxs = false;
399+            if (!request.params[2].isNull()) {
400+                fPurgeTxs = request.params[2].get_bool();
401+            }

jonatack commented at 12:34 pm on January 29, 2021:

idem

0const bool purge_txns{request.params[2].isNull() ? false : request.params[2].get_bool()};

in src/wallet/rpcdump.cpp:327 in d3a1077c85 outdated

322@@ -323,6 +323,103 @@ RPCHelpMan importaddress()
323     };
324 }
325 
326+static void RemoveAddress(CWallet*, const CTxDestination&, bool);
327+static void RemoveScript(CWallet* const pwallet, const CScript& script, bool isRedeemScript, bool purgeTxs) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)

jonatack commented at 12:37 pm on January 29, 2021:

consider using the current naming conventions in doc/developer-notes.md

0static void RemoveScript(CWallet* const pwallet, const CScript& script, bool is_redeem_script, bool purge_txns) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)

in src/wallet/scriptpubkeyman.cpp:908 in d3a1077c85 outdated

887+    }
888+
889+    if (!HaveWatchOnly())
890+        NotifyWatchonlyChanged(false);
891+    if (!WalletBatch(m_storage.GetDatabase()).EraseWatchOnly(dest))
892+        return false;

jonatack commented at 12:41 pm on January 29, 2021:

Please see doc/developer-notes.md about formatting conditionals

0  - If an `if` only has a single-statement `then`-clause, it can appear
1    on the same line as the `if`, without braces. In every other case,
2    braces are required, and the `then` and `else` clauses must appear
3    correctly indented on a new line.

in test/functional/wallet_basic.py:514 in d3a1077c85 outdated

511+            # Remove the address from node1
512+            self.nodes[1].removeaddress(address_to_import2)
513+
514+            # Check the address was removed and other address is unaffected
515+            assert(not self.nodes[1].getaddressinfo(address_to_import2)["iswatchonly"])
516+            assert(self.nodes[1].getaddressinfo(address_to_import)["ismine"])

jonatack commented at 12:45 pm on January 29, 2021:

nit, assert and assert not without parens seems to be the most-used style in the functional tests

jonatack commented at 12:45 pm on January 29, 2021: contributor

Concept ACK

in src/wallet/rpcdump.cpp:383 in d3a1077c85 outdated

377+            {"p2sh", RPCArg::Type::BOOL, /* default */ "false", "Remove the P2SH version of the script as well"},
378+            {"purgetransactions", RPCArg::Type::BOOL, /* default_val */ "false", "Remove existing transactions from the wallet"},
379+        },
380+        RPCResult{RPCResult::Type::NONE, "", ""},
381+        RPCExamples{
382+            "\nRemove an address\n" + HelpExampleCli("removeaddress", "\"myaddress\"") +

jonatack commented at 12:50 pm on January 29, 2021:

Perhaps add an example for removing a script as well

in src/wallet/rpcdump.cpp:375 in d3a1077c85 outdated

369+
370+RPCHelpMan removeaddress()
371+{
372+    return RPCHelpMan{
373+        "removeaddress",
374+        "\nRemoves an address or script (in hex) that was being watched as if it were in your wallet but was not being used to spend. Requires a new wallet backup.\n",

jonatack commented at 12:52 pm on January 29, 2021:

Since this RPC can remove an address or a script, is there a better name than removeaddress? (I don’t have an immediate suggestion.)

benthecarman commented at 0:22 am on February 6, 2021:

importaddress can import a script as well, I think this name works.

benthecarman force-pushed on Feb 6, 2021

benthecarman commented at 2:16 pm on February 7, 2021: contributor

Responded to @jonatack’s review

in src/wallet/wallet.cpp:1012 in fdbd01b50e outdated

1007+                }
1008+            }
1009+        }
1010+
1011+        if (txContainsScript && !IsFromMe(*wTx.tx) && !IsMine(*wTx.tx)) {
1012+            vWtx.push_back(MakeUnique<CWalletTx>(this, MakeTransactionRef(std::move(*wTx.tx))));

fanquake commented at 0:33 am on March 12, 2021:

Please use std::make_unique in new code.

ghost commented at 2:51 am on March 12, 2021: none

Concept ACK. Compiled successfully on Ubuntu. Tests passed.

Tried experimenting:

Adding address with importaddress and removing with removeaddress worked fine

~~Adding address with importmulti gave checksum error maybe because of new checksum used for bech32 addresses but I am not sure.~~

importmulti works fine I was using different public keys in both commands earlier.

Will try experimenting with addmultisigaddress as well. It gave error (The wallet contains the private key for this address) if the multisig was created using addresses from wallet. Maybe addresses/keys that do not belong to wallet will work fine and will be removed with removeaddress.

This functionality is only intended for use with non-watchonly addresses. https://bitcoincore.org/en/doc/0.21.0/rpc/wallet/addmultisigaddress/

So maybe removeaddress will only work for addresses added by importaddress or importmulti. I was assuming addmultisigaddress is a combination of createmultisig and importmulti.

in src/wallet/rpcdump.cpp:378 in fdbd01b50e outdated

373+        "removeaddress",
374+        "\nRemoves an address or script (in hex) that was being watched as if it were in your wallet but was not being used to spend. Requires a new wallet backup.\n",
375+        {
376+            {"address", RPCArg::Type::STR, RPCArg::Optional::NO, "The Bitcoin address (or hex-encoded script)"},
377+            {"p2sh", RPCArg::Type::BOOL, /* default */ "false", "Whether to remove the P2SH version of the script as well"},
378+            {"purge_transactions", RPCArg::Type::BOOL, /* default */ "false", "Whether to remove existing transactions from the wallet"},

luke-jr commented at 7:43 pm on June 22, 2021:

I don’t think these should be positional parameters.

luke-jr changes_requested

luke-jr referenced this in commit 5d0517be4e on Jun 27, 2021

benthecarman force-pushed on Aug 23, 2021

benthecarman commented at 6:56 pm on August 23, 2021: contributor

Rebased and fixed silent merge conflicts

Addressed @fanquake’s comment

unknown approved

unknown commented at 6:21 am on August 24, 2021: none

ACK https://github.com/bitcoin/bitcoin/pull/15129/commits/723aed1c0c36ea32ac831ce51b923ec43edc764b

Major changes since last review: #15129 (review)

kristapsk commented at 11:57 am on August 24, 2021: contributor

It looks to me there is no test for purge_transactions argument of removeaddress, that should be added.

benthecarman commented at 3:20 pm on August 24, 2021: contributor

It looks to me there is no test for purge_transactions argument of removeaddress, that should be added.

Added

benthecarman force-pushed on Aug 24, 2021

kristapsk approved

kristapsk commented at 4:12 pm on August 24, 2021: contributor

ACK 2eb5021e5045c06784244bd92de53b328d9f0b18

ghost commented at 4:51 pm on August 24, 2021: none

reACK https://github.com/bitcoin/bitcoin/commit/2eb5021e5045c06784244bd92de53b328d9f0b18

in src/wallet/rpcdump.cpp:387 in 2eb5021e50 outdated

382+        RPCExamples{
383+            "\nRemove an address\n" + HelpExampleCli("removeaddress", "\"myaddress\"") +
384+            "\nRemove a script\n" + HelpExampleCli("removeaddress", "\"myscript\"") +
385+            "\nAs a JSON-RPC call\n" + HelpExampleRpc("removeaddress", "\"myaddress\"")},
386+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue {
387+            std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);

meshcollider commented at 3:30 am on August 25, 2021:

nit: the body of RPC functions shouldn’t be indented for the RPCHelpMan

maflcko commented at 4:03 pm on August 25, 2021:

I’d say it is allowed for new code to use the right whitespace

in src/wallet/rpcdump.cpp:389 in 2eb5021e50 outdated

384+            "\nRemove a script\n" + HelpExampleCli("removeaddress", "\"myscript\"") +
385+            "\nAs a JSON-RPC call\n" + HelpExampleRpc("removeaddress", "\"myaddress\"")},
386+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue {
387+            std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);
388+            if (!wallet) return NullUniValue;
389+            CWallet& pwallet = *wallet.get();

meshcollider commented at 3:41 am on August 25, 2021:

Why not just use the shared_ptr everywhere?

benthecarman commented at 1:24 am on August 31, 2021:

I think I fixed this

in test/functional/wallet_basic.py:507 in 2eb5021e50 outdated

503@@ -504,15 +504,58 @@ def run_test(self):
504                                 {"address": address_to_import},
505                                 {"spendable": False})
506 
507-            # 5. Import private key of the previously imported address on node1
508+            # 5. Remove the address from node1

meshcollider commented at 4:17 am on August 25, 2021:

nit: I don’t think we need the numbering (makes it annoying to add more cases later)

benthecarman commented at 1:24 am on August 31, 2021:

Removed (this was very annoying)

in test/functional/wallet_basic.py:509 in 2eb5021e50 outdated

503@@ -504,15 +504,58 @@ def run_test(self):
504                                 {"address": address_to_import},
505                                 {"spendable": False})
506 
507-            # 5. Import private key of the previously imported address on node1
508+            # 5. Remove the address from node1
509+            self.nodes[1].removeaddress(address_to_import, False, False)
510+

meshcollider commented at 4:20 am on August 25, 2021:

Would be good to validate here that the address is not in the wallet (like below)

benthecarman commented at 1:23 am on August 31, 2021:

Added

in test/functional/wallet_basic.py:579 in 2eb5021e50 outdated

555+            self.nodes[1].removeaddress(address_to_import2)
556+
557+            # Check the address was removed and other address is unaffected
558+            assert not self.nodes[1].getaddressinfo(address_to_import2)["iswatchonly"]
559+            assert self.nodes[1].getaddressinfo(address_to_import)["ismine"]
560+

meshcollider commented at 4:22 am on August 25, 2021:

It would be good to test the p2sh flag as well, all these examples only touch addresses not scripts

benthecarman commented at 1:23 am on August 31, 2021:

Added.

meshcollider commented at 4:23 am on August 25, 2021: contributor

Thank you for maintaining this PR @benthecarman, it is definitely getting close!

rpc: Added ability to remove watch only addresses b8eb588069

benthecarman force-pushed on Aug 31, 2021

benthecarman commented at 1:24 am on August 31, 2021: contributor

Responded to @meshcollider’s review

in src/wallet/wallet.cpp:1056 in b8eb588069

1051+                    }
1052+                }
1053+            }
1054+        }
1055+
1056+        if (txContainsScript && !IsFromMe(*wTx.tx) && !IsMine(*wTx.tx)) {

ryanofsky commented at 4:46 pm on September 1, 2021:

In commit “rpc: Added ability to remove watch only addresses” (b8eb5880693358951229512987beffcff51e0ecf)

Note (no changes requested): Check for isfromme / ismine mirrors existing logic in AddToWalletIfInvolvingMe and seems sufficient. I was initially thinking the IsFromMe check might fail to remove all relevant transactions because the for loop here ignores dependencies between transactions, so if IsFromMe was called on a child transaction and the parent was only removed later, the child might fail to be removed. But I think this not a problem because the PR only supports moving watch-only addresses, not spendable addresses, so no IsFromMe result on any transaction will be changed by watch only address removal.

in src/wallet/rpcdump.cpp:378 in b8eb588069

373+    return RPCHelpMan{
374+        "removeaddress",
375+        "\nRemoves an address or script (in hex) that was being watched as if it were in your wallet but was not being used to spend. Requires a new wallet backup.\n",
376+        {
377+            {"address", RPCArg::Type::STR, RPCArg::Optional::NO, "The Bitcoin address (or hex-encoded script)"},
378+            {"p2sh", RPCArg::Type::BOOL, RPCArg::Default{false}, "Whether to remove the P2SH version of the script as well"},

ryanofsky commented at 5:08 pm on September 1, 2021:

In commit “rpc: Added ability to remove watch only addresses” (b8eb5880693358951229512987beffcff51e0ecf)

This documentation is mirrored from the importaddress documentation and it is good that you are keeping it consistent, but I think it is actually a little confusing and would be nice to clarify both places. I would maybe change the help string here to something like "If this is true and a script is being removed (not an address, if an address is being removed this is required to be false), then treat the script as a P2SH redeem script and remove the associated P2SH address from the wallet as well." For importaddress you could just use the same help text just replacing “remove[d]” with “add[ed]”.

It might be also be helpful to add a general note about the limitations of this RPC like “This RPC method does not currently support removing P2WSH or P2SH-P2WSH witness scripts at the same time as their associated addresses, the same way it does support this P2SH redeem scripts, so separate calls must be made to do this, first removing the addresses, and then removing the witness scripts.”

in src/wallet/rpcdump.cpp:346 in b8eb588069

341+        throw JSONRPCError(RPC_WALLET_ERROR, "Error removing address/script from wallet");
342+    }
343+    if (is_redeem_script) {
344+        CScriptID scriptID = CScriptID(script);
345+        if (!spk_man.HaveCScript(scriptID)) {
346+            throw JSONRPCError(RPC_WALLET_ERROR, "Error removing p2sh redeemScript from wallet");

ryanofsky commented at 5:24 pm on September 1, 2021:

In commit “rpc: Added ability to remove watch only addresses” (b8eb5880693358951229512987beffcff51e0ecf)

Important: It seems dangerous to do this error check and raise an error after calling PurgeWatchOnly above. It would be better to check for this error condition early so the remove either fully fails or fully succeeds instead of half removing the redeem script.

in src/wallet/rpcdump.cpp:345 in b8eb588069

340+    if (spk_man.HaveWatchOnly(script) && !spk_man.PurgeWatchOnly(script)) {
341+        throw JSONRPCError(RPC_WALLET_ERROR, "Error removing address/script from wallet");
342+    }
343+    if (is_redeem_script) {
344+        CScriptID scriptID = CScriptID(script);
345+        if (!spk_man.HaveCScript(scriptID)) {

ryanofsky commented at 5:43 pm on September 1, 2021:

In commit “rpc: Added ability to remove watch only addresses” (b8eb5880693358951229512987beffcff51e0ecf)

Important: Because there is no RemoveCScript function being called here to undo the effect of AddCScript/AddCScriptWithDB, the removeaddress call does not fully undo the effects of the equivalent importaddress call when the p2sh argument is true, and a copy of the script is still left behind in mapScripts and in the database file. It would be good to either note this as a limitation of the RPC in the documentation, or to fix it by actually removing the script and scriptid from the database.

ryanofsky commented at 6:06 pm on September 1, 2021:

re: #15129 (review)

a copy of the script is still left behind in mapScripts and in the database file

Might be good follow up with @achow101 about his earlier comment here #15129 (comment) since I’m assuming leaving data behind in mapscripts is wasteful but doesn’t cause real problems, but his comment seems to imply otherwise.

Also on Andrew’s other comment about the keypool, #15129 (comment), I think you could address it again with documentation just saying removeaddress only undoes the effects of importaddress, it doesn’t fully undo the effects of importmulti if the address was imported with other data like public keys.

in src/wallet/rpcdump.cpp:348 in b8eb588069

343+    if (is_redeem_script) {
344+        CScriptID scriptID = CScriptID(script);
345+        if (!spk_man.HaveCScript(scriptID)) {
346+            throw JSONRPCError(RPC_WALLET_ERROR, "Error removing p2sh redeemScript from wallet");
347+        }
348+        RemoveAddress(pwallet, ScriptHash(scriptID), purge_txns);

ryanofsky commented at 5:45 pm on September 1, 2021:

In commit “rpc: Added ability to remove watch only addresses” (b8eb5880693358951229512987beffcff51e0ecf)

Important: Again because this can throw it seems dangerous to call this here after PurgeWatchOnly has run and succeeded. Would be better to call PurgeWatchOnly after this so the RPC either completely fails or completely succeeds instead of partially succeeding.

ryanofsky commented at 5:55 pm on September 1, 2021: contributor

Code review almost-ack b8eb5880693358951229512987beffcff51e0ecf modulo my review comments marked “Important:” in the RemoveScript function, which should be easy to address with small changes.

DrahtBot commented at 1:06 pm on September 9, 2021: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

DrahtBot added the label Needs rebase on Sep 9, 2021

bitcoin deleted a comment on Sep 27, 2021

bitcoin locked this on Sep 27, 2021

bitcoin unlocked this on Sep 27, 2021

in src/wallet/wallet.cpp:1035 in b8eb588069

1030+{
1031+    LOCK(cs_wallet);
1032+
1033+    std::vector<std::unique_ptr<CWalletTx>> vWtx;
1034+    for (std::map<uint256, CWalletTx>::iterator it = mapWallet.begin(); it != mapWallet.end(); ++it) {
1035+        const CWalletTx& wTx = it->second;

luke-jr commented at 5:42 pm on October 20, 2021:

0    for (auto& [txid, wTx] : mapWallet) {

in src/wallet/wallet.cpp:1062 in b8eb588069

1057+            vWtx.push_back(std::make_unique<CWalletTx>(this, MakeTransactionRef(std::move(*wTx.tx))));
1058+        }
1059+    }
1060+
1061+    for (const auto& wtx : vWtx) {
1062+        mapWallet.erase(wtx->GetHash());

luke-jr commented at 5:45 pm on October 20, 2021:

At the very least, we need to delete it from wtxOrdered too - and probably mapTxSpends (see CWallet::ZapSelectTx - maybe we should abstract this to a new method). Ubuntu hirsute (newer libc?) crashes in the tests due to a use-after-free trying to iterate wtxOrdered after removeaddress; I’m not sure if there’s a good way to generalise this test to detect the problem on older systems.

Not sure why, but simply doing wtxOrdered.erase(wtx->m_it_wtxOrdered); doesn’t appear to fix it :/

luke-jr changes_requested

in test/functional/wallet_basic.py:496 in b8eb588069

492@@ -493,26 +493,90 @@ def run_test(self):
493                 assert_raises_rpc_error(-8, 'Invalid estimate_mode parameter, must be one of: "unset", "economical", "conservative"',
494                     self.nodes[2].sendtoaddress, address=address, amount=1, conf_target=target, estimate_mode=mode)
495 
496-            # 2. Import address from node2 to node1
497+            # Import address from node2 to node1

luke-jr commented at 6:27 pm on October 20, 2021:

This feels like we should have more self.log.info calls - is it still really about “sendtoaddress raises if an invalid fee_rate is passed” here?

darosior commented at 9:25 am on December 15, 2021: member

Can this be extended to descriptors?

DrahtBot commented at 1:07 pm on March 21, 2022: contributor

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

glozow commented at 10:01 am on September 26, 2022: member

Closing as this has needed rebase for more than 1 year. Feel free to reopen if you get a chance to work on this again in the future, thanks!

glozow closed this on Sep 26, 2022

kristapsk commented at 1:46 pm on September 26, 2022: contributor

@benthecarman You don’t plan to continue work on this? If not, I could take over.

benthecarman commented at 4:00 pm on September 26, 2022: contributor

@benthecarman You don’t plan to continue work on this? If not, I could take over.

Sorry I don’t have time to work on this at the moment. Feel free to take it over

bitcoin locked this on Sep 26, 2023

benthecarman deleted the branch on Feb 17, 2024

rpc: Added ability to remove watch only addresses #15129

Conflicts