[WIP] wallet: ensure wallet files are not reused across chains

rodentrabies commented at 7:54 pm on October 20, 2018: contributor

This implements a proposal in #12805.

I follow the suggestion by @ryanofsky and compare a genesis block hash, extracted from wallet’s best block locator, to an chainActive’s genesis block hash. If they don’t match, the error is reported.

fanquake added the label Wallet on Oct 20, 2018

DrahtBot commented at 3:46 am on October 21, 2018: 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:

#15443 (qa: Add getdescriptorinfo functional test by promag)
#13756 (wallet: “avoid_reuse” wallet flag for improved privacy by kallewoof)

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.

gmaxwell commented at 7:16 am on October 21, 2018: contributor

Genesis block isn’t sufficient, a lot of altcoins share genesis blocks. :( Also there should be a way to override, since moving wallet files between forks is a common way of collecting fork coins– obviously not something you’d want to happen accidentally but fine as an unsupported “you get to keep both pieces” option.

rodentrabies force-pushed on Oct 21, 2018

rodentrabies commented at 11:08 am on October 21, 2018: contributor

@gmaxwell, thanks for the suggestion. Just added an override option -allowreusewallets (maybe should be renamed to something clearer). Still working on sufficiency issue. First of all, since CChain::GetLocator builds a vHave sequence deterministically, maybe the whole vectors can be compared, but a lot of forked altcoins share those blocks as well, so I’m looking for an alternative.

in src/wallet/wallet.cpp:4099 in da99bd1315 outdated

4094@@ -4095,13 +4095,28 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(const std::string& name,
4095 
4096     LOCK(cs_main);
4097 
4098-    CBlockIndex *pindexRescan = chainActive.Genesis();
4099-    if (!gArgs.GetBoolArg("-rescan", false))
4100+    CBlockIndex* activeChainGenesis = chainActive.Genesis();
4101+    WalletBatch batch(*walletInstance->database);

practicalswift commented at 7:16 am on October 22, 2018:

Please use another variable name: batch is already used in this context :-)

rodentrabies commented at 5:56 pm on October 24, 2018:

It’s done on purpose in order to reuse the value, computed by it (bestBlockRead) both here and in -rescan check.

rodentrabies force-pushed on Oct 27, 2018

rodentrabies commented at 10:33 pm on October 27, 2018: contributor

Implemented the new check via FindForkInGlobalIndex. This solution looks at all blocks known to the wallet and not only at a genesis. It may misbehave in case of temporary forks (reorgs), but there seems to be no simple way around that.

This can be tested with the following function test (here two disconnected nodes in regtest mode generate two different chains with the same genesis, so can be used to model fork coins with shared genesis):

 0        # ...
 1        self.log.info("Generating initial distinct blockchains")
 2        self.nodes[0].generate(5)
 3        self.nodes[1].generate(5)
 4
 5        self.log.info("Creating wallets")
 6        w0_path = os.path.join(self.nodes[0].datadir, 'w0')
 7        w1_path = os.path.join(self.nodes[1].datadir, 'w1')
 8        self.nodes[0].createwallet(w0_path)
 9        self.nodes[1].createwallet(w1_path)
10
11        self.log.info("Restarting nodes")  # to close wallets
12        self.stop_node(0)
13        self.stop_node(1)
14        self.start_node(0)
15        self.start_node(1)
16
17        self.log.info("Loading wallet")
18        print(self.nodes[0].loadwallet(w1_path))  # this should fail with wallet reuse error

Not sure if this needs a separate test though, so any suggestions on which one should have it appended?

rodentrabies force-pushed on Oct 28, 2018

DrahtBot added the label Needs rebase on Nov 5, 2018

meshcollider commented at 4:47 am on November 10, 2018: contributor

It doesn’t seem like a good idea to prevent a wallet from opening in the case of even a 1 block reorg IMO. Maybe checking 6 blocks back in the chain or something would be better?

Concept ACK though, I think this is still a nice check to have

A new test file here would be fine too I think, but you also need to fix up the existing tests to work with this change too, travis is failing on some of them

in src/wallet/wallet.cpp:4130 in ddeb3dff85 outdated

4094@@ -4095,6 +4095,24 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(const std::string& name,
4095 
4096     LOCK(cs_main);
4097 
4098+    // Unless allowed, ensure wallet files are not reused across chains:
4099+    if (!gArgs.GetBoolArg("-allowreusewallets", false))

meshcollider commented at 4:49 am on November 10, 2018:

DEFAULT_ALLOWREUSEWALLETS

in src/wallet/wallet.cpp:4141 in ddeb3dff85 outdated

4105+            // Wallet is assumed to be from another chain, if its best known
4106+            // block is not in active chain:
4107+            CBlockIndex *forkIndex = FindForkInGlobalIndex(chainActive, locator);
4108+            if (locator.vHave.size() > 0 && forkIndex && forkIndex->GetBlockHash() != locator.vHave.front())
4109+            {
4110+                InitError(_("Wallet files should not be reused across chains"));

meshcollider commented at 4:52 am on November 10, 2018:

Maybe walletInstance->WalletLogPrintf the hashes or something too

rodentrabies force-pushed on Nov 10, 2018

DrahtBot removed the label Needs rebase on Nov 10, 2018

rodentrabies force-pushed on Nov 10, 2018

in src/wallet/init.cpp:41 in 9ffb1c658c outdated

37@@ -38,6 +38,7 @@ const WalletInitInterface& g_wallet_init_interface = WalletInit();
38 void WalletInit::AddWalletOptions() const
39 {
40     gArgs.AddArg("-addresstype", strprintf("What type of addresses to use (\"legacy\", \"p2sh-segwit\", or \"bech32\", default: \"%s\")", FormatOutputType(DEFAULT_ADDRESS_TYPE)), false, OptionsCategory::WALLET);
41+    gArgs.AddArg("-allowreusewallets", strprintf("Allow reusing wallet files across chains (default: %u)", DEFAULT_ALLOWREUSEWALLETS), false, OptionsCategory::WALLET);

luke-jr commented at 4:47 am on December 20, 2018:

I’m not sure this should really be an option?

luke-jr commented at 4:48 am on December 20, 2018:

If there is an option, the name -walletcrosschain might be better.

in src/wallet/wallet.cpp:4137 in 9ffb1c658c outdated

4132+        WalletBatch batch(*walletInstance->database);
4133+        CBlockLocator locator;
4134+        if (batch.ReadBestBlock(locator))
4135+        {
4136+            // Wallet is assumed to be from another chain, if its best known
4137+            // block is not in active chain:

luke-jr commented at 4:48 am on December 20, 2018:

This comment’s described behaviour is definitely wrong: the best known block could be a stale block!

rodentrabies commented at 11:04 am on December 20, 2018:

Indeed, but this is going away after the @MeshCollider’s comments are addressed. I didn’t have much time lately, will do it sometime the next two weeks.

luke-jr changes_requested

DrahtBot added the label Needs rebase on Jan 30, 2019

rodentrabies force-pushed on Jun 8, 2019

rodentrabies renamed this:
~~wallet: ensure wallet files are not reused across chains~~
[WIP] wallet: ensure wallet files are not reused across chains
on Jun 8, 2019

DrahtBot removed the label Needs rebase on Jun 8, 2019

wallet: ensure wallet files are not reused across chains dc487656b9

rodentrabies force-pushed on Jun 11, 2019

DrahtBot commented at 0:00 am on June 19, 2019: member

DrahtBot added the label Needs rebase on Jun 19, 2019

DrahtBot commented at 12:54 pm on September 30, 2019: member

DrahtBot added the label Up for grabs on Sep 30, 2019

DrahtBot closed this on Sep 30, 2019

ryanofsky commented at 1:58 pm on September 30, 2019: member

Not sure, but it may be more important to add this check with the new “Open Wallet -> Other…” GUI feature in #15204 to prevent mistakenly loading incompatible wallets.

laanwj removed the label Needs rebase on Oct 24, 2019

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

Looks like I completely forgot about this one. Updated and tested the old branch yesterday - https://github.com/mrwhythat/bitcoin/tree/wallet-file-reuse-prevention. Can it be reopened or should I just open another PR?

fanquake commented at 11:13 am on April 7, 2020: member

I’m unable to re-open this PR, so creating a new one should be ok.

DrahtBot locked this on Feb 15, 2022

MarcoFalke removed the label Up for grabs on Feb 16, 2022

[WIP] wallet: ensure wallet files are not reused across chains #14533

Conflicts