Add scripts to dumpwallet RPC #11667

meshcollider commented at 6:07 AM on November 12, 2017: contributor

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

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

meshcollider commented at 6:16 AM on November 12, 2017: contributor

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

laanwj commented at 1:35 PM on November 13, 2017: member

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

jonasschnelli commented at 8:34 PM on November 13, 2017: contributor

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 commented at 9:12 PM on November 13, 2017: contributor

@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?

in src/wallet/rpcdump.cpp:728 in 1e066dbdf7 outdated

 707 | @@ -694,6 +708,15 @@ UniValue dumpwallet(const JSONRPCRequest& request)
 708 |          }
 709 |      }
 710 |      file << "\n";
 711 | +    for (const CScriptID &scriptid : scripts) {
 712 | +        CScript script;
 713 | +        std::string address = EncodeDestination(scriptid);
 714 | +        if(pwallet->GetCScript(scriptid, script)) {
 715 | +            file << strprintf("%s 0 script=1", HexStr(script.begin(), script.end()));
 716 | +            file << strprintf(" # addr=%s\n", address);

promag commented at 10:27 AM on November 15, 2017:

We don't test the comment?

meshcollider commented at 10:35 AM on November 15, 2017:

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

promag commented at 10:48 AM on November 15, 2017:

Nevermind, address is parsed in https://github.com/bitcoin/bitcoin/blob/4db82b7aab4ad64717f742a7318e3dc6811b41be/test/functional/wallet-dump.py#L27-L33

in doc/release-notes.md:94 in 1e066dbdf7 outdated

  88 | @@ -89,6 +89,10 @@ Low-level RPC changes
  89 |  - `dumpwallet` no longer allows overwriting files. This is a security measure
  90 |    as well as prevents dangerous user mistakes.
  91 |  
  92 | +- `dumpwallet` now includes hex-encoded scripts from the wallet in the dumpfile, and
  93 | +  `importwallet` now imports these scripts, but corresponding addresses may not be added
  94 | +  correctly or a manual rescan may be required to find transactions.

promag commented at 10:29 AM on November 15, 2017:

to find relevant transactions or something along that.

promag commented at 10:30 AM on November 15, 2017: member

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.

meshcollider commented at 10:39 AM on November 15, 2017: contributor

@promag

cs_wallet should be locked in dumpwallet?

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

meshcollider commented at 10:50 AM on November 15, 2017: contributor

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 commented at 11:45 AM on November 15, 2017: member

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.

in src/wallet/rpcdump.cpp:717 in e56e5d22c3 outdated

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

ryanofsky commented at 2:37 PM on December 18, 2017:

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.

ryanofsky commented at 2:42 PM on December 18, 2017:

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

ryanofsky commented at 9:49 PM on December 20, 2017:

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.

ryanofsky commented at 2:49 PM on December 18, 2017: member

utACK 468771372c96378268272971212943e7b46596cb

Add GetCScripts to CBasicKeyStore cdc260afd5

Add CScripts to dumpwallet RPC b702ae812c

Add scripts to importwallet RPC ef0c730220

Add dumpwallet scripts test 9e1184dd54

Add test for importwallet 68c1e00a00

Add script dump note to RPC help text and release notes 1bab9b23af

Add script birthtime metadata to dump and import wallet 656fde53a3

meshcollider commented at 9:21 PM on December 20, 2017: contributor

@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 commented at 9:57 PM on December 20, 2017: member

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

laanwj commented at 12:03 PM on December 21, 2017: member

utACK 656fde53a3a0d88a1e3c1aef7ae99083e4b06a7d

Thanks for adding release notes!

laanwj merged this on Dec 21, 2017

laanwj closed this on Dec 21, 2017

laanwj referenced this in commit 711d16ca4a on Dec 21, 2017

meshcollider deleted the branch on Dec 21, 2017

str4d referenced this in commit 33293c7586 on Dec 6, 2019

str4d referenced this in commit 6e8f11cde0 on Dec 9, 2019

str4d referenced this in commit df00e51e3e on Dec 19, 2019

zkbot referenced this in commit 900ed4555f on Dec 19, 2019

PastaPastaPasta referenced this in commit 3832d7201d on Mar 23, 2020

PastaPastaPasta referenced this in commit 48829094b5 on Mar 28, 2020

PastaPastaPasta referenced this in commit cea5ee4125 on Mar 29, 2020

PastaPastaPasta referenced this in commit 7b8095c2a9 on Mar 31, 2020

UdjinM6 referenced this in commit e492322f12 on Mar 31, 2020

PastaPastaPasta referenced this in commit 02ab2efe4a on Apr 1, 2020

ckti referenced this in commit 8c8486e21b on Mar 28, 2021

gades referenced this in commit 8dd4f6f99b on Jun 30, 2021

DrahtBot locked this on Sep 8, 2021