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.

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.

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

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

        # ...
        self.log.info("Generating initial distinct blockchains")
        self.nodes[0].generate(5)
        self.nodes[1].generate(5)

        self.log.info("Creating wallets")
        w0_path = os.path.join(self.nodes[0].datadir, 'w0')
        w1_path = os.path.join(self.nodes[1].datadir, 'w1')
        self.nodes[0].createwallet(w0_path)
        self.nodes[1].createwallet(w1_path)

        self.log.info("Restarting nodes")  # to close wallets
        self.stop_node(0)
        self.stop_node(1)
        self.start_node(0)
        self.start_node(1)

        self.log.info("Loading wallet")
        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?

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

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.

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?

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