rpc: Added ability to remove watch only addresses #15129

benthecarman · 2019-01-08T14:41:50Z

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

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

promag

Please add some tests.

src/wallet/wallet.cpp

src/wallet/rpcdump.cpp

src/wallet/wallet.cpp

jonasschnelli · 2019-01-08T23:36:50Z

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

DrahtBot · 2019-01-09T22:47:31Z

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 · 2019-01-12T00:08:08Z

Added functionality to remove scripts as well

jonasschnelli · 2019-01-12T00:15:29Z

Looks much better!
Will test soon.

hebasto · 2019-01-16T20:48:14Z

@benthecarman could you address Travis linter failure?

benthecarman · 2019-01-17T19:52:51Z

Added release notes

benthecarman · 2019-02-04T11:42:31Z

Rebased and fixed release notes

hebasto · 2019-02-04T11:55:25Z

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

benthecarman · 2019-02-04T12:15:57Z

@hebasto Sorry about that, fixed

Sjors

Concept ACK

src/wallet/rpcdump.cpp

src/wallet/wallet.cpp

kristapsk · 2021-08-24T11:57:07Z

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

benthecarman · 2021-08-24T15:20:47Z

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

Added

kristapsk

ACK 2eb5021

ghost · 2021-08-24T16:51:57Z

reACK 2eb5021

meshcollider · 2021-08-25T03:30:02Z

src/wallet/rpcdump.cpp

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


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

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

meshcollider · 2021-08-25T03:41:19Z

src/wallet/rpcdump.cpp

+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue {
+            std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);
+            if (!wallet) return NullUniValue;
+            CWallet& pwallet = *wallet.get();


Why not just use the shared_ptr everywhere?

I think I fixed this

meshcollider · 2021-08-25T04:17:38Z

test/functional/wallet_basic.py

@@ -504,15 +504,58 @@ def run_test(self):
                                {"address": address_to_import},
                                {"spendable": False})

-            # 5. Import private key of the previously imported address on node1
+            # 5. Remove the address from node1


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

Removed (this was very annoying)

meshcollider · 2021-08-25T04:20:12Z

test/functional/wallet_basic.py

-            # 5. Import private key of the previously imported address on node1
+            # 5. Remove the address from node1
+            self.nodes[1].removeaddress(address_to_import, False, False)
+


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

meshcollider · 2021-08-25T04:22:05Z

test/functional/wallet_basic.py

+            # Check the address was removed and other address is unaffected
+            assert not self.nodes[1].getaddressinfo(address_to_import2)["iswatchonly"]
+            assert self.nodes[1].getaddressinfo(address_to_import)["ismine"]
+


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

meshcollider · 2021-08-25T04:23:12Z

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

benthecarman · 2021-08-31T01:24:30Z

Responded to @meshcollider's review

ryanofsky

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

ryanofsky · 2021-09-01T16:46:44Z

src/wallet/wallet.cpp

