wallettool: Add dump and createfromdump commands #19137

achow101 · 2020-06-01T22:30:04Z

Adds two commands to the bitcoin-wallet tool: dump and createfromdump. These commands will be useful for a wallet storage migration in the future. It is also generally useful to have a storage agnostic dump like this. These commands are similar to BDB's db_dump and db_load tools. This can also be useful for manual construction of a wallet file for tests.

dump outputs every key-value pair from the wallet as comma separated hex. Each key-value pair is on its own line with the key and value in hex separated by a comma. This is output to the file specified by the new -dumpfile option.

createfromdump takes a file produced by dump and creates a new wallet file with exactly the records specified in that file.

A new option -dumpfile is added to the wallet tool. When used with dump, the records will be written to the specified file. When used with createfromdump, the file is read and the key-value pairs constructed from it. createfromdump requires -dumpfile.

A simple round-trip test is added to the tool_wallet.py.

This PR is based on #19334,

DrahtBot · 2020-06-02T01:16:45Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

External signer support - Wallet Box edition #16546 (External signer support - Wallet Box edition by Sjors)

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.

jonasschnelli · 2020-06-02T06:40:03Z

Concept ACK

Sjors · 2020-06-08T11:24:06Z

Concept ACK. Can you add a sanity check to the test? E.g. a getaddressinfo call.

luke-jr · 2020-11-19T22:39:11Z

src/wallet/wallettool.cpp

            return ret;
        }
+        tfm::format(std::cout, "The dumpfile may contain private keys. To ensure the safety of your Bitcoin, do not share the dumpfile.\n");


Better to send this to stderr too, in case dumping to stdout (which can always be done with -dumpfile /dev/stdout even if not explicitly supported)

ryanofsky

Code review ACK 19b2f4d. Main changes since last review are nice format autodetection suggested by Luke, and extra error checking to disallow inapplicable options. I also like Luke's suggestions to send warnings to stderr and use stdin/stdout for data, and would still feel better to see recursive fs::remove_all replaced with a more selective delete. Also I don't think createfromdump and create need to be different commands. It seems like it would simplify usage and unify code if create command was extended to accept the -dumpfile option and createfromdump command was dropped.

But these are all minor issues that could be addressed in followups. Bigger issues have all been discussed and addressed and this PR seems very good in its current form.

ryanofsky · 2020-12-11T15:26:38Z

This is a somewhat complicated change but it only affects wallet tool and has had ACKs at different points from me #19137 (review), Sjors #19137 (comment), Ivan #19137 (comment), promag #19137 (review), meshcollider #19137 (comment) and some review suggestions implemented from Luke #19137 (comment).

It seems about ready for merge to me. Does it need more reviewers or reacks?

Sjors · 2020-12-15T12:18:40Z

re-tACK 19b2f4d

More appropriate use of stdout and stderr would be nice, but can wait for a followup.

luke-jr · 2020-12-15T17:18:24Z

NACK until this can handle dumping/restoring the wallet id. (Or alternatively simply refuse to dump/restore BDB wallets until this is fixed)

ryanofsky · 2020-12-15T18:58:09Z

NACK until this can handle dumping/restoring the wallet id. (Or alternatively simply refuse to dump/restore BDB wallets until this is fixed)

What are the negative consequences of merging the PR without this feature? The only place wallet ids are used is to forbid opening an older type of wallet that we don't create anymore to avoid runtime data corruption (#11429). This isn't relevant to dump/createfromdump because createfromdump doesn't create the type of wallet that would have this corruption (a BDB wallet sharing an BDB environment with another wallet that has the same fileid), and even if it did you'd think choosing a new id would actually be more helpful than choosing a duplicate id.

I know there are future PRs and maybe forks that envision using unique ids for new purposes, but if this PR causes a problem for them, I think a substantive NACK would explain specifically how the problem arises and what the consequences are.

luke-jr · 2020-12-15T19:30:18Z

What are the negative consequences of merging the PR without this feature?

A user might think they've created an identical clone of a wallet that is in fact different.

an older type of wallet that we don't create anymore

Interesting, I didn't realise the dedicated db environments fixed that. But ultimately not relevant.

I know there are future PRs and maybe forks that envision using unique ids for new purposes, but if this PR causes a problem for them, I think a substantive NACK would explain specifically how the problem arises and what the consequences are.

Anything that uses wallet ids (eg, pruning locks that persist even when the wallet is unloaded, across renames/moves/etc) would treat these "clones" as independent/unrelated wallets.

