Add scripts to dumpwallet RPC #11667

pull meshcollider wants to merge 7 commits into bitcoin:master from meshcollider:201710_dumpwallet_scripts changing 5 files +123 −40
  1. 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

  2. fanquake added the label RPC/REST/ZMQ on Nov 12, 2017
  3. 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

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

  5. 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?

  6. 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?

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

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

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

  10. 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);

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

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

  13. meshcollider cross-referenced this on Nov 18, 2017 from issue dumpwallet problem by khelle
  14. 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.

  15. ryanofsky commented at 2:49 PM on December 18, 2017: contributor

    utACK 468771372c96378268272971212943e7b46596cb

  16. Add GetCScripts to CBasicKeyStore cdc260afd5
  17. Add CScripts to dumpwallet RPC b702ae812c
  18. Add scripts to importwallet RPC ef0c730220
  19. Add dumpwallet scripts test 9e1184dd54
  20. Add test for importwallet 68c1e00a00
  21. Add script dump note to RPC help text and release notes 1bab9b23af
  22. Add script birthtime metadata to dump and import wallet 656fde53a3
  23. 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?

  24. ryanofsky commented at 9:57 PM on December 20, 2017: contributor

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

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

    utACK 656fde53a3a0d88a1e3c1aef7ae99083e4b06a7d

    Thanks for adding release notes!

  26. laanwj merged this on Dec 21, 2017
  27. laanwj closed this on Dec 21, 2017
  28. laanwj referenced this in commit 711d16ca4a on Dec 21, 2017
  29. meshcollider deleted the branch on Dec 21, 2017
  30. str4d referenced this in commit 33293c7586 on Dec 6, 2019
  31. str4d referenced this in commit 6e8f11cde0 on Dec 9, 2019
  32. str4d referenced this in commit df00e51e3e on Dec 19, 2019
  33. str4d cross-referenced this on Dec 19, 2019 from issue Bitcoin wallet PRs 3 by str4d
  34. zkbot referenced this in commit 900ed4555f on Dec 19, 2019
  35. PastaPastaPasta referenced this in commit 3832d7201d on Mar 23, 2020
  36. PastaPastaPasta referenced this in commit 48829094b5 on Mar 28, 2020
  37. PastaPastaPasta referenced this in commit cea5ee4125 on Mar 29, 2020
  38. PastaPastaPasta referenced this in commit 7b8095c2a9 on Mar 31, 2020
  39. UdjinM6 referenced this in commit e492322f12 on Mar 31, 2020
  40. PastaPastaPasta referenced this in commit 02ab2efe4a on Apr 1, 2020
  41. ckti referenced this in commit 8c8486e21b on Mar 28, 2021
  42. gades referenced this in commit 8dd4f6f99b on Jun 30, 2021
  43. bitcoin locked this on Sep 8, 2021
Contributors
meshcolliderlaanwjjonasschnellipromagryanofsky
Labels
RPC/REST/ZMQ
Linked (view graph)
#11289 Add wallet backup text to import* and add* RPCs#11715 dumpwallet problem
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:55 UTC