+            }
+        }
+
+        if (txContainsScript && !IsFromMe(*wTx.tx) && !IsMine(*wTx.tx)) {


In commit "rpc: Added ability to remove watch only addresses" (b8eb588)

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.

ryanofsky · 2021-09-01T17:08:38Z

src/wallet/rpcdump.cpp

+        "\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",
+        {
+            {"address", RPCArg::Type::STR, RPCArg::Optional::NO, "The Bitcoin address (or hex-encoded script)"},
+            {"p2sh", RPCArg::Type::BOOL, RPCArg::Default{false}, "Whether to remove the P2SH version of the script as well"},


In commit "rpc: Added ability to remove watch only addresses" (b8eb588)

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."

ryanofsky · 2021-09-01T17:20:23Z

src/wallet/rpcdump.cpp

+            {"p2sh", RPCArg::Type::BOOL, /* default */ "false", "Remove the P2SH version of the script as well"},
+            {"purgetransactions", RPCArg::Type::BOOL, /* default_val */ "false", "Remove existing transactions from the wallet"},
+        },
+        RPCResult{RPCResult::Type::NONE, "", ""},


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.

ryanofsky · 2021-09-01T17:24:36Z

src/wallet/rpcdump.cpp

+    if (is_redeem_script) {
+        CScriptID scriptID = CScriptID(script);
+        if (!spk_man.HaveCScript(scriptID)) {
+            throw JSONRPCError(RPC_WALLET_ERROR, "Error removing p2sh redeemScript from wallet");


In commit "rpc: Added ability to remove watch only addresses" (b8eb588)

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.

ryanofsky · 2021-09-01T17:43:51Z

src/wallet/rpcdump.cpp

+    }
+    if (is_redeem_script) {
+        CScriptID scriptID = CScriptID(script);
+        if (!spk_man.HaveCScript(scriptID)) {


In commit "rpc: Added ability to remove watch only addresses" (b8eb588)

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.

re: #15129 (comment)

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.

ryanofsky · 2021-09-01T17:45:25Z

src/wallet/rpcdump.cpp

+        if (!spk_man.HaveCScript(scriptID)) {
+            throw JSONRPCError(RPC_WALLET_ERROR, "Error removing p2sh redeemScript from wallet");
+        }
+        RemoveAddress(pwallet, ScriptHash(scriptID), purge_txns);


In commit "rpc: Added ability to remove watch only addresses" (b8eb588)

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.

DrahtBot · 2021-09-09T13:06:45Z

🐙 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".}

luke-jr · 2021-10-20T17:42:40Z

src/wallet/wallet.cpp

+    for (std::map<uint256, CWalletTx>::iterator it = mapWallet.begin(); it != mapWallet.end(); ++it) {
+        const CWalletTx& wTx = it->second;


Suggested change

for (std::map<uint256, CWalletTx>::iterator it = mapWallet.begin(); it != mapWallet.end(); ++it) {

const CWalletTx& wTx = it->second;

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

luke-jr · 2021-10-20T17:45:16Z

src/wallet/wallet.cpp

+    }
+
+    for (const auto& wtx : vWtx) {
+        mapWallet.erase(wtx->GetHash());


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 · 2021-10-20T18:27:38Z

test/functional/wallet_basic.py

@@ -493,26 +493,90 @@ def run_test(self):
                assert_raises_rpc_error(-8, 'Invalid estimate_mode parameter, must be one of: "unset", "economical", "conservative"',
                    self.nodes[2].sendtoaddress, address=address, amount=1, conf_target=target, estimate_mode=mode)

-            # 2. Import address from node2 to node1
+            # Import address from node2 to node1


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 · 2021-12-15T09:25:43Z

Can this be extended to descriptors?

DrahtBot · 2022-03-21T13:07:55Z

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

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 · 2022-09-26T10:01:49Z

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!

kristapsk · 2022-09-26T13:46:28Z

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

benthecarman · 2022-09-26T16:00:24Z

@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

fanquake added Wallet RPC/REST/ZMQ labels Jan 8, 2019

promag reviewed Jan 8, 2019

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

src/wallet/rpcdump.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

benthecarman force-pushed the remove_watch_only_address branch 2 times, most recently from ba80d4f to 685f00f Compare January 8, 2019 16:25

benthecarman mentioned this pull request Jan 8, 2019

wallet: Added function to remove transactions that are no longer in wallet #15130

Closed

benthecarman force-pushed the remove_watch_only_address branch 4 times, most recently from aeffaea to a3a1233 Compare January 8, 2019 22:42

jonasschnelli reviewed Jan 8, 2019

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

benthecarman force-pushed the remove_watch_only_address branch 3 times, most recently from 0f6e428 to f49c523 Compare January 12, 2019 00:07

benthecarman force-pushed the remove_watch_only_address branch 2 times, most recently from 7533c58 to 4c4a43f Compare January 17, 2019 19:52

DrahtBot added the Needs rebase label Jan 30, 2019

benthecarman force-pushed the remove_watch_only_address branch from 4c4a43f to 0e4483e Compare February 4, 2019 11:41

DrahtBot removed the Needs rebase label Feb 4, 2019

benthecarman force-pushed the remove_watch_only_address branch from 0e4483e to 4c7d2b6 Compare February 4, 2019 12:13

Sjors reviewed Aug 15, 2019

View reviewed changes

src/wallet/rpcdump.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

benthecarman force-pushed the remove_watch_only_address branch from 723aed1 to 2eb5021 Compare August 24, 2021 15:20

kristapsk approved these changes Aug 24, 2021

View reviewed changes

meshcollider reviewed Aug 25, 2021

View reviewed changes

This was referenced Aug 25, 2021

scripted-diff: Use generate* from TestFramework #22788

Merged

test: Implicitly sync after generate* to preempt races and intermittent test failures #22567

Closed

rpc: Added ability to remove watch only addresses

b8eb588

benthecarman force-pushed the remove_watch_only_address branch from 2eb5021 to b8eb588 Compare August 31, 2021 01:23

ryanofsky reviewed Sep 1, 2021

View reviewed changes

DrahtBot mentioned this pull request Sep 3, 2021

Add Single Random Draw as an additional coin selection algorithm #17526

Merged

DrahtBot added the Needs rebase label Sep 9, 2021

bitcoin deleted a comment from ladykevs Sep 27, 2021

bitcoin locked and limited conversation to collaborators Sep 27, 2021

bitcoin unlocked this conversation Sep 27, 2021

luke-jr suggested changes Oct 20, 2021

View reviewed changes

luke-jr reviewed Oct 20, 2021

View reviewed changes

ghost mentioned this pull request Dec 13, 2021

add ability to remove imported public keys/addresses (or even privkeys) #23765

Open

glozow closed this Sep 26, 2022

bitcoin locked and limited conversation to collaborators Sep 26, 2023

benthecarman deleted the remove_watch_only_address branch February 17, 2024 14:28

		for (std::map<uint256, CWalletTx>::iterator it = mapWallet.begin(); it != mapWallet.end(); ++it) {
		const CWalletTx& wTx = it->second;

	for (std::map<uint256, CWalletTx>::iterator it = mapWallet.begin(); it != mapWallet.end(); ++it) {
	const CWalletTx& wTx = it->second;
	for (auto& [txid, wTx] : mapWallet) {

rpc: Added ability to remove watch only addresses #15129

rpc: Added ability to remove watch only addresses #15129

Uh oh!

Conversation

benthecarman commented Jan 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonasschnelli commented Jan 8, 2019

Uh oh!

DrahtBot commented Jan 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

benthecarman commented Jan 12, 2019

Uh oh!

jonasschnelli commented Jan 12, 2019

Uh oh!

hebasto commented Jan 16, 2019

Uh oh!

benthecarman commented Jan 17, 2019

Uh oh!

benthecarman commented Feb 4, 2019

Uh oh!

hebasto commented Feb 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benthecarman commented Feb 4, 2019

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kristapsk commented Aug 24, 2021

Uh oh!

benthecarman commented Aug 24, 2021

Uh oh!

kristapsk left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Aug 24, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meshcollider commented Aug 25, 2021

Uh oh!

benthecarman commented Aug 31, 2021

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Sep 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benthecarman commented Jan 8, 2019 •

edited

Loading

DrahtBot commented Jan 9, 2019 •

edited

Loading

hebasto commented Feb 4, 2019 •

edited

Loading

ryanofsky Sep 1, 2021 •

edited

Loading

luke-jr Oct 20, 2021 •

edited

Loading