ryanofsky · 2020-12-15T20:14:47Z

What are the negative consequences of merging the PR without this feature?

A user might think they've created an identical clone of a wallet that is in fact different.

As I explained, we don't have any code that treats these wallets as different. So the NACK above is based on other code which isn't part of the software and hasn't been described in any detail, which makes it not seem very substantive. If there is a legitimate objection, it should be possible to describe a scenario where createfromdump interacts badly with prunelocks or another feature in some other code where not merging this PR somehow makes things better.

(For prunelocks, it would seem like the safest thing would be to treat different wallet files as different and not automatically delete locks, but you would be the expert there so please fill me in!)

maflcko · 2020-12-16T06:49:29Z

It is already possible with RPCs to create an "identical clone of a wallet that is in fact different", so I think the NACK describes something unrelated to this pull and should thus be filed as an issue.

Even if the NACK applied to this pull, instead of wholesale rejecting it, a simple documentation fixup can be applied to document the shortcomings and dangers, if there are any.

maflcko

cr ACK 19b2f4d 🌑

Show signature and timestamp

Signature:

Timestamp of file with hash 8e4d758db391119ee801077cd63a6a83488b5e8acd377363236fecba12d56f34 -

src/wallet/dump.h

maflcko · 2020-12-16T09:46:22Z

More appropriate use of stdout and stderr would be nice, but can wait for a followup.

Sorry, this has been removed based on my feedback, because I missed that all strings written are hex-encoded (or ascii). See #19137 (comment)

maflcko · 2020-12-16T09:46:51Z

@achow101 Let me know if you want this merged or want to address the style nits

173cc9b test: walettool create descriptors (Ivan Metlushko) 345e88e wallettool: add param to create descriptors wallet (Ivan Metlushko) 6d3af3a wallettool: pass in DatabaseOptions into MakeWallet (Ivan Metlushko) Pull request description: Rationale: expose and promote descriptor wallets in more places; make cli tool more consistent with `createwallet` rpc. Add `-descriptors` parameter which is off by default. When specified it will create a new descriptors wallet with sqlite backend, which is consistent with `createwallet` rpc. This PR is based on a suggestion from **ryanofsky** bitcoin/bitcoin#19137 (comment) Example: ``` $ ./src/bitcoin-wallet -wallet=fewty -descriptors create Topping up keypool... Wallet info =========== Name: fewty Format: sqlite Descriptors: yes Encrypted: no HD (hd seed available): yes Keypool Size: 6000 Transactions: 0 Address Book: 0 ``` ``` $ ./src/bitcoin-wallet -wallet=fewty create Topping up keypool... Wallet info =========== Name: fewty Format: bdb Descriptors: no Encrypted: no HD (hd seed available): yes Keypool Size: 2000 Transactions: 0 Address Book: 0 ``` ACKs for top commit: achow101: ACK 173cc9b ryanofsky: Code review ACK 173cc9b. This seems pretty nicely implemented now, with opportunities to clean up more and dedup later MarcoFalke: Concept ACK 173cc9b 🌠 Tree-SHA512: cc32ba336ff709de2707ee15f495b4617908e8700ede8401a58e894f44cda485c544d644023c9a6604d88a62db9d92152383ee2e8abf691688c25cf6e222c622

achow101 · 2020-12-16T17:13:49Z

Seeing as this now needs rebase, I guess I'll address the style nits.

Adds a new dump command to bitcoin-wallet which prints out all of the wallet's records in hex.

Creates a new wallet file using the dump file produced by the dump command

Sjors · 2020-12-17T12:27:33Z

