wallettool: Add dump and createfromdump commands

achow101 commented at 10:30 pm on June 1, 2020: member

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,

achow101 force-pushed on Jun 1, 2020

DrahtBot added the label Build system on Jun 1, 2020

DrahtBot added the label GUI on Jun 1, 2020

DrahtBot added the label Tests on Jun 1, 2020

DrahtBot added the label Wallet on Jun 1, 2020

DrahtBot commented at 1:16 am on June 2, 2020: 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:

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

achow101 force-pushed on Jun 2, 2020

jonasschnelli removed the label GUI on Jun 2, 2020

jonasschnelli commented at 6:40 am on June 2, 2020: contributor

Concept ACK

DrahtBot added the label Needs rebase on Jun 2, 2020

achow101 force-pushed on Jun 2, 2020

DrahtBot removed the label Needs rebase on Jun 2, 2020

Sjors commented at 11:24 am on June 8, 2020: member

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

DrahtBot added the label Needs rebase on Jun 17, 2020

achow101 force-pushed on Jun 17, 2020

DrahtBot removed the label Needs rebase on Jun 17, 2020

DrahtBot added the label Needs rebase on Jun 18, 2020

achow101 force-pushed on Jun 18, 2020

DrahtBot removed the label Needs rebase on Jun 18, 2020

achow101 force-pushed on Jun 18, 2020

in src/wallet/wallettool.cpp:186 in 7ea3df8ccd outdated

182@@ -183,6 +183,79 @@ static bool DumpWallet(std::shared_ptr<CWallet> wallet)
183     return ret;
184 }
185 
186+static bool CreateFromDump(const std::string& name, const fs::path& wallet_path)

ryanofsky commented at 9:22 pm on June 18, 2020:

In commit “wallettool: Add createfromdump command” (7ea3df8ccd13096088538fb4602208eae0831c13)

Would suggest moving this to src/wallet/dump.cpp similar to src/wallet/salvage.cpp so the dump /load functionality be accessed other places outside the wallet tool, like by RPCs, the GUI, and c++ unit tests. Way arguments passed and errors are returned might have to change a little, though

achow101 commented at 0:46 am on June 19, 2020:

I’ve moved it to a new file.

ryanofsky commented at 9:40 pm on June 18, 2020: member

Concept ACK, but might be worth considering if we want add protections to this new dump/load functionality and text format. Maybe there should be a checksum at the end of the file to detect corruption. Maybe there should be a magic string or version number at the beginning in case we want to extend the format in the future and prevent current parsing function from trying to read it

achow101 force-pushed on Jun 19, 2020

achow101 force-pushed on Jun 20, 2020

in src/bitcoin-wallet.cpp:29 in 348be8dafe outdated

