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

rodentrabies · 2020-04-07T11:20:04Z

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.

DrahtBot · 2020-04-16T01:02:26Z

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

Conflicts

No conflicts as of last run.

src/chain.cpp

luke-jr · 2020-04-23T04:20:11Z

src/wallet/init.cpp

@@ -61,6 +61,7 @@ void WalletInit::AddWalletOptions() const
    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);
    gArgs.AddArg("-walletbroadcast",  strprintf("Make the wallet broadcast transactions (default: %u)", DEFAULT_WALLETBROADCAST), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
    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);
+    gArgs.AddArg("-walletcrosschain", strprintf("Allow reusing wallet files across chains (default: %u)", DEFAULT_WALLETCROSSCHAIN), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);


Probably should be a debug option only?

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.

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

Understood. Should it also be in WALLET_DEBUG_TEST category?

src/wallet/wallet.cpp

adamjonas · 2021-03-19T16:07:22Z

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

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

rodentrabies · 2021-07-17T00:24:08Z

src/wallet/wallet.cpp

+            }
+        }
+    }
+


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.

ghost · 2021-12-17T08:36:27Z

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:

error code: -18
error message:
Wallet 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

EXCEPTION: St13runtime_error       
Invalid combination of -regtest, -signet, -testnet and -chain. Can use at most one.       
bitcoin in AppInit()       