In my experience treating wallets created from a dump as unique is a feature, not a bug. By definition the dump no longer gets updated, while the original does. And when you load from a dump, any rescan has to start where the dump left off, rather than whenever you closed the original wallet. In fact I like to keep both Sqlite and BDB versions around, and open, while testing these features (though that's not a common use case).

A followup could add an option to reuse the same wallet id.

re-utACK 23cac24

maflcko · 2020-12-17T12:33:02Z

re review ACK 23cac24 only change is rebase and removing useless shared_ptr wrapper 🎼

Show signature and timestamp

Signature:

Timestamp of file with hash 82f548fd46541ccebb08bdfe077afdd42eda0b903148cceba0f7b365cebec1ab -

ryanofsky

Code review ACK 23cac24. Only changes since last review rebase and changing a pointer to a reference

ryanofsky · 2020-12-17T13:32:17Z

src/wallet/wallettool.cpp

@@ -106,6 +107,12 @@ bool ExecuteWalletToolFunc(const std::string& command, const std::string& name)
 {
    fs::path path = fs::absolute(name, GetWalletDir());

+    // -dumpfile is only allowed with dump and createfromdump. Disallow it for all other commands.
+    if (gArgs.IsArgSet("-dumpfile") && command != "dump" && command != "createfromdump") {


In commit "wallettool: Add dump command" (e1e7a90)

For a followup, it would be good to print an error in the if (gArgs.IsArgSet("-descriptors") && command != "create") case so there is an explicit error if someone tries to pass -descriptors/-nodescriptors to createfromdump. (Suggested originally in S3RK's PR #20365 (comment))

achow101 force-pushed the dumpwalletrecords branch from 567785f to e8d04c5 Compare June 1, 2020 22:37

DrahtBot added Build system GUI Tests Wallet labels Jun 1, 2020

achow101 force-pushed the dumpwalletrecords branch from e8d04c5 to 3754271 Compare June 2, 2020 03:36

jonasschnelli removed the GUI label Jun 2, 2020

DrahtBot mentioned this pull request Jun 2, 2020

Use shared pointers only in validation interface #18354

Closed

DrahtBot added the Needs rebase label Jun 2, 2020

achow101 force-pushed the dumpwalletrecords branch from 3754271 to 2fd241f Compare June 2, 2020 16:45

DrahtBot removed the Needs rebase label Jun 2, 2020

This was referenced Jun 9, 2020

wallet: Remove first parameter to ScanForWalletTransactions start_hash #19216

Closed

wallet: Refactor the classes in wallet/db.{cpp/h} #18971

Closed

luke-jr reviewed Nov 19, 2020

View reviewed changes

fitrajaya999 mentioned this pull request Nov 20, 2020

Check bitcoinbook/bitcoinbook#702

Closed

DrahtBot added the Needs rebase label Nov 23, 2020

achow101 force-pushed the dumpwalletrecords branch from 47b4599 to 19b2f4d Compare November 23, 2020 17:31

DrahtBot removed the Needs rebase label Nov 23, 2020

ryanofsky approved these changes Dec 2, 2020

View reviewed changes

maflcko approved these changes Dec 16, 2020

View reviewed changes

src/wallet/dump.h Outdated Show resolved Hide resolved

achow101 added 3 commits December 16, 2020 12:32

wallettool: Add dump command

e1e7a90

Adds a new dump command to bitcoin-wallet which prints out all of the wallet's records in hex.

wallettool: Add createfromdump command

a88c320

Creates a new wallet file using the dump file produced by the dump command

tests: Test bitcoin-wallet dump and createfromdump

23cac24

achow101 force-pushed the dumpwalletrecords branch from 19b2f4d to 23cac24 Compare December 16, 2020 17:37

ryanofsky approved these changes Dec 17, 2020

View reviewed changes

maflcko merged commit 143bd10 into bitcoin:master Dec 17, 2020

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 17, 2020

Merge bitcoin#19137: wallettool: Add dump and createfromdump commands

cc0d58a

barrystyle mentioned this pull request Feb 13, 2021

1.21 codebase rebrand dogecoin/dogecoin#1711

Merged

achow101 mentioned this pull request Oct 6, 2021

Proposed Timeline for Legacy Wallet and BDB removal #20160

Closed

48 tasks

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

wallettool: Add dump and createfromdump commands #19137

wallettool: Add dump and createfromdump commands #19137

Uh oh!

Conversation

achow101 commented Jun 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jun 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jonasschnelli commented Jun 2, 2020

Uh oh!

Sjors commented Jun 8, 2020

Uh oh!

luke-jr Nov 19, 2020

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Dec 11, 2020

Uh oh!

Sjors commented Dec 15, 2020

Uh oh!

luke-jr commented Dec 15, 2020

Uh oh!

ryanofsky commented Dec 15, 2020

Uh oh!

luke-jr commented Dec 15, 2020

Uh oh!

ryanofsky commented Dec 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Dec 16, 2020

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Dec 16, 2020

Uh oh!

maflcko commented Dec 16, 2020

Uh oh!

achow101 commented Dec 16, 2020

Uh oh!

Sjors commented Dec 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Dec 17, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Dec 17, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

achow101 commented Jun 1, 2020 •

edited

Loading

DrahtBot commented Jun 2, 2020 •

edited

Loading

ryanofsky commented Dec 15, 2020 •

edited

Loading

Sjors commented Dec 17, 2020 •

edited

Loading