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

Saibato · 2021-02-03T09:25:42Z

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 · 2021-02-03T10:07:46Z

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?)

Saibato · 2021-02-03T10:15:44Z

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.

ghost

is a reserved word

src/wallet/wallet.cpp

ghost

The wording wallets is reserved from use is better.

src/wallet/wallet.cpp

Saibato · 2021-02-04T00:12:59Z

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

CLICK ME

diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py
index 4bda73599..5a2ca8af3 100755
--- a/test/functional/test_framework/test_framework.py
+++ b/test/functional/test_framework/test_framework.py
@@ -403,7 +403,8 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
             n = self.nodes[i]
             if wallet_name is not None:
                 n.createwallet(wallet_name=wallet_name, descriptors=self.options.descriptors, load_on_startup=True)
-            n.importprivkey(privkey=n.get_deterministic_priv_key().key, label='coinbase')
+            if self.chain != "main":
+                n.importprivkey(privkey=n.get_deterministic_priv_key().key, label='coinbase')
 
     def run_test(self):
         """Tests must override this method to define test logic"""
diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py
index 123c48852..24806eab2 100644
--- a/test/functional/test_framework/util.py
+++ b/test/functional/test_framework/util.py
@@ -399,7 +399,10 @@ def get_auth_cookie(datadir, chain):
:
@@ -403,7 +403,8 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
             n = self.nodes[i]
             if wallet_name is not None:
                 n.createwallet(wallet_name=wallet_name, descriptors=self.options.descriptors, load_on_startup=True)
-            n.importprivkey(privkey=n.get_deterministic_priv_key().key, label='coinbase')
+            if self.chain != "main":
+                n.importprivkey(privkey=n.get_deterministic_priv_key().key, label='coinbase')
 
     def run_test(self):
         """Tests must override this method to define test logic"""
diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py
index 123c48852..24806eab2 100644
--- a/test/functional/test_framework/util.py
+++ b/test/functional/test_framework/util.py
@@ -399,7 +399,10 @@ def get_auth_cookie(datadir, chain):
                     assert password is None  # Ensure that there is only one rpcpassword line
                     password = line.split("=")[1].strip("\n")
     try:
-        with open(os.path.join(datadir, chain, ".cookie"), 'r', encoding="ascii") as f:
+        subdir = chain
+        if subdir == "main":
+            subdir = ""
+        with open(os.path.join(datadir, subdir, ".cookie"), 'r', encoding="ascii") as f:
             userpass = f.read()
             split_userpass = userpass.split(':')
             user = split_userpass[0]
@@ -413,7 +416,10 @@ def get_auth_cookie(datadir, chain):
 
 # If a cookie file exists in the given datadir, delete it.
 def delete_cookie_file(datadir, chain):
-    if os.path.isfile(os.path.join(datadir, chain, ".cookie")):
+    subdir = chain
+    if chain == "main":
+        subdir = ""
+    if os.path.isfile(os.path.join(datadir, subdir, ".cookie")):
         logger.debug("Deleting leftover cookie file")
         os.remove(os.path.join(datadir, chain, ".cookie"))
 