25@@ -26,12 +26,15 @@ static void SetupWalletToolArgs()
26 
27     gArgs.AddArg("-datadir=<dir>", "Specify data directory", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
28     gArgs.AddArg("-wallet=<wallet-name>", "Specify wallet name", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::OPTIONS);
29+    gArgs.AddArg("-dumpfile=<file name>", "When used with 'dump', writes out the records to this file instead of printing to stdout. When used with 'createfromdump', loads the records into a new wallet.", ArgsManager::ALLOW_STRING, OptionsCategory::OPTIONS);

MarcoFalke commented at 10:27 am on June 20, 2020:

Is there any reason to support stdout?

I’d find it easier to force this setting for both settings and it would simplify reasoning about arbitrary code execution. IIRC in bash simply catting a file can execute arbitrary commands.

achow101 commented at 7:27 pm on June 22, 2020:

Removed the output to stdout.

achow101 force-pushed on Jun 22, 2020

achow101 commented at 7:29 pm on June 22, 2020: member

Maybe there should be a checksum at the end of the file to detect corruption.

I’m not sure if that’s really useful or practical. We are writing to the wallet file as we read the records so a checksum wouldn’t be able to be computed until all records have been read and written to the file, at which point it’s too late for a checksum to be useful.

Maybe there should be a magic string or version number at the beginning in case we want to extend the format in the future and prevent current parsing function from trying to read it

I’ve added a line at the beginning to identify the file with the magic string BITCOIN_CORE_WALLET_DUMP and a version number.

in test/functional/tool_wallet.py:219 in abd8925e04 outdated

210@@ -211,6 +211,56 @@ def test_salvage(self):
211 
212         self.assert_tool_output('', '-wallet=salvage', 'salvage')
213 
214+    def test_dump_createfromdump(self):
215+        self.start_node(0, ['-wallet=todump', '-wallet=todump2'])
216+        self.stop_node(0)
217+
218+        self.log.info('Checking dump arguments')
219+        self.assert_raises_tool_error('No dump file provided. To use dump, -dumpfile=<filename> must be provided.', '-wallet=todump', 'dump');

adamjonas commented at 6:02 pm on June 29, 2020:

Remove ; at end of line.

achow101 commented at 8:56 pm on June 29, 2020:

Done

in test/functional/tool_wallet.py:225 in abd8925e04 outdated

220+
221+        self.log.info('Checking basic dump')
222+        wallet_dump = os.path.join(self.nodes[0].datadir, "wallet.dump")
223+        self.assert_tool_output('', '-wallet=todump', '-dumpfile={}'.format(wallet_dump), 'dump')
224+
225+        with open(wallet_dump, 'r') as f:

adamjonas commented at 6:04 pm on June 29, 2020:

This is getting caught in lint-python-utf8-encoding for not specifying encoding="utf8".

0        with open(wallet_dump, 'r', encoding="utf8") as f:

Same with the other with opens below (lines 244, 250, 255, and 260).

achow101 commented at 8:56 pm on June 29, 2020:

Fixed

in src/wallet/bdb.h:93 in abd8925e04 outdated

92@@ -93,53 +93,66 @@ std::shared_ptr<BerkeleyEnvironment> GetWalletEnv(const fs::path& wallet_path, s
93 /** Return wheter a BDB wallet database is currently loaded. */

adamjonas commented at 6:24 pm on June 29, 2020:

not your line but, s/wheter/whether/ if you have to touch-up

achow101 commented at 8:57 pm on June 29, 2020:

I think that’d be better to fix in one of the prerequisite PRs or a followup to this whole PR stack.

achow101 force-pushed on Jun 29, 2020

DrahtBot added the label Needs rebase on Jul 1, 2020

achow101 force-pushed on Jul 1, 2020

DrahtBot removed the label Needs rebase on Jul 1, 2020

DrahtBot added the label Needs rebase on Jul 5, 2020

achow101 force-pushed on Jul 6, 2020

DrahtBot removed the label Needs rebase on Jul 6, 2020

DrahtBot added the label Needs rebase on Jul 8, 2020

achow101 force-pushed on Jul 9, 2020

DrahtBot removed the label Needs rebase on Jul 9, 2020

in src/wallet/dump.cpp:51 in 4d7d60a710 outdated

46+            bool ret = batch->ReadAtCursor(ss_key, ss_value, complete);
47+            if (complete) {
48+                break;
49+            } else if (!ret) {
50+                error = _("Error reading next record from wallet database");
51+                ret = false;

Sjors commented at 5:47 pm on July 9, 2020:

4d7d60a710f38a7bd2d9b8fb5da92186f2ad64ee: ret is already false

achow101 commented at 6:08 pm on July 9, 2020:

Hmm. That should be a break. Fixed.

Sjors commented at 5:54 pm on July 9, 2020: member

It would be nice to make the format a bit more human readable. For example each line could be followed by a comment line that contains a human readable version. For this PR, it should be enough to just skip lines starting with #. Or we can do that in a future version.

When I use -dumpfile=~/temp/dump (instead of an absolute path) it doesn’t seem to produce any file, at least not where I can find it.

tACK 29e3d73a899340b5ed0e8e06138faa11a40bc59a modulo issue below

Don’t forget to replace the reference to #18971 in PR description with #19334. Also remove the bit about stdout.

achow101 force-pushed on Jul 9, 2020

achow101 commented at 6:09 pm on July 9, 2020: member

Not sure about the path issue. I just use our fs module to try to get the absolute path.

Also updated the OP.

achow101 force-pushed on Jul 14, 2020

ryanofsky approved

ryanofsky commented at 9:21 pm on July 21, 2020: member

Code review ACK 922e124a73b55ba7363bd9563467b7f600620bd2 last 3 commits. No changes since last review other than rebase

f2b46a53b5553d5567e878ae4640716c7cd69e0a wallettool: Add dump command (5/7)
d75547148660cf2d8a5999b981a80d03845e50e0 wallettool: Add createfromdump command (6/7)
922e124a73b55ba7363bd9563467b7f600620bd2 tests: Test bitcoin-wallet dump and createfromdump (7/7)

re: #19137 (comment)

Maybe there should be a checksum at the end of the file to detect corruption.

I’m not sure if that’s really useful or practical.

The use case is when you make a backup of a wallet and copy it somewhere (usb drive, remote server) and the backup or copy operations fail midway without you noticing. With the current PR truncated wallet backups can load successfully and leave you with a wallet that is missing keys or transactions without any indication.

With a checksum at the end of the file, this problem would not happen. It would be an error to try to restore from a corrupt or truncated dump. Also, a checksum at the end of the file should be easy to compute by just literally hashing the content of the file as it is written and adding the checksum (up to that point) on a final line.

achow101 force-pushed on Jul 23, 2020

ryanofsky approved

ryanofsky commented at 7:04 pm on July 24, 2020: member

Code review ACK c362966def776b5f6974b142e89194f9353ae287. Only change since last review was rebase

MarcoFalke removed the label Build system on Jul 25, 2020

MarcoFalke removed the label Tests on Jul 25, 2020

MarcoFalke added the label Scripts and tools on Jul 25, 2020

DrahtBot added the label Needs rebase on Jul 30, 2020

achow101 force-pushed on Jul 30, 2020

DrahtBot removed the label Needs rebase on Jul 30, 2020

ryanofsky approved

ryanofsky commented at 10:40 pm on August 12, 2020: member

Code review ACK 6befc763491360c52968b4ad56a2e2b673d49a6b, only change since last review was rebase

DrahtBot added the label Needs rebase on Aug 14, 2020

achow101 force-pushed on Aug 14, 2020

DrahtBot removed the label Needs rebase on Aug 14, 2020

ryanofsky approved

ryanofsky commented at 10:37 pm on August 24, 2020: member

Code review ACK ddb7f7f4bc41deb3d7bc8e54ed22a7b756cc6cc2. Only change since last review was rebase

promag commented at 7:47 pm on August 30, 2020: member

Concept ACK.

in src/wallet/dump.cpp:38 in 4e631d278a outdated

33+        error = _("Error: Couldn't create cursor into database");
34+        ret = false;
35+    }
36+
37+    // Write out a magic string with version
38+    tfm::format(dump_file, "%s,1\n", DUMP_MAGIC);

Sjors commented at 6:08 pm on August 31, 2020:

Nit: DUMP_VERSION

achow101 commented at 2:38 am on September 1, 2020:

Done

Sjors approved

Sjors commented at 6:14 pm on August 31, 2020: member

Lightly tested ACK ddb7f7f4bc41deb3d7bc8e54ed22a7b756cc6cc2

I like the checksum suggestion, also for printing and OCR.

meshcollider commented at 10:55 pm on August 31, 2020: contributor

Code review ACK ddb7f7f4bc41deb3d7bc8e54ed22a7b756cc6cc2

I wonder if we should include a warning either in the output or in the dumpfile itself that the file should not be shared. I’ll wait for a comment about this from achow101 before I merge.

IMO the dumpfile tests should include something which signs with a key from the original wallet too, but that can be left for later.

achow101 commented at 11:14 pm on August 31, 2020: member

I wonder if we should include a warning either in the output or in the dumpfile itself that the file should not be shared.

I suppose we should. I would prefer to just output it on stdout rather than put it in the file.

promag commented at 11:33 pm on August 31, 2020: member

Maybe followup if you prefer, but we could support stdin/stdout support in dump/createfromdump instead of just file. For instance for dump:

 0--- a/src/wallet/dump.cpp
 1+++ b/src/wallet/dump.cpp
 2@@ -7,15 +7,15 @@
 3
 4 static const std::string DUMP_MAGIC = "BITCOIN_CORE_WALLET_DUMP";
 5
 6+bool DumpWallet(std::basic_ostream<char>& dump_file, std::shared_ptr<CWallet> wallet, bilingual_str& error);
 7+
 8 bool DumpWallet(std::shared_ptr<CWallet> wallet, bilingual_str& error)
 9 {
10     // Get the dumpfile
11     std::string dump_filename = gArgs.GetArg("-dumpfile", "");
12     if (dump_filename.empty()) {
13-        error = _("No dump file provided. To use dump, -dumpfile=<filename> must be provided.");
14-        return false;
15+        return DumpWallet(std::cout, wallet, error);
16     }
17-
18     fs::path path = dump_filename;
19     path = fs::absolute(path);
20     if (fs::exists(path)) {
21@@ -24,7 +24,11 @@ bool DumpWallet(std::shared_ptr<CWallet> wallet, bilingual_str& error)
22     }
23     fsbridge::ofstream dump_file;
24     dump_file.open(path);
25+    return DumpWallet(dump_file, wallet,  error);
26+}
27
28+bool DumpWallet(std::basic_ostream<char>& dump_file, std::shared_ptr<CWallet> wallet, bilingual_str& error)
29+{
30     WalletDatabase& db = wallet->GetDatabase();
31     std::unique_ptr<DatabaseBatch> batch = db.MakeBatch();

This way -dumpfile is optional and it is more shell friendly.

meshcollider commented at 11:33 pm on August 31, 2020: contributor

@achow101 that sounds fine. Can you quickly add that in before I merge please.

meshcollider commented at 11:46 pm on August 31, 2020: contributor

@promag do you have a proposed use-case for piping a wallet dump? That seems a little dangerous to me.

promag commented at 11:49 pm on August 31, 2020: member

Compress or encrypt or grep 🙉

meshcollider commented at 0:04 am on September 1, 2020: contributor

I think it should be left for a followup but in that case I think it should be done in a way that you have to explicitly choose to pass to stdout. I think many users may accidentally type “dump” without realizing they need to configure -dumpfile, and have their whole wallet just dumped to their terminal.

promag commented at 0:09 am on September 1, 2020: member

Yeah that’s fine, like -dumpfile - or something.

achow101 force-pushed on Sep 1, 2020

achow101 commented at 2:38 am on September 1, 2020: member

Added the warning and a checksum to the dumpfile.

achow101 force-pushed on Sep 1, 2020

meshcollider commented at 10:44 am on September 1, 2020: contributor

Re-utACK 7e845b87c18719274c1db3c9d04fbf99407716c0

Sjors commented at 12:31 pm on September 1, 2020: member

Spurious Travis failure reported in #19853 and intentionally not restarted.

Also:

0wallet/wallettool.cpp:134:33: warning: loop variable 'warning' of type 'const bilingual_str' creates a copy from type 'const bilingual_str' [-Wrange-loop-analysis]
1                for (const auto warning : warnings) {
2                                ^
3wallet/wallettool.cpp:134:22: note: use reference type 'const bilingual_str &' to prevent copying
4                for (const auto warning : warnings) {
5                     ^~~~~~~~~~~~~~~~~~~~
61 warning generated.

achow101 commented at 4:15 pm on September 1, 2020: member

@sjors I don’t think that was caused by this PR.

in src/wallet/dump.cpp:29 in b9cb50ed29 outdated

22+    if (fs::exists(path)) {
23+        error = strprintf(_("File %s already exists. If you are sure this is what you want, move it out of the way first."), path.string());
24+        return false;
25+    }
26+    fsbridge::ofstream dump_file;
27+    dump_file.open(path);

promag commented at 11:52 am on September 2, 2020:

Could check if open is successful and add corresponding test.

achow101 commented at 5:07 pm on September 2, 2020:

Not sure how that would be tested. The open occurs after file existence has been checked for, so the only thing it would error on is some system thing.

achow101 commented at 5:27 pm on September 2, 2020:

Added a check that open doesn’t fail.

promag commented at 6:06 pm on September 2, 2020:

You can test by creating a directory and removing write access, then try to create the dump there.

promag commented at 0:24 am on September 3, 2020:

Like

0self.assert_raises_tool_error('Unable to open /root/cant_write for writing', '-wallet=todump', '-dumpfile=/root/cant_write', 'dump')

achow101 force-pushed on Sep 2, 2020

promag commented at 0:26 am on September 3, 2020: member

I wonder if the content should have the dump timestamp, at the end like timestamp,...?

achow101 commented at 3:51 pm on September 3, 2020: member

I wonder if the content should have the dump timestamp, at the end like timestamp,...?

I don’t think that’s necessary. The file itself will have a creation timestamp and I think that’s good enough.

If we do want this, we can add it later. Records can be added after the checksum and createfromdump will ignore them.

promag commented at 11:55 pm on September 5, 2020: member

Tested ACK 7fba46eba9df4b3a501dee0e4f6c244f21dfbe13.

DrahtBot added the label Needs rebase on Sep 7, 2020

achow101 force-pushed on Sep 7, 2020

DrahtBot removed the label Needs rebase on Sep 7, 2020

ryanofsky approved

ryanofsky commented at 12:21 pm on October 14, 2020: member

Code review ACK f6c4c61715d8514ff6888cad8552d1705ca51bb9. Just rebased and version and checksum added since last review. Checksum seems useful in case backup is truncated or corrupted

achow101 commented at 10:22 pm on October 15, 2020: member

This will need to have a format option to specify bdb or sqlite. I’ll work on that next week.

Sjors commented at 9:14 am on October 16, 2020: member

Great, I’ll try migrating some BDB descriptor wallets to Sqlite once that’s ready.

achow101 commented at 6:49 pm on October 18, 2020: member

Updated with a -format option added to createfromdump

achow101 force-pushed on Oct 19, 2020

achow101 force-pushed on Oct 20, 2020

luke-jr changes_requested

luke-jr commented at 7:03 pm on October 20, 2020: member

Apparently this fails to dump/restore the wallet unique id.

achow101 commented at 4:12 pm on October 21, 2020: member

Apparently this fails to dump/restore the wallet unique id.

The wallet id is not something we are actually using and supporting. It is a byproduct of BDB. Instead of forcing this wallet logic and database type agnostic tool to have to understand BDB, we should make the wallet id actually supported with #20205. I’m not adding BDB id stuff to this PR, it’s out of scope and a layer violation.

in test/functional/tool_wallet.py:298 in 35442fa568 outdated

293+        self.assert_raises_tool_error('Error: Missing checksum', '-wallet=load5', '-format=bdb', '-dumpfile={}'.format(bad_sum_wallet_dump), 'createfromdump')
294+        bad_sum_wallet_dump = os.path.join(self.nodes[0].datadir, "wallet-bad_sum3.dump")
295+        dump_data = orig_dump[:-65] + "2" * 10
296+        with open(bad_sum_wallet_dump, 'w+', encoding='utf8') as f:
297+            f.write(dump_data)
298+        self.assert_raises_tool_error('Error: Dumpfile checksum does not match. Computed {}, expected {}{}'.format(checksum, "2" * 10, "0" * 54), '-wallet=load6', '-format=bdb', '-dumpfile={}'.format(bad_sum_wallet_dump), 'createfromdump')

ryanofsky commented at 11:27 pm on October 21, 2020:

In commit “tests: Test bitcoin-wallet dump and createfromdump” (35442fa568583c0da2c687966e49b3742f403870)

Looking at the CreateFromDump implementation it seems like the wallet will be created but empty if there is a checksum error or other import error. It would be good if there was a check at this point to verify if the created wallets are loadable if they are supposed to be loadable or not loadable if they are not supposed be loadable.

achow101 commented at 3:13 am on October 22, 2020:

I’ve added a check that the file exists (or doesn’t for failure cases).

in src/wallet/dump.cpp:218 in 6137750082 outdated

213+        std::vector<unsigned char> v = ParseHex(value);
214+
215+        CDataStream ss_key(k, SER_DISK, CLIENT_VERSION);
216+        CDataStream ss_value(v, SER_DISK, CLIENT_VERSION);
217+
218+        batch->Write(ss_key, ss_value);

ryanofsky commented at 11:28 pm on October 21, 2020:

In commit “wallettool: Add createfromdump command” (6137750082dd28bf38bbb2922d6d040d4e854643)

ret should be set to false if the Write call fails

achow101 commented at 3:13 am on October 22, 2020:

Done

in src/wallet/dump.cpp:243 in 6137750082 outdated

230+    }
231+
232+    if (ret) {
233+        batch->TxnCommit();
234+    } else {
235+        batch->TxnAbort();

ryanofsky commented at 11:33 pm on October 21, 2020:

In commit “wallettool: Add createfromdump command” (6137750082dd28bf38bbb2922d6d040d4e854643)

Ideally, it would probably be preferable to delete the half created wallet than to just leave it behind.

I think you could add if (!ret) wallet->Delete() after Wallet->Close() below with Delete method implementations that just delete the main datafile after the database is closed.

achow101 commented at 11:45 pm on October 21, 2020:

It would be preferable to do that but deleting things was non-trivial enough that I didn’t want to spend the effort to figure it out.

ryanofsky commented at 0:05 am on October 22, 2020:

It would be preferable to do that but deleting things was non-trivial enough that I didn’t want to spend the effort to figure it out.

Sure. I don’t know, but I’d expect a simple implementation like the following to be adequate and safe:

0assert(!m_db);
1fs::remove(m_file_path);
2fs::remove(m_dir_path);

achow101 commented at 3:13 am on October 22, 2020:

Added wallet removal.

ryanofsky approved

ryanofsky commented at 11:35 pm on October 21, 2020: member

Code review ACK 35442fa568583c0da2c687966e49b3742f403870. Left some suggestions below to improve error handling, but I think they aren’t needed and could be implemented later if desired

achow101 force-pushed on Oct 22, 2020

in src/wallet/dump.cpp:69 in f7a5bf729c outdated

56+            bool ret = batch->ReadAtCursor(ss_key, ss_value, complete);
57+            if (complete) {
58+                break;
59+            } else if (!ret) {
60+                error = _("Error reading next record from wallet database");
61+                break;

S3RK commented at 4:10 am on October 22, 2020:

Because ret overshadows another variable in the parent scope the function will return true and the error won’t be shown to the user.
even if we return false and show an error the dumpfile is still not complete, which could lead to broken backups if user misses an error. I suggest we remove the file if something goes wrong. So if the dumpfile exists it’s always a complete dump.

achow101 commented at 5:26 am on October 22, 2020:

Stopped shadowing.
Removing the dump file if !ret.

in src/wallet/wallettool.cpp:116 in f7a5bf729c outdated

112@@ -112,7 +113,7 @@ bool ExecuteWalletToolFunc(const std::string& command, const std::string& name)
113             WalletShowInfo(wallet_instance.get());
114             wallet_instance->Close();
115         }
116-    } else if (command == "info" || command == "salvage") {
117+    } else if (command == "info" || command == "salvage" || command == "dump") {

S3RK commented at 4:14 am on October 22, 2020:

nit: What’s similar about info, salvage and dump? Can we make it a one-level switch instead?

achow101 commented at 5:26 am on October 22, 2020:

Removed this else if. I think this is a left over of when info and salvage shared wallet opening code.

in src/wallet/dump.cpp:125 in f7a5bf729c outdated

120+        error = strprintf(_("Dump file %s does not exist."), dump_path.string());
121+        return false;
122+    }
123+    fsbridge::ifstream dump_file(dump_path);
124+
125+    if (fs::exists(wallet_path)) {

S3RK commented at 4:44 am on October 22, 2020:

nit: not necessary as MakeDatabase with options.require_create = true will guarantee it doesn’t exist.

achow101 commented at 5:26 am on October 22, 2020:

Removed.

S3RK commented at 4:50 am on October 22, 2020: member

Comments below

achow101 force-pushed on Oct 22, 2020

S3RK commented at 3:49 am on October 23, 2020: member

Code review ACK 225cfa9f0c44798ab42c27d161670c5ee74902a3 CI failure seems unrelated

in src/bitcoin-wallet.cpp:31 in 225cfa9f0c outdated

25@@ -26,12 +26,16 @@ static void SetupWalletToolArgs(ArgsManager& argsman)
26 
27     argsman.AddArg("-datadir=<dir>", "Specify data directory", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
28     argsman.AddArg("-wallet=<wallet-name>", "Specify wallet name", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::OPTIONS);
29+    argsman.AddArg("-dumpfile=<file name>", "When used with 'dump', writes out the records to this file. When used with 'createfromdump', loads the records into a new wallet.", ArgsManager::ALLOW_STRING, OptionsCategory::OPTIONS);
30     argsman.AddArg("-debug=<category>", "Output debugging information (default: 0).", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
31+    argsman.AddArg("-format=<format>", "The format of the wallet file to create. Either \"bdb\" or \"sqlite\".", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);

MarcoFalke commented at 8:27 am on October 23, 2020:

This doesn’t seem to affect the create command, but the createfromdump command? Should that be mentioned?

achow101 commented at 4:39 pm on October 23, 2020:

Added a mention.

achow101 force-pushed on Oct 23, 2020

Sjors commented at 11:07 am on October 26, 2020: member

tACK eff678b

MarcoFalke added this to the milestone 22.0 on Oct 26, 2020

DrahtBot added the label Needs rebase on Nov 1, 2020

in src/wallet/dump.cpp:167 in fd1f2380cf outdated

108+    }
109+
110+    // Get the data file format
111+    std::string file_format = gArgs.GetArg("-format", "");
112+    if (file_format.empty()) {
113+        error = _("No wallet file format provided. To use createfromdump, -format=<format> must be provided.");

ryanofsky commented at 4:03 pm on November 3, 2020:

In commit “wallettool: Add createfromdump command” (fd1f2380cf736df25a27c2607f745ae248c73933)

This seems all right for now, but we should think about how to improve usability in the future because this is inconsistent with other ways wallets are created. Other ways to create wallets either default to BDB format or choose format based on a descriptors/no descriptors option, not a BDB/SQLite format option. Also, dumping and recreating a wallet in the original format may be a challenge here, since both wallet formats look like directories with wallet.dat files, and you can’t tell them apart without opening them or looking for external clues. This -format option could also lead to creation of less well tested BDB+descriptor or SQLite+no-descriptor wallet types.

I think:

Ideally best format would be detected automatically and there should be no need to have a -format option.
If -format option is kept it should give either a warning or an error if an unusual BDB+descriptor or SQLite+no-descriptor wallet is created.
If -format option is kept, wallet wallet-tool create command should either respect it or explicitly disallow it, not ignore it. More ideally a -descriptors/-nodescriptors option would be used to choose the format, so wallet-tool create command would be consistent with other ways wallets are created

luke-jr commented at 4:47 pm on November 18, 2020:

Maybe it should be included in the dump? (Can allow overriding still if user desires to do so)

ryanofsky commented at 6:18 pm on November 18, 2020:

re: #19137 (review)

Maybe it should be included in the dump? (Can allow overriding still if user desires to do so)

:+1:

Like that idea

achow101 commented at 6:04 pm on November 19, 2020:

Given that this tool is supposed to operate at the database level, I don’t think it makes sense to have a -descriptors/-nodescriptors option for it. Additionally, because this tool doesn’t have application level logic, I don’t think it should be able to do any introspection of the wallet it creates in order to warn about unusual format and wallet type combinations.

I would prefer that we keep -format but make it directly an option of createfromdump. It would be better if our args system had something like Python Argparse’s subcommands. But I don’t think that is currently possible to do with our args system.

achow101 commented at 8:11 pm on November 19, 2020:

I’ve limited -format to only createfromdump. I’ve also added a format line to the dump which is used to determine the database format if -format isn’t provided. This required shuffling around some of the code in createfromdump. Tests have also been updated and partially refactored to make them a bit more readable.

in src/wallet/dump.cpp:278 in fd1f2380cf outdated

248+    dump_file.close();
249+    wallet->Close();
250+
251+    // Remove the wallet dir if we have a failure
252+    if (!ret) {
253+        fs::remove_all(wallet_path);

ryanofsky commented at 4:26 pm on November 3, 2020:

In commit “wallettool: Add createfromdump command” (fd1f2380cf736df25a27c2607f745ae248c73933)

I think code probably should call wallet.reset() before this point. wallet->Close is called above, but it seems like Close for BDB only flushes the database, so the environment might not get freed until after fs::remove_all, and lead to later attempted writes.
Alternately this could use a different approach and call a WalletDatabase::Remove method that just removes the known wallet files and empty directory (https://github.com/bitcoin/bitcoin/pull/19137#discussion_r509803825) instead of calling fs::remove_all. Even though remove_all should be safe because of the options.require_create = true line above, a selective remove would seem more safe than a recursive one

achow101 commented at 6:12 pm on November 3, 2020:

Added a reset.

ryanofsky approved

ryanofsky commented at 4:36 pm on November 3, 2020: member

Code review ACK eff678b4ecedb2aabacc1372f531793410024764. I left some suggestions, but none are critical. Changes since last review: removing bad created databases and dumps, code cleanup for info and salvage commands, removing redundant wallet exists check, adding tests to make sure output files exist or don’t exist

achow101 force-pushed on Nov 3, 2020

achow101 commented at 6:12 pm on November 3, 2020: member

Rebased and updated the tests.

DrahtBot removed the label Needs rebase on Nov 3, 2020

achow101 force-pushed on Nov 3, 2020

ryanofsky approved

ryanofsky commented at 3:50 pm on November 9, 2020: member

Code review ACK 7f5adfaf6f7b706ea5182d592a1bddb638172b58. Changes since last review: rebase, cleaned up test code for manipulating output and checking database format, completely closing the wallet before removing files. In the future, I think it would be good to follow up on suggestions in this PR, like autodetecting correct -format so it doesn’t have to be specified, or warning if warn if nonstandard -format is chosen (https://github.com/bitcoin/bitcoin/pull/19137#discussion_r516779603) and adding a safer non-recursive remove

achow101 force-pushed on Nov 19, 2020

luke-jr commented at 10:33 pm on November 19, 2020: member

Any reason to disallow dumping to stdout and creating from stdin?

in src/wallet/wallettool.cpp:177 in 47b4599b7b outdated

176+    } else if (command == "createfromdump") {
177+        bilingual_str error;
178+        std::vector<bilingual_str> warnings;
179+        bool ret = CreateFromDump(name, path, error, warnings);
180+        for (const auto& warning : warnings) {
181+            tfm::format(std::cout, "%s\n", warning.original);

luke-jr commented at 10:38 pm on November 19, 2020:

Warnings should typically go to stderr.

in src/wallet/wallettool.cpp:170 in 47b4599b7b outdated

169+        bool ret = DumpWallet(wallet_instance, error);
170+        if (!ret && !error.empty()) {
171+            tfm::format(std::cerr, "%s\n", error.original);
172             return ret;
173         }
174+        tfm::format(std::cout, "The dumpfile may contain private keys. To ensure the safety of your Bitcoin, do not share the dumpfile.\n");

luke-jr commented at 10:39 pm on November 19, 2020:

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)

DrahtBot added the label Needs rebase on Nov 23, 2020

achow101 force-pushed on Nov 23, 2020

DrahtBot removed the label Needs rebase on Nov 23, 2020

ryanofsky approved

ryanofsky commented at 9:31 pm on December 2, 2020: member

Code review ACK 19b2f4d6f79c9545b40259dff04cd3c936217a2e. 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 commented at 3:26 pm on December 11, 2020: member

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

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

Sjors commented at 12:18 pm on December 15, 2020: member

re-tACK 19b2f4d

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

luke-jr commented at 5:18 pm on December 15, 2020: member

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

ryanofsky commented at 6:58 pm on December 15, 2020: member

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 commented at 7:30 pm on December 15, 2020: member

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 commented at 8:14 pm on December 15, 2020: member

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

MarcoFalke commented at 6:49 am on December 16, 2020: member

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.

in src/wallet/dump.h:12 in 75ac0ad9ac outdated

 7+
 8+class CWallet;
 9+
10+struct bilingual_str;
11+
12+bool DumpWallet(std::shared_ptr<CWallet> wallet, bilingual_str& error);

MarcoFalke commented at 9:03 am on December 16, 2020:

0./wallet/dump.h:12:17: error: use of undeclared identifier 'std'
1bool DumpWallet(std::shared_ptr<CWallet> wallet, bilingual_str& error);
2                ^

MarcoFalke commented at 9:13 am on December 16, 2020:

Also, is there any reason to pass a shared_ptr, when a reference suffices?

Suggested diff for first commit:

 0diff --git a/src/wallet/dump.cpp b/src/wallet/dump.cpp
 1index 242d8dc31c..2e366b2a19 100644
 2--- a/src/wallet/dump.cpp
 3+++ b/src/wallet/dump.cpp
 4@@ -2,13 +2,15 @@
 5 // Distributed under the MIT software license, see the accompanying
 6 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 7 
 8+#include <wallet/dump.h>
 9+
10 #include <util/translation.h>
11 #include <wallet/wallet.h>
12 
13 static const std::string DUMP_MAGIC = "BITCOIN_CORE_WALLET_DUMP";
14 uint32_t DUMP_VERSION = 1;
15 
16-bool DumpWallet(std::shared_ptr<CWallet> wallet, bilingual_str& error)
17+bool DumpWallet(CWallet& wallet, bilingual_str& error)
18 {
19     // Get the dumpfile
20     std::string dump_filename = gArgs.GetArg("-dumpfile", "");
21@@ -32,7 +34,7 @@ bool DumpWallet(std::shared_ptr<CWallet> wallet, bilingual_str& error)
22 
23     CHashWriter hasher(0, 0);
24 
25-    WalletDatabase& db = wallet->GetDatabase();
26+    WalletDatabase& db = wallet.GetDatabase();
27     std::unique_ptr<DatabaseBatch> batch = db.MakeBatch();
28 
29     bool ret = true;
30@@ -52,7 +54,6 @@ bool DumpWallet(std::shared_ptr<CWallet> wallet, bilingual_str& error)
31     hasher.write(line.data(), line.size());
32 
33     if (ret) {
34-
35         // Read the records
36         while (true) {
37             CDataStream ss_key(SER_DISK, CLIENT_VERSION);
38@@ -78,7 +79,7 @@ bool DumpWallet(std::shared_ptr<CWallet> wallet, bilingual_str& error)
39     batch.reset();
40 
41     // Close the wallet after we're done with it. The caller won't be doing this
42-    wallet->Close();
43+    wallet.Close();
44 
45     if (ret) {
46         // Write the hash
47diff --git a/src/wallet/dump.h b/src/wallet/dump.h
48index a62c388b72..0f17ee1d0d 100644
49--- a/src/wallet/dump.h
50+++ b/src/wallet/dump.h
51@@ -9,6 +9,6 @@ class CWallet;
52 
53 struct bilingual_str;
54 
55-bool DumpWallet(std::shared_ptr<CWallet> wallet, bilingual_str& error);
56+bool DumpWallet(CWallet& wallet, bilingual_str& error);
57 
58 #endif // BITCOIN_WALLET_DUMP_H
59diff --git a/src/wallet/wallettool.cpp b/src/wallet/wallettool.cpp
60index 1fed63b755..f78ebd4376 100644
61--- a/src/wallet/wallettool.cpp
62+++ b/src/wallet/wallettool.cpp
63@@ -149,7 +149,7 @@ bool ExecuteWalletToolFunc(const std::string& command, const std::string& name)
64         std::shared_ptr<CWallet> wallet_instance = MakeWallet(name, path, /* create= */ false);
65         if (!wallet_instance) return false;
66         bilingual_str error;
67-        bool ret = DumpWallet(wallet_instance, error);
68+        bool ret = DumpWallet(*wallet_instance, error);
69         if (!ret && !error.empty()) {
70             tfm::format(std::cerr, "%s\n", error.original);
71             return ret;

achow101 commented at 5:37 pm on December 16, 2020:

Done

MarcoFalke approved

MarcoFalke commented at 9:46 am on December 16, 2020: member

cr ACK 19b2f4d6f79c9545b40259dff04cd3c936217a2e 🌑

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3cr ACK 19b2f4d6f79c9545b40259dff04cd3c936217a2e 🌑
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUi6dwv/Q7n60qh/mNckiFcdaCie+46b616Ht/Eb4OhQb2eFxEJtzbkRTNRAUmKo
 8UAisA8eZyijggrVJACW8xsXUb7ZJV9//ffcof7S9Kty7kKe/NA8x2eW539Gd4fdH
 9GtaGG1Ds+KlA0xMylpmWWhPBIHdQnHjxA30OOL3dWiYF46Es3qojcp2vSQbh3DjW
10cf+ZESMhq1955LotIMdJRmiIfADKKAKdchpuHMncVDaW9Thw8g582+5qFui6aG+O
11UlO4TG1uY348IzCaiEOmkMNmzu7Zlb//oHMSBnM1DHDGEASNUAuTF374/abK4uUr
12u18NDPu5pvN3NZJT6nlgKmgPVioVO0hZwH1lCDheS0tkxnwdLUM6h0qhXe1l4rUm
13ldRQjfMkzhBBO+2P928XbBmetl/D0Wsi6Ao3yjjdpEuiwpuJHfDm2pfMXHddcpm0
14/++Tlk50ucllbM2bL/VTrpUtzZ6Q9U1i6byXGwAlvAUU13yTMmFI93jrzAuxNJr4
15kmY39zE3
16=uoBI
17-----END PGP SIGNATURE-----

Timestamp of file with hash 8e4d758db391119ee801077cd63a6a83488b5e8acd377363236fecba12d56f34 -

MarcoFalke commented at 9:46 am on December 16, 2020: member

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 (review)

MarcoFalke commented at 9:46 am on December 16, 2020: member

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

MarcoFalke referenced this in commit 69f1ee1922 on Dec 16, 2020

achow101 commented at 5:13 pm on December 16, 2020: member

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

wallettool: Add dump command

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

e1e7a90d5f

wallettool: Add createfromdump command

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

a88c320041

tests: Test bitcoin-wallet dump and createfromdump 23cac24dd3

achow101 force-pushed on Dec 16, 2020

Sjors commented at 12:27 pm on December 17, 2020: member

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

MarcoFalke commented at 12:33 pm on December 17, 2020: member

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

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re review ACK 23cac24dd3f2aaf88aab978e7ef4905772815cd2 only change is rebase and removing useless shared_ptr wrapper 🎼
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjHkwv/YsYYRt14UrM5EcbCKReDDt9Ul4OE3P6iwjGU7ztOmfpT4yvDVwxvs6N4
 8x2YVIkLx7tCJSZagdgg3aJY5P4g7GLzFRtR4Gpz6iLRD3Q4BXvKgRlYqpbWfVY0Z
 9FypJn0duhvB6GT6YlduyaZlqzepPmDo9RNMxsZJ0DsU9dSwP498LfDnpPPxI4Cu2
10Z7XGrNRRPPRlBPpD88ZNCC3NxAfjcmpecbfv3aAKwUuWdw7H1ksNiDZeYXuBtw9I
119XdwJhzzVxyj+8uhwqTljUVBIp+kDnhCqZCZeaEVNx66aG/2YdkBinY3D5Z+rpSd
12dB6b22W2S3TnbUrjCiz/bF7NxaZxbsGms3hXG332+VKSQR+/6PSVh33VGK7woHDM
13TQ/cwmw7xCHeVVtwhsbpUFiAzSh+Hen89jsXNM0PlB8n1rhO2T/DufYFeQyNskhi
14WjglPPQTcugCpRZo/TLYEuZ1WrcrtzaATNI+ROpcDCsA16WaSKG5XLO/4QwhLUsF
15rcaeEhD0
16=Do37
17-----END PGP SIGNATURE-----

Timestamp of file with hash 82f548fd46541ccebb08bdfe077afdd42eda0b903148cceba0f7b365cebec1ab -

in src/wallet/wallettool.cpp:111 in e1e7a90d5f outdated

106@@ -106,6 +107,12 @@ bool ExecuteWalletToolFunc(const std::string& command, const std::string& name)
107 {
108     fs::path path = fs::absolute(name, GetWalletDir());
109 
110+    // -dumpfile is only allowed with dump and createfromdump. Disallow it for all other commands.
111+    if (gArgs.IsArgSet("-dumpfile") && command != "dump" && command != "createfromdump") {

ryanofsky commented at 1:32 pm on December 17, 2020:

In commit “wallettool: Add dump command” (e1e7a90d5f0616a46ffadd62a9f1c65406cca6b4)

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 (review))

ryanofsky approved

ryanofsky commented at 1:34 pm on December 17, 2020: member

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

MarcoFalke merged this on Dec 17, 2020

MarcoFalke closed this on Dec 17, 2020

sidhujag referenced this in commit cc0d58a761 on Dec 17, 2020

DrahtBot locked this on Feb 15, 2022

wallettool: Add dump and createfromdump commands #19137

Conflicts