Add scripts to dumpwallet RPC #11667

meshcollider · 2017-11-12T06:07:45Z

As discussed in #11289 (comment), adds the CScripts from the wallet to the dumpwallet RPC and then allows them to be imported with the importwallet RPC. Includes a basic test, and modifies the helptext of the dumpwallet RPC.

Notes:

Reviewers: use ?w=1 to avoid the indentation-only change in commit Add scripts to importwallet RPC
currently the scripts are followed with # addr= comments just as the other keys are, unsure if this might confuse users into thinking all the scripts are for valid P2SH addresses though, but I don't think that should be an issue.
there are no birthtimes for scripts, so script imports don't affect rescans
importwallet imports the CScripts but I'm not sure how to approach specifying whether scripts are for P2SH addresses, BIP173 addresses, etc. whether that matters or not. Otherwise the RPC helptext might just need modification.

Fixes #11715

meshcollider · 2017-11-12T06:16:58Z

@TheBlueMatt @laanwj you might want to take a look after the discussion in the other PR

laanwj · 2017-11-13T13:35:47Z

Concept ACK, thanks for having a shot at this,will review in detail later.

jonasschnelli · 2017-11-13T20:34:50Z

Concept ACK
Maybe importwallet should get an overhauled description (currently "\nImports keys from a wallet dump file (see dumpwallet)....?
Should we returns warning when importwallet was importing scripts?

meshcollider · 2017-11-13T21:12:34Z

@jonasschnelli

Should we returns warning when importwallet was importing scripts?

you mean like a warning about manually rescanning or something? Just to let the user know it contained scripts too?

promag

cs_wallet should be locked in dumpwallet?

GetAllReserveKeys() is not protected, worst, it returns a const reference.

And now with the extra GetCScripts() this should be "atomic", IMO.

promag · 2017-11-15T10:27:20Z

src/wallet/rpcdump.cpp

+        std::string address = EncodeDestination(scriptid);
+        if(pwallet->GetCScript(scriptid, script)) {
+            file << strprintf("%s 0 script=1", HexStr(script.begin(), script.end()));
+            file << strprintf(" # addr=%s\n", address);


We don't test the comment?

~~Sorry, unsure what you mean?~~ Discussed on IRC

Nevermind, address is parsed in

bitcoin/test/functional/wallet-dump.py

Lines 27 to 33 in 4db82b7

key_label, comment = line.split("#")

# key = key_label.split(" ")[0]

keytype = key_label.split(" ")[2]

if len(comment) > 1:

addr_keypath = comment.split(" addr=")[1]

addr = addr_keypath.split(" ")[0]

keypath = None

promag · 2017-11-15T10:29:31Z

doc/release-notes.md

@@ -89,6 +89,10 @@ Low-level RPC changes
 - `dumpwallet` no longer allows overwriting files. This is a security measure
  as well as prevents dangerous user mistakes.

+- `dumpwallet` now includes hex-encoded scripts from the wallet in the dumpfile, and
+  `importwallet` now imports these scripts, but corresponding addresses may not be added
+  correctly or a manual rescan may be required to find transactions.


to find relevant transactions or something along that.

meshcollider · 2017-11-15T10:39:13Z

@promag

cs_wallet should be locked in dumpwallet?

It is locked on line 630 right? LOCK2(cs_main, pwallet->cs_wallet);

meshcollider · 2017-11-15T10:50:37Z

Fixed @promag's comment nit, thanks :)

The release notes and comment change may still need modification based on what needs to be said about BIP173, etc.

promag · 2017-11-15T11:45:02Z

How about const map<>& GetScripts() const, which returns mapScripts, to avoid the set creation and copy and lookups added further? Not sure about cs_KeyStore.

ryanofsky

utACK 468771372c96378268272971212943e7b46596cb

ryanofsky · 2017-12-18T14:37:17Z

src/wallet/rpcdump.cpp

@@ -694,6 +696,15 @@ UniValue dumpwallet(const JSONRPCRequest& request)
        }
    }
    file << "\n";