bitcoind: chainparamsbase.cpp:35: const CBaseChainParams& BaseParams(): Assertion `globalChainBaseParams' failed.

achow101 · 2021-12-20T21:45:49Z

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.

test/functional/wallet_crosschain.py

ghost · 2022-02-15T01:50:22Z

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 · 2022-02-15T08:51:41Z

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

src/wallet/wallet.cpp

ghost · 2022-02-16T11:08:27Z

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 wallet: ensure wallet files are not reused across chains #18554 (comment) which I understood now after looking at PR Make descriptor wallets by default #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:

$ bitcoin-cli loadwallet "/home/prayank/.bitcoin/testnet3/wallets/T3"
{
  "name": "/home/prayank/.bitcoin/testnet3/wallets/T3",
  "warning": ""
}

rodentrabies · 2022-02-16T11:55:39Z

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

$ ./bitcoin-22.0/bin/bitcoind -testnet -maxconnections=0
...
$ ./bitcoin-22.0/bin/bitcoind-cli -testnet createwallet t3
...
# stop bitcoind
...
$ ./bitcoin/src/bitcoind -maxconnections=0
...
$ ./bitcoin/src/bitcoin-cli loadwallet ~/.bitcoin/testnet3/wallets/t3
error code: -4
error message:
Wallet 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.

src/wallet/wallet.cpp

achow101 · 2022-02-16T17:59:38Z

ACK 5f21321

ghost · 2022-02-16T20:46:37Z

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 · 2022-02-16T21:38:45Z

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 · 2022-02-16T22:23:20Z

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.

ghost

tACK 5f21321

dongcarl · 2022-04-28T19:48:54Z

Code Review ACK 5f21321

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

rodentrabies · 2022-04-29T09:11:40Z

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

…s chains 5f21321 tests: add tests for cross-chain wallet use prevention (Seibart Nedor) 9687659 wallet: ensure wallet files are not reused across chains (Seibart Nedor) Pull request description: This implements a proposal in bitcoin#12805 and is a rebase of bitcoin#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. ACKs for top commit: achow101: ACK 5f21321 dongcarl: Code Review ACK 5f21321 [deleted]: tACK bitcoin@5f21321 Tree-SHA512: 2c934300f113e772fc31c16ef5588526300bbc36e4dcef7d77bd0760c5c8f0ec77f766b1bed5503eb0157fa26dc900ed54d2ad1b41863c1f736ce5c1f3b67bec

fanquake added the Wallet label Apr 7, 2020

rodentrabies force-pushed the wallet-file-reuse-prevention branch from 9b93678 to e1dc5b4 Compare April 7, 2020 11:26

This was referenced Apr 16, 2020

Wallet passive startup #15719

Closed

Reverse cs_main, cs_wallet lock order and reduce cs_main locking #16426

Merged

luke-jr reviewed Apr 23, 2020

View reviewed changes

src/chain.cpp Outdated Show resolved Hide resolved

luke-jr reviewed Apr 23, 2020

View reviewed changes

rodentrabies force-pushed the wallet-file-reuse-prevention branch 2 times, most recently from 3ce1335 to 0f02b2c Compare April 24, 2020 14:33

DrahtBot added the Needs rebase label May 1, 2020

rodentrabies force-pushed the wallet-file-reuse-prevention branch from 0f02b2c to 0ae2e5c Compare May 1, 2020 20:25

DrahtBot removed the Needs rebase label May 1, 2020

rodentrabies force-pushed the wallet-file-reuse-prevention branch from 0ae2e5c to 36bb866 Compare May 1, 2020 22:19

DrahtBot mentioned this pull request Jun 22, 2020

test: Refactor tests using restart_node #19350

Merged

DrahtBot added the Needs rebase label Jun 22, 2020

fanquake reviewed Mar 15, 2021

View reviewed changes

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

rodentrabies force-pushed the wallet-file-reuse-prevention branch from 36bb866 to 5030801 Compare March 15, 2021 11:08

DrahtBot removed the Needs rebase label Mar 15, 2021

DrahtBot mentioned this pull request Mar 15, 2021

Remove RewindBlockIndex logic #21009

Merged

rodentrabies force-pushed the wallet-file-reuse-prevention branch 2 times, most recently from 51b4f8c to 2844377 Compare March 20, 2021 00:29

DrahtBot mentioned this pull request Apr 8, 2021

tests: Skip SQLite fsyncs while testing #21634

Merged

DrahtBot added the Needs rebase label Apr 13, 2021

rodentrabies force-pushed the wallet-file-reuse-prevention branch from 2844377 to 4eb1434 Compare June 15, 2021 23:13

DrahtBot removed the Needs rebase label Jun 16, 2021

DrahtBot mentioned this pull request Jun 16, 2021

Default to NODE_WITNESS in nLocalServices #21090

Merged

rodentrabies force-pushed the wallet-file-reuse-prevention branch 2 times, most recently from 50c8e2b to d48ed7a Compare July 16, 2021 22:26

rodentrabies commented Jul 17, 2021

View reviewed changes

rodentrabies force-pushed the wallet-file-reuse-prevention branch from 3188695 to ece5cd1 Compare November 28, 2021 20:24

achow101 reviewed Dec 20, 2021

View reviewed changes

test/functional/wallet_crosschain.py Outdated Show resolved Hide resolved

test/functional/wallet_crosschain.py Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Jan 6, 2022

wallet: add config to prioritize a solution that doesn't create change in coin selection #23475

Closed

rodentrabies mentioned this pull request Feb 15, 2022

build: compilation error in tinyformat.h on Manjaro Linux #24351

Closed

rodentrabies force-pushed the wallet-file-reuse-prevention branch 2 times, most recently from d93f366 to 6c31b80 Compare February 15, 2022 18:11

achow101 reviewed Feb 15, 2022

View reviewed changes

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

rodentrabies force-pushed the wallet-file-reuse-prevention branch from 6c31b80 to b933b80 Compare February 16, 2022 08:18

maflcko reviewed Feb 16, 2022

View reviewed changes

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

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

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

rodentrabies added 2 commits February 16, 2022 15:02

wallet: ensure wallet files are not reused across chains

9687659

tests: add tests for cross-chain wallet use prevention

5f21321

rodentrabies force-pushed the wallet-file-reuse-prevention branch from b933b80 to 5f21321 Compare February 16, 2022 13:06

ghost approved these changes Feb 16, 2022

View reviewed changes

achow101 merged commit 606ce05 into bitcoin:master Apr 28, 2022

prusnak mentioned this pull request Apr 28, 2022

Wallet file should not be reused across chains #12805

Closed

rodentrabies deleted the wallet-file-reuse-prevention branch April 29, 2022 09:11

bitcoin locked and limited conversation to collaborators Apr 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rodentrabies commented Apr 7, 2020

DrahtBot commented Apr 16, 2020 •

edited

Loading

luke-jr Apr 23, 2020

rodentrabies Apr 23, 2020

luke-jr Apr 23, 2020

rodentrabies Apr 24, 2020

adamjonas commented Mar 19, 2021

rodentrabies Jul 17, 2021

ghost commented Dec 17, 2021

achow101 commented Dec 20, 2021

ghost commented Feb 15, 2022 •

edited by ghost

Loading

rodentrabies commented Feb 15, 2022

ghost commented Feb 16, 2022 •

edited by ghost

Loading

rodentrabies commented Feb 16, 2022 •

edited

Loading

achow101 commented Feb 16, 2022

ghost commented Feb 16, 2022 •

edited by ghost

Loading

achow101 commented Feb 16, 2022

ghost commented Feb 16, 2022

ghost left a comment

dongcarl commented Apr 28, 2022

rodentrabies commented Apr 29, 2022

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

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

Conversation

rodentrabies commented Apr 7, 2020

DrahtBot commented Apr 16, 2020 • edited Loading

Conflicts

luke-jr Apr 23, 2020

Choose a reason for hiding this comment

rodentrabies Apr 23, 2020

Choose a reason for hiding this comment

luke-jr Apr 23, 2020

Choose a reason for hiding this comment

rodentrabies Apr 24, 2020

Choose a reason for hiding this comment

adamjonas commented Mar 19, 2021

rodentrabies Jul 17, 2021

Choose a reason for hiding this comment

ghost commented Dec 17, 2021

achow101 commented Dec 20, 2021

ghost commented Feb 15, 2022 • edited by ghost Loading

rodentrabies commented Feb 15, 2022

ghost commented Feb 16, 2022 • edited by ghost Loading

rodentrabies commented Feb 16, 2022 • edited Loading

achow101 commented Feb 16, 2022

ghost commented Feb 16, 2022 • edited by ghost Loading

achow101 commented Feb 16, 2022

ghost commented Feb 16, 2022

ghost left a comment

Choose a reason for hiding this comment

dongcarl commented Apr 28, 2022

rodentrabies commented Apr 29, 2022

DrahtBot commented Apr 16, 2020 •

edited

Loading

ghost commented Feb 15, 2022 •

edited by ghost

Loading

ghost commented Feb 16, 2022 •

edited by ghost

Loading

rodentrabies commented Feb 16, 2022 •

edited

Loading

ghost commented Feb 16, 2022 •

edited by ghost

Loading