wallet: check when create wallets for the reserved name "wallets"

Saibato commented at 9:25 am on February 3, 2021: contributor

We allow since 0.17 to create a wallet with the the name or dirprefix :“wallets”.

That might be not a good idea and we should discourage users to do that to prevent unexpected boating accidents and whale heart attacks.

edit@saibato The wallets subdir is not created when chain == ‘main’ by default to allow the node to support upgrading or using older walletdirs or datadirs created by older core releases, testing could not catch this since this effects only mannet nodes, -regtest behave different and there we don’t have this issue.

laanwj commented at 10:07 am on February 3, 2021: member

Please add some explanation, why is “wallets” a special name? Wallets are created, by default, in a directory named “wallets” but I don’t see how creating a sub-directory in that named wallet should be problematic?

(and if so, isn’t that a bug to be fixed and a functional test added for instead of worked around?)

laanwj added the label Wallet on Feb 3, 2021

Saibato commented at 10:15 am on February 3, 2021: contributor

Please add some explanation, why is “wallets” a special name? Wallets are created, by default, in a directory named “wallets” but I don’t see how creating a sub-directory in that named wallet should be problematic?

(and if so, isn’t that a bug to be fixed and a functional test added for instead of worked around?)

Sorry, but afaics u can not test this with regtest,signet or testnet that effects only mainnet nodes. edit@saibato If the node has no wallets dir i.e old and run with a new version or a fresh mainnet bitcoind or bitcoin-qt-datadir=somewhere node. ` the wallets subdir since 0.17 and above up to 0.21 is not created when chain == ‘main’ ‘by default and if u create a wallet named wallets on node restart u will loose access to older wallets.

in src/wallet/wallet.cpp:254 in 4504fabe10 outdated

248@@ -249,6 +249,13 @@ std::shared_ptr<CWallet> CreateWallet(interfaces::Chain& chain, const std::strin
249     uint64_t wallet_creation_flags = options.create_flags;
250     const SecureString& passphrase = options.create_passphrase;
251 
252+    // Sanity check bitcoin internal reserved
253+    if (std::memcmp(name.c_str(), "wallets", 7) == 0) {
254+        error = Untranslated("Please use a different walletname \"") + Untranslated(name) + Untranslated("\" - wallets is an reserved word.") + Untranslated(" ") + error;

unknown commented at 5:53 pm on February 3, 2021:

0        error = Untranslated("Please use a different walletname \"") + Untranslated(name) + Untranslated("\" - wallets is a reserved word.") + Untranslated(" ") + error;

unknown commented at 7:32 pm on February 3, 2021:

0        error = Untranslated("Please use a different walletname \"") + Untranslated(name) + Untranslated("\" - wallets is reserved from use.") + Untranslated(" ") + error;

unknown changes_requested

unknown commented at 5:53 pm on February 3, 2021: none

is a reserved word

unknown changes_requested

unknown commented at 7:35 pm on February 3, 2021: none

The wording wallets is reserved from use is better.

Saibato commented at 0:12 am on February 4, 2021: contributor

@laanwj here a patch to test also mainnet nodes and this issue with our framework, that could be adapted to a full functional test :-)

  0diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py
  1index 4bda73599..5a2ca8af3 100755
  2--- a/test/functional/test_framework/test_framework.py
  3+++ b/test/functional/test_framework/test_framework.py
  4@@ -403,7 +403,8 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
  5             n = self.nodes[i]
  6             if wallet_name is not None:
  7                 n.createwallet(wallet_name=wallet_name, descriptors=self.options.descriptors, load_on_startup=True)
  8-            n.importprivkey(privkey=n.get_deterministic_priv_key().key, label='coinbase')
  9+            if self.chain != "main":
 10+                n.importprivkey(privkey=n.get_deterministic_priv_key().key, label='coinbase')
 11 
 12     def run_test(self):
 13         """Tests must override this method to define test logic"""
 14diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py
 15index 123c48852..24806eab2 100644
 16--- a/test/functional/test_framework/util.py
 17+++ b/test/functional/test_framework/util.py
 18@@ -399,7 +399,10 @@ def get_auth_cookie(datadir, chain):
 19:
 20@@ -403,7 +403,8 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
 21             n = self.nodes[i]
 22             if wallet_name is not None:
 23                 n.createwallet(wallet_name=wallet_name, descriptors=self.options.descriptors, load_on_startup=True)
 24-            n.importprivkey(privkey=n.get_deterministic_priv_key().key, label='coinbase')
 25+            if self.chain != "main":
 26+                n.importprivkey(privkey=n.get_deterministic_priv_key().key, label='coinbase')
 27 
 28     def run_test(self):
 29         """Tests must override this method to define test logic"""
 30diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py
 31index 123c48852..24806eab2 100644
 32--- a/test/functional/test_framework/util.py
 33+++ b/test/functional/test_framework/util.py
 34@@ -399,7 +399,10 @@ def get_auth_cookie(datadir, chain):
 35                     assert password is None  # Ensure that there is only one rpcpassword line
 36                     password = line.split("=")[1].strip("\n")
 37     try:
 38-        with open(os.path.join(datadir, chain, ".cookie"), 'r', encoding="ascii") as f:
 39+        subdir = chain
 40+        if subdir == "main":
 41+            subdir = ""
 42+        with open(os.path.join(datadir, subdir, ".cookie"), 'r', encoding="ascii") as f:
 43             userpass = f.read()
 44             split_userpass = userpass.split(':')
 45             user = split_userpass[0]
 46@@ -413,7 +416,10 @@ def get_auth_cookie(datadir, chain):
 47 
 48 # If a cookie file exists in the given datadir, delete it.
 49 def delete_cookie_file(datadir, chain):
 50-    if os.path.isfile(os.path.join(datadir, chain, ".cookie")):
 51+    subdir = chain
 52+    if chain == "main":
 53+        subdir = ""
 54+    if os.path.isfile(os.path.join(datadir, subdir, ".cookie")):
 55         logger.debug("Deleting leftover cookie file")
 56         os.remove(os.path.join(datadir, chain, ".cookie"))
 57 
 58diff --git a/test/functional/wallet_mainnet_test.py b/test/functional/wallet_mainnet_test.py
 59new file mode 100755
 60index 000000000..cb78239fc
 61--- /dev/null
 62+++ b/test/functional/wallet_mainnet_test.py
 63@@ -0,0 +1,41 @@
 64+#!/usr/bin/env python3
 65+# Copyright (c) 2017-2019 The Bitcoin Core developers
 66+# Distributed under the MIT software license, see the accompanying
 67+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
 68+"""Test if on mainnet wallets can vanish unaccesable.
 69+
 70+Verify that a bitcoind mainet node will not loose wallets on new startup
 71+"""
 72+from test_framework.test_framework import BitcoinTestFramework
 73+from test_framework.util import (
 74+    assert_equal,
 75+    assert_raises_rpc_error,
 76+)
 77+import collections
 78+
 79+class WalletStartupTest(BitcoinTestFramework):
 80+
 81+    def set_test_params(self):
 82+        self.chain = 'main'
 83+        self.num_nodes = 1
 84+        self.setup_clean_chain = True
 85+
 86+    def run_test(self):
 87+        self.log.info('test if we still have the original wallets key')
 88+        #self.nodes[0].createwallet("") //uncoment this line,  if u run the test against current master 
 89+        self.nodes[0].createwallet("w1")
 90+        self.nodes[0].createwallet("w2")
 91+        assert_equal(self.nodes[0].listwallets(), ['', 'w1', 'w2'])
 92+        self.log.info(self.nodes[0].listwallets())
 93+        n = self.nodes[0].get_wallet_rpc('')
 94+        address = n.getnewaddress()
 95+        self.nodes[0].createwallet("wallets")
 96+        n = self.nodes[0].get_wallet_rpc('')
 97+        self.log.info('now restart node')
 98+        self.restart_node(0)
 99+        self.log.info(self.nodes[0].listwallets())
100+        n = self.nodes[0].get_wallet_rpc('')
101+        #assert_raises_rpc_error(-4, " is not known", n.dumpprivkey, address)
102+        n.dumpprivkey(address)
103+
104+if __name__ == '__main__':
105+    WalletStartupTest().main()

just patch and run test/functional/wallet_mainnet_test.py

That should raise on releases above 0.17 an error that there is no key for the wallet address. When the user creates a wallet named wallets and the node had no wallets subdir all older wallets will be inaccessible after next node restart. We should at least warn the users. And btw with this patch to test_framework/the different possible differences between mainnet and testchains can now be tested.

MarcoFalke commented at 7:37 am on February 4, 2021: member

This should be unrelated to the chain. As long as there is a wallet.dat in the net specific datadir, and the user creates another wallet wallets, that folder will be treated as the new wallets dir.

Saibato commented at 9:11 am on February 4, 2021: contributor

This should be unrelated to the chain

Should, yes, but… So i think now about to split his into two PR. A PR that enables by placing the cookie in the correct path and adding address pairs for real fund coins easy mainnet testing, maybe even fund the real fund addresses on the fly from a dev fund with expensive test dust?

It seams to me clear that testing i.g. only confirms the test to run correct. What happens if you use the real chain and funds could often be different and I wonder now what more can be found? hmm…

So this PR here is more a reminder fix and TODO.

Saibato force-pushed on Feb 6, 2021

Saibato commented at 11:08 am on February 6, 2021: contributor

@laanwj

(and if so, isn’t that a bug to be fixed and a functional test added for instead of worked around?)

I thought about that and but if we want to support seamless upgrade and use of older releases i see not much other way, considering that this effects also new sqlite wallets from newer releases, it seams to me the right way to just skip creating a wallets root subdir by chance.

Saibato force-pushed on Feb 6, 2021

[wallet]: Skip creating wallets that include the default_walletdir name.

bugfix. wallets is no longer the safe word ;-)

4ed3ae3f55

Allow self.chain = 'main' in python bitcoin test framework

mainchain and testchains could behave different.
So make sure we can now easy test this.

c8166e6802

Saibato force-pushed on Feb 6, 2021

DrahtBot commented at 6:50 pm on February 6, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20354 (test: Add feature_taproot.py –previous_release by MarcoFalke)
#16546 (External signer support - Wallet Box edition by Sjors)

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.

DrahtBot commented at 5:52 pm on February 23, 2021: member

🐙 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 Feb 23, 2021

DrahtBot commented at 11:21 am on December 15, 2021: member

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.

DrahtBot commented at 1:06 pm on March 21, 2022: member

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.

MarcoFalke added the label Up for grabs on Mar 21, 2022

fanquake closed this on Mar 21, 2022

DrahtBot locked this on Mar 21, 2023

wallet: check when create wallets for the reserved name “wallets” #21073

Conflicts