diff --git a/test/functional/wallet_mainnet_test.py b/test/functional/wallet_mainnet_test.py
new file mode 100755
index 000000000..cb78239fc
--- /dev/null
+++ b/test/functional/wallet_mainnet_test.py
@@ -0,0 +1,41 @@
+#!/usr/bin/env python3
+# Copyright (c) 2017-2019 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""Test if on mainnet wallets can vanish unaccesable.
+
+Verify that a bitcoind mainet node will not loose wallets on new startup
+"""
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import (
+    assert_equal,
+    assert_raises_rpc_error,
+)
+import collections
+
+class WalletStartupTest(BitcoinTestFramework):
+
+    def set_test_params(self):
+        self.chain = 'main'
+        self.num_nodes = 1
+        self.setup_clean_chain = True
+
+    def run_test(self):
+        self.log.info('test if we still have the original wallets key')
+        #self.nodes[0].createwallet("") //uncoment this line,  if u run the test against current master 
+        self.nodes[0].createwallet("w1")
+        self.nodes[0].createwallet("w2")
+        assert_equal(self.nodes[0].listwallets(), ['', 'w1', 'w2'])
+        self.log.info(self.nodes[0].listwallets())
+        n = self.nodes[0].get_wallet_rpc('')
+        address = n.getnewaddress()
+        self.nodes[0].createwallet("wallets")
+        n = self.nodes[0].get_wallet_rpc('')
+        self.log.info('now restart node')
+        self.restart_node(0)
+        self.log.info(self.nodes[0].listwallets())
+        n = self.nodes[0].get_wallet_rpc('')
+        #assert_raises_rpc_error(-4, " is not known", n.dumpprivkey, address)
+        n.dumpprivkey(address)
+
+if __name__ == '__main__':
+    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.

maflcko · 2021-02-04T07:37:15Z

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 · 2021-02-04T09:11:21Z

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 · 2021-02-06T11:08:57Z

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

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

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

DrahtBot · 2021-02-06T18:50:26Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

test: Add feature_taproot.py --previous_release #20354 (test: Add feature_taproot.py --previous_release by MarcoFalke)
External signer support - Wallet Box edition #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 · 2021-02-23T17:52:37Z

🐙 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 · 2021-12-15T11:21:14Z

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.

DrahtBot · 2022-03-21T13:06:50Z

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.

laanwj added the Wallet label Feb 3, 2021

ghost suggested changes Feb 3, 2021

View reviewed changes

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

ghost suggested changes Feb 3, 2021

View reviewed changes

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

Saibato force-pushed the sanitycheck branch 3 times, most recently from 8b669e1 to 04a0af9 Compare February 6, 2021 10:49

Saibato force-pushed the sanitycheck branch 2 times, most recently from de95270 to d7b5a50 Compare February 6, 2021 13:39

Saibato added 2 commits February 6, 2021 17:02

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

4ed3ae3

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

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

c8166e6

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

Saibato force-pushed the sanitycheck branch from d7b5a50 to c8166e6 Compare February 6, 2021 17:08

This was referenced Feb 6, 2021

test: Add feature_taproot.py --previous_release #20354

Closed

External signer support - Wallet Box edition #16546

Merged

DrahtBot added the Needs rebase label Feb 23, 2021

maflcko added the Up for grabs label Mar 21, 2022

fanquake closed this Mar 21, 2022

bitcoin locked and limited conversation to collaborators Mar 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

Saibato commented Feb 3, 2021 •

edited

Loading

Uh oh!

laanwj commented Feb 3, 2021 •

edited

Loading

Uh oh!

Saibato commented Feb 3, 2021 •

edited

Loading

Uh oh!

ghost left a comment

Uh oh!

Uh oh!

ghost left a comment

Uh oh!

Uh oh!

Saibato commented Feb 4, 2021 •

edited

Loading

Uh oh!

maflcko commented Feb 4, 2021

Uh oh!

Saibato commented Feb 4, 2021

Uh oh!

Saibato commented Feb 6, 2021 •

edited

Loading

Uh oh!

DrahtBot commented Feb 6, 2021 •

edited

Loading

Uh oh!

DrahtBot commented Feb 23, 2021

Uh oh!

DrahtBot commented Dec 15, 2021

Uh oh!

DrahtBot commented Mar 21, 2022

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

Saibato commented Feb 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Feb 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Saibato commented Feb 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

maflcko commented Feb 4, 2021

Uh oh!

Saibato commented Feb 4, 2021

Uh oh!

Saibato commented Feb 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Feb 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

DrahtBot commented Feb 23, 2021

Uh oh!

DrahtBot commented Dec 15, 2021

Uh oh!

DrahtBot commented Mar 21, 2022

Uh oh!

Uh oh!

Saibato commented Feb 3, 2021 •

edited

Loading

laanwj commented Feb 3, 2021 •

edited

Loading

Saibato commented Feb 3, 2021 •

edited

Loading

Saibato commented Feb 4, 2021 •

edited

Loading

Saibato commented Feb 6, 2021 •

edited

Loading

DrahtBot commented Feb 6, 2021 •

edited

Loading