wallet: ensure wallet files are not reused across chains

rodentrabies commented at 11:20 am on April 7, 2020: contributor

This implements a proposal in #12805 and is a rebase of #14533.

This seems to be a working approach, but I’m not sure why the p2p_segwit.py functional test needed a change, so I’ll look into it more.

fanquake added the label Wallet on Apr 7, 2020

rodentrabies force-pushed on Apr 7, 2020

DrahtBot commented at 1:02 am on April 16, 2020: member

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

Conflicts

No conflicts as of last run.

in src/chain.cpp:64 in e1dc5b483c outdated

60@@ -61,7 +61,7 @@ const CBlockIndex *CChain::FindFork(const CBlockIndex *pindex) const {
61 
62 CBlockIndex* CChain::FindEarliestAtLeast(int64_t nTime, int height) const
63 {
64-    std::pair<int64_t, int> blockparams = std::make_pair(nTime, height);
65+2    std::pair<int64_t, int> blockparams = std::make_pair(nTime, height);

luke-jr commented at 4:19 am on April 23, 2020:

typo?

rodentrabies commented at 12:08 pm on April 23, 2020:

Yes. Fixed.

in src/wallet/init.cpp:64 in e1dc5b483c outdated

60@@ -61,6 +61,7 @@ void WalletInit::AddWalletOptions() const
61     gArgs.AddArg("-wallet=<path>", "Specify wallet database path. Can be specified multiple times to load multiple wallets. Path is interpreted relative to <walletdir> if it is not absolute, and will be created if it does not exist (as a directory containing a wallet.dat file and log files). For backwards compatibility this will also accept names of existing data files in <walletdir>.)", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::WALLET);
62     gArgs.AddArg("-walletbroadcast",  strprintf("Make the wallet broadcast transactions (default: %u)", DEFAULT_WALLETBROADCAST), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
63     gArgs.AddArg("-walletdir=<dir>", "Specify directory to hold wallets (default: <datadir>/wallets if it exists, otherwise <datadir>)", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::WALLET);
64+    gArgs.AddArg("-walletcrosschain", strprintf("Allow reusing wallet files across chains (default: %u)", DEFAULT_WALLETCROSSCHAIN), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);

luke-jr commented at 4:20 am on April 23, 2020:

Probably should be a debug option only?

rodentrabies commented at 12:07 pm on April 23, 2020:

I think it makes sense to allow users who know what they are doing to override this. This was suggested by @gmaxwell in #14533 discussion.

luke-jr commented at 4:02 pm on April 23, 2020:

Yes, debug options are for users who know what they are doing…

rodentrabies commented at 12:22 pm on April 24, 2020:

Understood. Should it also be in WALLET_DEBUG_TEST category?

rodentrabies force-pushed on Apr 23, 2020

rodentrabies force-pushed on Apr 24, 2020

DrahtBot added the label Needs rebase on May 1, 2020

rodentrabies force-pushed on May 1, 2020

DrahtBot removed the label Needs rebase on May 1, 2020

rodentrabies force-pushed on May 1, 2020

DrahtBot added the label Needs rebase on Jun 22, 2020

in src/wallet/wallet.cpp:4075 in 36bb866d45 outdated

3922+        {
3923+            // Wallet is assumed to be from another chain, if none of
3924+            // its last 6 best known blocks are in the active chain
3925+            // (this heavily relies on the fact that locator stores
3926+            // last 10 blocks consecutively):
3927+            const Optional<int> fork_height = chain.findLocatorFork(locator);

fanquake commented at 2:03 am on March 15, 2021:

Please use std::optional in new code.

rodentrabies commented at 11:09 am on March 15, 2021:

done

rodentrabies force-pushed on Mar 15, 2021

DrahtBot removed the label Needs rebase on Mar 15, 2021

adamjonas commented at 4:07 pm on March 19, 2021: member

Looks like the CI picked up a memory leak running wallet_crosschain.py:

0 node0 stderr =================================================================
1==39469==ERROR: LeakSanitizer: detected memory leaks
2Direct leak of 22096 byte(s) in 18 object(s) allocated from:
3    [#0](/bitcoin-bitcoin/0/) 0x556b3fed1e6d in malloc (/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/bitcoind+0x17b7e6d)
4    [#1](/bitcoin-bitcoin/1/) 0x7fd21ead48c4 in __os_malloc (/lib/x86_64-linux-gnu/libdb_cxx-5.3.so+0x1698c4)
5Indirect leak of 2016 byte(s) in 18 object(s) allocated from:
6    [#0](/bitcoin-bitcoin/0/) 0x556b3fed1e6d in malloc (/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/bitcoind+0x17b7e6d)
7    [#1](/bitcoin-bitcoin/1/) 0x7fd21ead48c4 in __os_malloc (/lib/x86_64-linux-gnu/libdb_cxx-5.3.so+0x1698c4)
8SUMMARY: AddressSanitizer: 24112 byte(s) leaked in 36 allocation(s).

rodentrabies force-pushed on Mar 20, 2021

DrahtBot added the label Needs rebase on Apr 13, 2021

rodentrabies force-pushed on Jun 15, 2021

DrahtBot removed the label Needs rebase on Jun 16, 2021

rodentrabies force-pushed on Jul 16, 2021

in src/wallet/wallet.cpp:2934 in d48ed7a7d1 outdated

2752+                error = _("Wallet files should not be reused across chains.");
2753+                return false;
2754+            }
2755+        }
2756+    }
2757+

rodentrabies commented at 0:24 am on July 17, 2021:

Had to move it here, since putting this after line 2766 where I placed it originally results in memory leaks, as detected by the sanitizer.

theStack commented at 11:25 am on July 18, 2021: member

Concept ACK

DrahtBot added the label Needs rebase on Jul 22, 2021

rodentrabies force-pushed on Aug 12, 2021

DrahtBot removed the label Needs rebase on Aug 12, 2021

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

This PR does not fix the issues that I was expecting it would. Have shared the details in https://github.com/bitcoin/bitcoin/issues/22792

in src/wallet/wallet.cpp:2761 in 37428c459a outdated

2756+            // Wallet is assumed to be from another chain, if none of
2757+            // its last 6 best known blocks are in the active chain
2758+            // (this heavily relies on the fact that locator stores
2759+            // last 10 blocks consecutively):
2760+            const std::optional<int> fork_height = chain.findLocatorFork(locator);
2761+            if (fork_height && std::distance(locator.vHave.begin(),

sipa commented at 7:14 pm on August 27, 2021:

This seems overly complicated. The last entry in the block locator is always the genesis block, so that can be used to compare to the expected chain’s genesis block hash.

michaelfolkson commented at 7:21 pm on August 27, 2021:

As discussed in the wallet meeting if we want to take into account altcoins that could share Bitcoin mainnet’s genesis block we’d need more than just a genesis block comparison. Also stated here: #14533 (comment)

S3RK commented at 7:48 pm on August 27, 2021:

For descriptor wallets the case with altcoins is already covered by storing network magic.

My vote goes for simpler code and genesis block hash check.

rodentrabies commented at 2:45 pm on September 2, 2021:

As I understand there is an agreement to restore the simpler implementation I used to have in #14533, right?

unknown commented at 9:27 pm on September 2, 2021:

@rodentrabies Yes

ghost commented at 7:58 pm on November 10, 2021: none

@rodentrabies Any update on this PR or is it up for grabs?

rodentrabies commented at 10:31 am on November 11, 2021: contributor

@prayank23, yeah, sorry about that, I’ll change it to the simpler implementation as discussed above this week end.

rodentrabies force-pushed on Nov 28, 2021

ghost commented at 8:36 am on December 17, 2021: none

I tested this today and tried to reproduce the issue with the steps mentioned in this issue: #22792

I was unable to reproduce the issue. However, I am getting a different error so I am not sure:

0error code: -18
1error message:
2Wallet file verification failed. Failed to load database path '/mnt/testnet3/T1'. Data is not in recognized format.

One test is failing in CI: wallet_crosschain.py
I am not sure if this is introduced in this PR. Getting an error when trying to run bitcoind -regtest=1 -datadir=/path

0EXCEPTION: St13runtime_error       
1Invalid combination of -regtest, -signet, -testnet and -chain. Can use at most one.       
2bitcoin in AppInit()       
3
4bitcoind: chainparamsbase.cpp:35: const CBaseChainParams& BaseParams(): Assertion `globalChainBaseParams' failed.

achow101 commented at 9:45 pm on December 20, 2021: member

I was unable to reproduce the issue. However, I am getting a different error so I am not sure:

If that is a descriptor wallet, then that would be because the sqlite implementation (only used for descriptor wallets) includes a crosschain protection mechanism already. However legacy wallets don’t, and this PR would allow for that.

One test is failing in CI: wallet_crosschain.py

Indeed. The test is incorrect because all regtest chains use the same genesis even if the rest of the blockchain is different. @rodentrabies You will need to change the test to start a node in testnet or mainnet mode (without syncing of course) in order to test the crosschain protection.

I am not sure if this is introduced in this PR. Getting an error when trying to run bitcoind -regtest=1 -datadir=/path

I don’t think that is related, you should double check your bitcoin.conf and make sure it does not specify a specific chain to use.

in test/functional/wallet_crosschain.py:27 in ece5cd1116 outdated

22+        self.setup_nodes()
23+
24+    def run_test(self):
25+        self.log.info("Generating initial blockchains")
26+        self.nodes[0].generate(5)
27+        self.nodes[1].generate(5)

achow101 commented at 9:53 pm on December 20, 2021:

In ece5cd111660005ce71cb8a7092f96c3e084d975 “tests: add tests for cross-chain wallet use prevention”

0        self.generate(self.nodes[0], 5, sync_fun=self.no_op)
1        self.generate(self.nodes[1], 5, sync_fun=self.no_op)

rodentrabies commented at 3:05 pm on February 15, 2022:

Removed generation completely, as it doesn’t seem necessary for the purposes of this test.

in test/functional/wallet_crosschain.py:43 in ece5cd1116 outdated

38+        node_2_wallet = os.path.join(self.nodes[2].datadir, 'node_2_wallet')
39+        self.nodes[2].createwallet(node_2_wallet)
40+        self.nodes[2].unloadwallet(node_2_wallet)
41+
42+        self.log.info("Loading wallets into nodes with a different genesis blocks")
43+        assert_raises_rpc_error(-4, 'Wallet files should not be reused across chains.', self.nodes[0].loadwallet, node_1_wallet)

achow101 commented at 9:54 pm on December 20, 2021:

In ece5cd111660005ce71cb8a7092f96c3e084d975 “tests: add tests for cross-chain wallet use prevention”

This assertion is currently not raised. The two wallets are detected as being on the same chain because all regtest nodes use the same genesis block regardless of what comes after.

ghost commented at 1:50 am on February 15, 2022: none

If that is a descriptor wallet, then that would be because the sqlite implementation (only used for descriptor wallets) includes a crosschain protection mechanism already. However legacy wallets don’t, and this PR would allow for that.

I am not sure if there was some misunderstanding. Not able to reproduce issue in PR branch was a good thing but error was confusing. @rodentrabies its been couple of years and issue is still not fixed. There were some suggestions by @achow101 as well. You tried your best and I appreciate it however I think this can lead to some kind of vulnerability if not fixed soon. Last update was 3 months ago. Is it okay if someone else can work on this pull request?

rodentrabies commented at 8:51 am on February 15, 2022: contributor

@prayank23 I’m sorry for the slow progress on this. I’ll work on wrapping this up today, if you don’t mind. In fact I’ve spent 2 hours this weekend trying to make the functional test start both regtest and testnet nodes for me by playing with self.chain, but without success, so I’ll just have to do that manually.

rodentrabies force-pushed on Feb 15, 2022

in src/wallet/wallet.cpp:2922 in 6c31b80d5d outdated

2917@@ -2918,6 +2918,23 @@ bool CWallet::AttachChain(const std::shared_ptr<CWallet>& walletInstance, interf
2918     assert(!walletInstance->m_chain || walletInstance->m_chain == &chain);
2919     walletInstance->m_chain = &chain;
2920 
2921+    // Unless allowed, ensure wallet files are not reused across chains:
2922+    if (!gArgs.GetBoolArg("-walletcrosschain", DEFAULT_WALLETCROSSCHAIN))

achow101 commented at 8:47 pm on February 15, 2022:

Can you add a test for this option?

rodentrabies commented at 8:18 am on February 16, 2022:

Done.

MarcoFalke commented at 12:38 pm on February 16, 2022:

0    if (!gArgs.GetBoolArg("-walletcrosschain", DEFAULT_WALLETCROSSCHAIN)) {

nit: no newline after if

rodentrabies commented at 1:06 pm on February 16, 2022:

Done. Followed the style of several ifs on the lines below, but I see now that those are outliers.

rodentrabies force-pushed on Feb 16, 2022

ghost commented at 11:08 am on February 16, 2022: none

Tried testing different things. Two tests worked as expected and 1 failed.

✅ Test 1 :

Run bitcoind with PR branch
Create a testnet wallet T1 with bitcoin-cli createwallet T1. This creates a descriptor wallet as shared by @achow101 in #18554 (comment) which I understood now after looking at PR #23002 and reading everything again.
Try to load this wallet when running bitcoind for mainnet. This already has a protection so gives a different error: Wallet file verification failed. Failed to load database path '/home/prayank/.bitcoin/testnet3/wallets/T1'. Data is not in recognized format.

✅ Test 2:

Run bitcoind with PR branch
Create a testnet wallet T2 with bitcoin-cli -named createwallet -wallet_name="T2" -descriptors=false. This creates a legacy wallet.
Try to load this wallet when running bitcoind for mainnet. This gives an error: Wallet loading failed. Wallet files should not be reused across chains.

❌ Test 3:

Run bitcoind with v22.0 binary
Create a testnet wallet T3 with bitcoin-cli createwallet T3.
Try to load this wallet in bitcoind (PR branch - mainnet) and it works:

0$ bitcoin-cli loadwallet "/home/prayank/.bitcoin/testnet3/wallets/T3"
1{
2  "name": "/home/prayank/.bitcoin/testnet3/wallets/T3",
3  "warning": ""
4}

rodentrabies commented at 11:55 am on February 16, 2022: contributor

Just tried this (test 3) and got expected behavior:

 0$ ./bitcoin-22.0/bin/bitcoind -testnet -maxconnections=0
 1...
 2$ ./bitcoin-22.0/bin/bitcoind-cli -testnet createwallet t3
 3...
 4# stop bitcoind
 5...
 6$ ./bitcoin/src/bitcoind -maxconnections=0
 7...
 8$ ./bitcoin/src/bitcoin-cli loadwallet ~/.bitcoin/testnet3/wallets/t3
 9error code: -4
10error message:
11Wallet loading failed. Wallet files should not be reused across chains.

Perhaps maxconnections somehow affects the condition batch.ReadBestBlock(locator) && locator.vHave.size() > 0 && chain.getHeight() at wallet.cpp:2926. I’ll investigate.

EDIT: just tried the same without maxconnections (though before block header synchronization finishes on either of the node instances) and still see the expected behavior.

in src/wallet/wallet.cpp:2932 in b933b805b9 outdated

2927+        {
2928+            // Wallet is assumed to be from another chain, if genesis block in the active
2929+            // chain is differs from the genesis block know to the wallet.
2930+            if (chain.getBlockHash(0) != locator.vHave.back())
2931+            {
2932+                error = _("Wallet files should not be reused across chains.");

MarcoFalke commented at 12:38 pm on February 16, 2022:

should this mention the setting to disable the check? Also, could go with Untranslated()? It is not like this is something that happens in everyday operation?

rodentrabies commented at 1:07 pm on February 16, 2022:

Added a sentence mentioning -walletcrosschain and wrapped with Untranslated(...).

in src/wallet/wallet.cpp:2929 in b933b805b9 outdated

2924+        WalletBatch batch(walletInstance->GetDatabase());
2925+        CBlockLocator locator;
2926+        if (batch.ReadBestBlock(locator) && locator.vHave.size() > 0 && chain.getHeight())
2927+        {
2928+            // Wallet is assumed to be from another chain, if genesis block in the active
2929+            // chain is differs from the genesis block know to the wallet.

MarcoFalke commented at 12:40 pm on February 16, 2022:

0            // chain differs from the genesis block known to the wallet.

rodentrabies commented at 1:06 pm on February 16, 2022:

Done.

wallet: ensure wallet files are not reused across chains 968765973b

tests: add tests for cross-chain wallet use prevention 5f213213cb

rodentrabies force-pushed on Feb 16, 2022

achow101 commented at 5:59 pm on February 16, 2022: member

ACK 5f213213cb17429353ef7ec3e97b185af06d236f

ghost commented at 8:46 pm on February 16, 2022: none

Just tried this (test 3) and got expected behavior:

Even I could not reproduce it and getting cross chain error now for a new wallet. Old wallet still loads in mainnet that was created on testnet. I have added the wallet files if that helps: https://github.com/prayank23/bitcoin/blob/test-wallets/T2.zip

achow101 commented at 9:38 pm on February 16, 2022: member

Just tried this (test 3) and got expected behavior:

Even I could not reproduce it and getting cross chain error now for a new wallet. Old wallet still loads in mainnet that was created on testnet. I have added the wallet files if that helps: https://github.com/prayank23/bitcoin/blob/test-wallets/T2.zip

Are you sure you didn’t just make a mistake when doing this test? The provided wallet file has a mainnet block locator record and it doesn’t load in testnet with this PR.

ghost commented at 10:23 pm on February 16, 2022: none

Are you sure you didn’t just make a mistake when doing this test? The provided wallet file has a mainnet block locator record and it doesn’t load in testnet with this PR.

Right. Looks like I did something wrong. I think this PR fixes the issue and if I find anything else it can be done in a follow up PR. Its weird that this wallet T2 exists with other testnet wallets in testnet3 directory.

unknown approved

unknown commented at 10:23 pm on February 16, 2022: none

tACK https://github.com/bitcoin/bitcoin/pull/18554/commits/5f213213cb17429353ef7ec3e97b185af06d236f

dongcarl commented at 7:48 pm on April 28, 2022: member

Code Review ACK 5f213213cb17429353ef7ec3e97b185af06d236f

Apropos of nothing: @rodentrabies has quite the fortitude, carrying this PR since 2018

achow101 merged this on Apr 28, 2022

achow101 closed this on Apr 28, 2022

rodentrabies commented at 9:11 am on April 29, 2022: contributor

@dongcarl to be frank, I didn’t do a very good job of carrying it. Had to be reminded several times to rebase/update it :) Great it’s finally merged!

rodentrabies deleted the branch on Apr 29, 2022

sidhujag referenced this in commit 64fa7c2c9c on Apr 29, 2022

DrahtBot locked this on Apr 29, 2023

wallet: ensure wallet files are not reused across chains #18554

Conflicts