+    for (const CScriptID &scriptid : scripts) {


This is not dumping the information in m_script_metadata. At very least, should add a comment with possible todo here to include metadata in the future. Or you could implement a minimal version of this by adding an EncodeDumpTime(m_script_metadata.at(scriptid)nCreateTime) field to the output. It would also be possible to extend GetKeyBirthTimes to work with scripts and derive birth times from transaction times, and sort output by these times, to make script output consistent with key output.

~~It would also be good to either dump or add a TODO for dumping WitnessV0ScriptHash and WitnessV0KeyHash keys and scripts.~~

It would also be good to either dump or add a TODO for dumping WitnessV0ScriptHash and WitnessV0KeyHash keys and scripts.

Aren't WitnessV0* scripts and keys also included in mapScripts and mapKeys? Or do you mean like witness derivatives of "normal" keys?

Yeah, they are. I was thinking of mapAddressBook entries, but these aren't exported at all by dumpwallet.

meshcollider · 2017-12-20T21:21:59Z

@ryanofsky m_script_metadata didn't actually exist at the time I made this PR, you introduced it in 11854, so I've rebased onto master to use it. Aren't WitnessV0* scripts and keys also included in mapScripts and mapKeys? Or do you mean like witness derivatives of "normal" keys?

ryanofsky

utACK 656fde5, only change since last review is the new script metadata commit.

laanwj · 2017-12-21T12:03:19Z

utACK 656fde5

Thanks for adding release notes!

656fde5 Add script birthtime metadata to dump and import wallet (MeshCollider) 1bab9b2 Add script dump note to RPC help text and release notes (MeshCollider) 68c1e00 Add test for importwallet (MeshCollider) 9e1184d Add dumpwallet scripts test (MeshCollider) ef0c730 Add scripts to importwallet RPC (MeshCollider) b702ae8 Add CScripts to dumpwallet RPC (MeshCollider) cdc260a Add GetCScripts to CBasicKeyStore (MeshCollider) Pull request description: As discussed in #11289 (comment), adds the CScripts from the wallet to the `dumpwallet` RPC and then allows them to be imported with the `importwallet` RPC. Includes a basic test, and modifies the helptext of the dumpwallet RPC. Notes: - Reviewers: use `?w=1` to avoid the indentation-only change in commit `Add scripts to importwallet RPC ` - currently the scripts are followed with `# addr=` comments just as the other keys are, unsure if this might confuse users into thinking all the scripts are for valid P2SH addresses though, but I don't think that should be an issue. - there are no birthtimes for scripts, so script imports don't affect rescans - `importwallet` imports the CScripts but I'm not sure how to approach specifying whether scripts are for P2SH addresses, BIP173 addresses, etc. whether that matters or not. Otherwise the RPC helptext might just need modification. Fixes #11715 Tree-SHA512: 36c55837b3a58b9d3499d4c0c2ae82153d62aa71919e751574651b63a1d2b8ecc83796db4553cc65dad9b5341c3a42ae2fcf4d62598c30af267f8e1461ba8272

Zcash: Excludes changes to importwallet (missing bitcoin/bitcoin#11667) and ProcessImport (missing bitcoin/bitcoin#7551).

Bitcoin wallet PRs 3 Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#7687 - bitcoin/bitcoin#11116 - Excludes SegWit tests - Excludes `isInvalid` code (which is part of the SegWit support structure) - bitcoin/bitcoin#13002 - Excludes changes to `importwallet` (missing bitcoin/bitcoin#11667) - Excludes changes to `ProcessImport` (missing bitcoin/bitcoin#7551) - Third commit was rewritten to add an internal SigVersion argument (which we didn't have because it was added to support SegWit logic).

656fde5 Add script birthtime metadata to dump and import wallet (MeshCollider) 1bab9b2 Add script dump note to RPC help text and release notes (MeshCollider) 68c1e00 Add test for importwallet (MeshCollider) 9e1184d Add dumpwallet scripts test (MeshCollider) ef0c730 Add scripts to importwallet RPC (MeshCollider) b702ae8 Add CScripts to dumpwallet RPC (MeshCollider) cdc260a Add GetCScripts to CBasicKeyStore (MeshCollider) Pull request description: As discussed in bitcoin#11289 (comment), adds the CScripts from the wallet to the `dumpwallet` RPC and then allows them to be imported with the `importwallet` RPC. Includes a basic test, and modifies the helptext of the dumpwallet RPC. Notes: - Reviewers: use `?w=1` to avoid the indentation-only change in commit `Add scripts to importwallet RPC ` - currently the scripts are followed with `# addr=` comments just as the other keys are, unsure if this might confuse users into thinking all the scripts are for valid P2SH addresses though, but I don't think that should be an issue. - there are no birthtimes for scripts, so script imports don't affect rescans - `importwallet` imports the CScripts but I'm not sure how to approach specifying whether scripts are for P2SH addresses, BIP173 addresses, etc. whether that matters or not. Otherwise the RPC helptext might just need modification. Fixes bitcoin#11715 Tree-SHA512: 36c55837b3a58b9d3499d4c0c2ae82153d62aa71919e751574651b63a1d2b8ecc83796db4553cc65dad9b5341c3a42ae2fcf4d62598c30af267f8e1461ba8272

fanquake added the RPC/REST/ZMQ label Nov 12, 2017

promag reviewed Nov 15, 2017

View reviewed changes

meshcollider mentioned this pull request Nov 18, 2017

dumpwallet problem #11715

Closed

ryanofsky reviewed Dec 18, 2017

View reviewed changes

meshcollider added 7 commits December 20, 2017 18:47

Add GetCScripts to CBasicKeyStore

cdc260a

Add CScripts to dumpwallet RPC

b702ae8

Add scripts to importwallet RPC

ef0c730

Add dumpwallet scripts test

9e1184d

Add test for importwallet

68c1e00

Add script dump note to RPC help text and release notes

1bab9b2

Add script birthtime metadata to dump and import wallet

656fde5

ryanofsky reviewed Dec 20, 2017

View reviewed changes

laanwj merged commit 656fde5 into bitcoin:master Dec 21, 2017

meshcollider deleted the 201710_dumpwallet_scripts branch December 21, 2017 19:40

str4d pushed a commit to str4d/zcash that referenced this pull request Dec 19, 2019

Make CScript -> CScriptID conversion explicit

df00e51

Zcash: Excludes changes to importwallet (missing bitcoin/bitcoin#11667) and ProcessImport (missing bitcoin/bitcoin#7551).

str4d mentioned this pull request Dec 19, 2019

Bitcoin wallet PRs 3 zcash/zcash#4271

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add scripts to dumpwallet RPC #11667

Add scripts to dumpwallet RPC #11667

meshcollider commented Nov 12, 2017 •

edited

meshcollider commented Nov 12, 2017

laanwj commented Nov 13, 2017

jonasschnelli commented Nov 13, 2017

meshcollider commented Nov 13, 2017

promag left a comment

promag Nov 15, 2017

meshcollider Nov 15, 2017 •

edited

promag Nov 15, 2017

promag Nov 15, 2017

meshcollider commented Nov 15, 2017

meshcollider commented Nov 15, 2017

promag commented Nov 15, 2017 •

edited

ryanofsky left a comment

ryanofsky Dec 18, 2017

ryanofsky Dec 18, 2017 •

edited

ryanofsky Dec 20, 2017

meshcollider commented Dec 20, 2017 •

edited

ryanofsky left a comment

laanwj commented Dec 21, 2017 •

edited

	key_label, comment = line.split("#")
	# key = key_label.split(" ")[0]
	keytype = key_label.split(" ")[2]
	if len(comment) > 1:
	addr_keypath = comment.split(" addr=")[1]
	addr = addr_keypath.split(" ")[0]
	keypath = None

Add scripts to dumpwallet RPC #11667

Add scripts to dumpwallet RPC #11667

Conversation

meshcollider commented Nov 12, 2017 • edited

meshcollider commented Nov 12, 2017

laanwj commented Nov 13, 2017

jonasschnelli commented Nov 13, 2017

meshcollider commented Nov 13, 2017

promag left a comment

Choose a reason for hiding this comment

promag Nov 15, 2017

Choose a reason for hiding this comment

meshcollider Nov 15, 2017 • edited

Choose a reason for hiding this comment

promag Nov 15, 2017

Choose a reason for hiding this comment

promag Nov 15, 2017

Choose a reason for hiding this comment

meshcollider commented Nov 15, 2017

meshcollider commented Nov 15, 2017

promag commented Nov 15, 2017 • edited

ryanofsky left a comment

Choose a reason for hiding this comment

ryanofsky Dec 18, 2017

Choose a reason for hiding this comment

ryanofsky Dec 18, 2017 • edited

Choose a reason for hiding this comment

ryanofsky Dec 20, 2017

Choose a reason for hiding this comment

meshcollider commented Dec 20, 2017 • edited

ryanofsky left a comment

Choose a reason for hiding this comment

laanwj commented Dec 21, 2017 • edited

meshcollider commented Nov 12, 2017 •

edited

meshcollider Nov 15, 2017 •

edited

promag commented Nov 15, 2017 •

edited

ryanofsky Dec 18, 2017 •

edited

meshcollider commented Dec 20, 2017 •

edited

laanwj commented Dec 21, 2017 •

edited