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

pull rodentrabies wants to merge 2 commits into bitcoin:master from rodentrabies:wallet-file-reuse-prevention changing 6 files +78 −0
  1. 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.

  2. fanquake added the label Wallet on Apr 7, 2020
  3. rodentrabies force-pushed on Apr 7, 2020
  4. DrahtBot commented at 1:02 AM on April 16, 2020: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

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

  6. 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?

  7. rodentrabies force-pushed on Apr 23, 2020
  8. rodentrabies force-pushed on Apr 24, 2020
  9. DrahtBot added the label Needs rebase on May 1, 2020
  10. rodentrabies force-pushed on May 1, 2020
  11. DrahtBot removed the label Needs rebase on May 1, 2020
  12. rodentrabies force-pushed on May 1, 2020
  13. DrahtBot added the label Needs rebase on Jun 22, 2020
  14. 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

  15. rodentrabies force-pushed on Mar 15, 2021
  16. DrahtBot removed the label Needs rebase on Mar 15, 2021
  17. adamjonas commented at 4:07 PM on March 19, 2021: member

    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](/bitcoin-bitcoin/0/) 0x556b3fed1e6d in malloc (/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/bitcoind+0x17b7e6d)
    [#1](/bitcoin-bitcoin/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](/bitcoin-bitcoin/0/) 0x556b3fed1e6d in malloc (/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/bitcoind+0x17b7e6d)
    [#1](/bitcoin-bitcoin/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).
  18. rodentrabies force-pushed on Mar 20, 2021
  19. rodentrabies force-pushed on Mar 20, 2021
  20. DrahtBot added the label Needs rebase on Apr 13, 2021
  21. rodentrabies force-pushed on Jun 15, 2021
  22. DrahtBot removed the label Needs rebase on Jun 16, 2021
  23. rodentrabies force-pushed on Jul 16, 2021
  24. rodentrabies force-pushed on Jul 16, 2021
  25. 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 12: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.

  26. theStack commented at 11:25 AM on July 18, 2021: member

    Concept ACK

  27. DrahtBot added the label Needs rebase on Jul 22, 2021
  28. rodentrabies force-pushed on Aug 12, 2021
  29. DrahtBot removed the label Needs rebase on Aug 12, 2021
  30. 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

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

  32. ghost commented at 7:58 PM on November 10, 2021: none

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

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

  34. rodentrabies force-pushed on Nov 28, 2021
  35. rodentrabies force-pushed on Nov 28, 2021
  36. 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

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

    1. One test is failing in CI: wallet_crosschain.py

    2. 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.
  37. 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.

  38. 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"

            self.generate(self.nodes[0], 5, sync_fun=self.no_op)
        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.

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

  40. 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?

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

  42. rodentrabies force-pushed on Feb 15, 2022
  43. rodentrabies force-pushed on Feb 15, 2022
  44. 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: 
        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.

  45. rodentrabies force-pushed on Feb 16, 2022
  46. 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 :

    1. Run bitcoind with PR branch
    2. 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.
    3. 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:

    1. Run bitcoind with PR branch
    2. Create a testnet wallet T2 with bitcoin-cli -named createwallet -wallet_name="T2" -descriptors=false. This creates a legacy wallet.
    3. 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:

    1. Run bitcoind with v22.0 binary
    2. Create a testnet wallet T3 with bitcoin-cli createwallet T3.
    3. 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": ""
}
  47. rodentrabies commented at 11:55 AM on February 16, 2022: contributor

    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.

  48. 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(...).

  49. 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: 
                // chain differs from the genesis block known to the wallet.
    rodentrabies commented at 1:06 PM on February 16, 2022:

    Done.

  50. wallet: ensure wallet files are not reused across chains 968765973b
  51. tests: add tests for cross-chain wallet use prevention 5f213213cb
  52. rodentrabies force-pushed on Feb 16, 2022
  53. achow101 commented at 5:59 PM on February 16, 2022: member

    ACK 5f213213cb17429353ef7ec3e97b185af06d236f

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

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

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

  57. unknown approved
  59. 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

  60. achow101 merged this on Apr 28, 2022
  61. achow101 closed this on Apr 28, 2022
  62. 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!

  63. rodentrabies deleted the branch on Apr 29, 2022
  64. sidhujag referenced this in commit 64fa7c2c9c on Apr 29, 2022
  65. DrahtBot locked this on Apr 29, 2023
Contributors
rodentrabiesDrahtBotluke-jrfanquakeadamjonastheStackghostsipamichaelfolksonS3RKachow101MarcoFalkedongcarl
Labels
Wallet
Linked (view graph)
#12805 Wallet file should not be reused across chains#14533 [WIP] wallet: ensure wallet files are not reused across chains
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-02 15:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me