[wallet] wallet-tool: command to remove key metadata #15424

Sjors commented at 8:39 PM on February 16, 2019: member

Adds a keymetadata method to wallet-tool with currently only a delete operation.

Sjors commented at 8:42 PM on February 16, 2019: member

I found this useful for salvaging* wallets touched by intermediate versions of #14021.

It is a bit dangerous, so perhaps some seatbelt is useful? E.g. we could disable this command for mainnet in release builds.

A useful followup would be a reconstruct operation, which takes the master key, re-derives all keys, finds those in the wallet and then restores key origin info. It could take an additional version argument, which restores key metadata for earlier wallet versions. The latter can be useful for backwards compatibility and upgrade path testing.

I'm unsure if the EraseWatchOnlyMeta bit is correct. I do have a wallet that I mangled in such a way that its watchmeta entries are corrupt, but this PR doesn't strip those entries. I think that's because keymetadata delete works by iterating over all known keys, and these entries correspond to disappeared keys.

The test case for watch-only entries passes without the EraseWatchOnlyMeta bit, because afaik importmulti seems to add keymeta entries instead of watchmeta entries. I'm not sure if that's correct. cc @achow101

* = which pops up a NONCRITICAL_ERROR warning in QT every time, which by the way loadwallet doesn't add to its warnings field.

in src/wallet/wallet.cpp:363 in 0edbb1d4e6 outdated

 357 | @@ -358,6 +358,20 @@ void CWallet::LoadScriptMetadata(const CScriptID& script_id, const CKeyMetadata
 358 |      m_script_metadata[script_id] = meta;
 359 |  }
 360 |  
 361 | +bool CWallet::EraseWatchOnlyMeta(const CScript& script)
 362 | +{
 363 | +    AssertLockHeld(cs_wallet); // m_script_metadata

practicalswift commented at 9:46 PM on February 16, 2019:

Could drop this: we already have the locking annotations.

in src/wallet/wallet.cpp:418 in 0edbb1d4e6 outdated

 412 | @@ -399,6 +413,37 @@ void CWallet::UpgradeKeyMetadata()
 413 |      SetWalletFlag(WALLET_FLAG_KEY_ORIGIN_METADATA);
 414 |  }
 415 |  
 416 | +void CWallet::DeleteKeyMetadata()
 417 | +{
 418 | +    AssertLockHeld(cs_wallet);

practicalswift commented at 9:46 PM on February 16, 2019:

Could drop this: we already have the locking annotations.

fanquake added the label Wallet on Feb 16, 2019

in src/bitcoin-wallet.cpp:149 in 0edbb1d4e6 outdated

 149 | +        };
 150 | +        if (arguments.empty() || operations.find(arguments[0]) == operations.end()) {
 151 | +            fprintf(stderr, "Specify operation: delete\n");
 152 | +            return EXIT_FAILURE;
 153 | +        } else {
 154 | +            if (!WalletTool::ExecuteKeyMetadata(name, arguments[0])) return EXIT_FAILURE;

practicalswift commented at 10:21 AM on February 17, 2019:

Else after return: could drop else here?

in src/wallet/wallet.h:904 in 0edbb1d4e6 outdated

 900 | @@ -897,6 +901,7 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
 901 |  
 902 |      //! Adds a watch-only address to the store, and saves it to disk.
 903 |      bool AddWatchOnly(const CScript& dest, int64_t nCreateTime) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 904 | +    bool EraseWatchOnlyMeta(const CScript &dest) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);

practicalswift commented at 10:22 AM on February 17, 2019:

Make sure parameter names match between definition and declaration :-)

in src/wallet/wallettool.cpp:68 in 0edbb1d4e6 outdated

  67 | @@ -68,23 +68,26 @@ static std::shared_ptr<CWallet> LoadWallet(const std::string& name, const fs::pa
  68 |      }

practicalswift commented at 10:23 AM on February 17, 2019:

When changing this file: please consider changing to const reference in catch on L65.

in test/functional/tool_wallet.py:110 in 0edbb1d4e6 outdated

 105 | +        self.start_node(0, ['-wallet=foo'])
 106 | +        address = self.nodes[0].getnewaddress()
 107 | +        address_info_before = self.nodes[0].getaddressinfo(address)
 108 | +        assert("hdmasterfingerprint" in address_info_before)
 109 | +        self.nodes[0].importmulti([{"desc": "wpkh([deadbeef/1/2'/3/4']03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd)",
 110 | +                               "timestamp": "now",

practicalswift commented at 10:24 AM on February 17, 2019:

Weird indentation: Consider using black to get the proper indentation :-)

[wallet] tool: deduplicate and make wallet loading code reusable ab544a1dc0

[wallet] tool: do not abort loading for DBErrors::NONCRITICAL_ERROR a8fec09cd8

[wallet] tool: add keymetadata method and delete operation d85e4f74a8

[wallet] DeleteKeyMetadata 33165a7ac5

Sjors force-pushed on Feb 20, 2019

[wallet] delete watch-only key metadata 4b707eaaa4

Sjors force-pushed on Feb 20, 2019

DrahtBot commented at 7:43 PM on March 28, 2019: 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:

#15741 (Batch write imported stuff in importmulti by achow101)
#15687 (test: tool wallet test coverage for unexpected writes to wallet by jonatack)

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.

DrahtBot commented at 7:34 AM on May 29, 2019: member

Needs rebase

DrahtBot added the label Needs rebase on May 29, 2019

Sjors commented at 3:00 PM on July 5, 2019: member

Closing for now due to lack of interest and me not using it much either.

Sjors closed this on Jul 5, 2019

laanwj commented at 3:06 PM on July 5, 2019: member

It is a bit dangerous, so perhaps some seatbelt is useful? E.g. we could disable this command for mainnet in release builds.

Don't know whether this kind of very case-specific recovery tooling belongs in wallet-tool or makes more sense as a separate script/tool for direct wallet database manipulation. I think in general it's very hard to keep maintained because only few people will have interest in it at a certain time, but when you do happen to need it it's great to have.

laanwj removed the label Needs rebase on Oct 24, 2019

DrahtBot locked this on Dec 16, 2021