rpc: Enable wallet import on pruned nodes #16037

pull promag wants to merge 4 commits into bitcoin:master from promag:2019-05-importwallet-pruned changing 1 files +23 −21
  1. promag commented at 6:37 PM on May 16, 2019: member

    Before this change importwallet fails if any block is pruned. This PR makes it possible to importwallet if all required blocks aren't pruned. This is possible because the dump format includes key timestamps.

  2. laanwj added the label RPC/REST/ZMQ on May 16, 2019
  3. laanwj added the label Wallet on May 16, 2019
  4. in src/wallet/rpcdump.cpp:656 in 90101d2930 outdated 
     651 |              }
 652 |          }
 653 |          file.close();
 654 | +        if (pwallet->chain().havePruned()) {
 655 | +            Optional<int> pruned_height = locked_chain->findPruned();
 656 | +            if (pruned_height && nTimeBegin < locked_chain->getBlockTime(*pruned_height)) {
    laanwj commented at 6:53 PM on May 16, 2019:

    should there be some kind of grace period here, because block timestamps aren't exact?

    MarcoFalke commented at 7:09 PM on May 16, 2019:

    Yeah, just use the same grace period as is used for the wallet birthday

    promag commented at 7:13 PM on May 16, 2019:

    I was expecting this is wrong, will fix.

  5. gmaxwell commented at 6:55 PM on May 16, 2019: contributor

    The block lookup should be using findFirstBlockWithTime (see RescanFromTime) and allow some slop, because miners could have wrong clocks and users could have wrong clocks, TIMESTAMP_WINDOW, (again see RescanFromTime). This is particularly important for birthdays because installing the software and instantly generating a key, handing it out, and getting paid is a perfectly sensible sequence of events.

    Otherwise-- Concept ACK on doing this!

  6. promag commented at 7:31 PM on May 16, 2019: member

    I think the best place to test this is in feature_prunning.py, not wallet_dump.py, WDYT?

  7. MarcoFalke commented at 7:40 PM on May 16, 2019: member

    Jup. And ideally feature_pruning should run with and without the wallet compiled in, so you can either guard it in feature_pruning behind the test framework check for the wallet or add a new test wallet_pruning.

  8. promag force-pushed on May 16, 2019
  9. promag renamed this:
    rpc: Fail importwallet only if a required block is pruned
    rpc: Early fail import(wallet,multi) when a required block is pruned
    on May 17, 2019
  10. promag force-pushed on May 17, 2019
  11. promag commented at 2:31 PM on May 17, 2019: member

    Updated to include the same check in importmulti.

  12. MarcoFalke commented at 2:54 PM on May 17, 2019: member

    Updated to include the same check in importmulti.

    How does that change the error message? Also, the import will happen before the check in importmuli, whereas in importwallet it will happen after.

  13. promag force-pushed on May 17, 2019
  14. promag commented at 5:35 PM on May 17, 2019: member

    the import will happen before the check in importmuli, whereas in importwallet it will happen after.

    Now also checking before importing on importmulti.

  15. in src/wallet/rpcdump.cpp:1453 in 9066b21cc4 outdated 
    1449 | @@ -1446,21 +1450,27 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
1450 |          LOCK(pwallet->cs_wallet);
1451 |          EnsureWalletIsUnlocked(pwallet);
1452 |  
1453 | -        // Verify all timestamps are present before importing any keys.
1454 | +        const int64_t minimumTimestamp = 1;
    MarcoFalke commented at 5:51 PM on May 17, 2019:

    style-nit: Why is the comment and constant moved?

    promag commented at 10:11 PM on July 28, 2019:

    Reverted.

  16. MarcoFalke commented at 5:51 PM on May 17, 2019: member

    utACK 9066b21cc45fdd9749d6e2d461d125e64182c963

    Good stuff

  17. promag commented at 9:14 PM on May 17, 2019: member

    Thanks for the review. I'm going to test these changes and add a release note. I'm leaning towards a new test file like you suggested.

    BTW is there's a way to change the .blk maximum size?

  18. DrahtBot commented at 5:18 AM on June 28, 2019: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #17266 (util: Rename DecodeDumpTime to ParseISO8601DateTime by elichai)
    • #17261 (Make ScriptPubKeyMan an actual interface and the wallet to have multiple by achow101)
    • #17260 (Split some CWallet functions into new LegacyScriptPubKeyMan by achow101)
    • #16546 ([WIP] External signer support - Wallet Box edition by Sjors)
    • #16528 ([WIP] Native Descriptor Wallets (take 2) by achow101)

    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.

  19. DrahtBot added the label Needs rebase on Jul 26, 2019
  20. promag force-pushed on Jul 28, 2019
  21. DrahtBot removed the label Needs rebase on Jul 29, 2019
  22. Sjors commented at 2:56 PM on August 6, 2019: member

    Concept ACK. Travis unhappy.

    Suggest renaming the PR to "[rpc] enable wallet import on pruned nodes"

  23. rpc: Compute nLowestTimestamp on the first iteration in importmulti 23cf377170
  24. rpc: Compute nTimeBegin on the first iteration in importwallet db3256e35a
  25. rpc: Add EnsureBlockDataFromTime utility 0791e6488e
  26. rpc: Enable wallet import on pruned nodes cffd615a1b
  27. promag force-pushed on Aug 6, 2019
  28. promag renamed this:
    rpc: Early fail import(wallet,multi) when a required block is pruned
    rpc: Enable wallet import on pruned nodes
    on Aug 6, 2019
  29. promag commented at 12:02 AM on August 7, 2019: member

    @Sjors happy travis, and reworded like you suggested, thanks.

  30. in src/wallet/rpcdump.cpp:107 in cffd615a1b 
     100 | @@ -101,6 +101,14 @@ static void RescanWallet(CWallet& wallet, const WalletRescanReserver& reserver,
 101 |      }
 102 |  }
 103 |  
 104 | +static void EnsureBlockDataFromTime(interfaces::Chain::Lock& locked_chain, int64_t timestamp)
 105 | +{
 106 | +    const Optional<int> height = locked_chain.findFirstBlockWithTimeAndHeight(timestamp - TIMESTAMP_WINDOW, 0, nullptr);
 107 | +    if (height && !locked_chain.haveBlockOnDisk(*height)) {
    luke-jr commented at 9:55 PM on August 19, 2019:

    Shouldn't we fail if any blocks after this one are pruned too?

    promag commented at 11:50 AM on September 9, 2019:

    Is that possible? I mean, if block X is available then X+1 is also available?

    luke-jr commented at 1:12 PM on November 4, 2019:

    Blocks are pruned as entire files, so it's possible block X is in the same file as block X+2, and we don't want to prune block X+2, but we do want to prune block X+1. This can result in block X+1 being pruned before block X.

  31. in src/wallet/rpcdump.cpp:630 in cffd615a1b 
     625 | +                if (birth_time > 0) nTimeBegin = std::min(nTimeBegin, birth_time);
 626 |                  scripts.push_back(std::pair<CScript, int64_t>(script, birth_time));
 627 |              }
 628 |          }
 629 |          file.close();
 630 | +        if (pwallet->chain().havePruned()) {
    luke-jr commented at 10:00 PM on August 19, 2019:

    Should just check this inside EnsureBlockDataFromTime

  32. luke-jr changes_requested
  33. laanwj added the label Feature on Sep 30, 2019
  34. DrahtBot added the label Needs rebase on Oct 28, 2019
  35. DrahtBot commented at 2:39 PM on October 28, 2019: member

    <!--cf906140f33d8803c4a75a2196329ecb-->Needs rebase

  36. in src/wallet/rpcdump.cpp:106 in cffd615a1b 
     100 | @@ -101,6 +101,14 @@ static void RescanWallet(CWallet& wallet, const WalletRescanReserver& reserver,
 101 |      }
 102 |  }
 103 |  
 104 | +static void EnsureBlockDataFromTime(interfaces::Chain::Lock& locked_chain, int64_t timestamp)
 105 | +{
 106 | +    const Optional<int> height = locked_chain.findFirstBlockWithTimeAndHeight(timestamp - TIMESTAMP_WINDOW, 0, nullptr);
    fanquake commented at 2:08 AM on March 15, 2021:

    Please use std::optional in new code.

  37. DrahtBot commented at 11:22 AM on December 15, 2021: member

    <!--13523179cfe9479db18ec6c5d236f789-->There hasn't been much activity lately and the patch still needs rebase. What is the status here?

    • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
    • Is it no longer relevant? ➡️ Please close.
    • Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.
  38. DrahtBot commented at 1:07 PM on March 21, 2022: member

    <!--13523179cfe9479db18ec6c5d236f789-->There hasn't been much activity lately and the patch still needs rebase. What is the status here?

    • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
    • Is it no longer relevant? ➡️ Please close.
    • Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.
  39. MarcoFalke added the label Up for grabs on Mar 21, 2022
  40. MarcoFalke commented at 1:49 PM on March 21, 2022: member

    Marked "up for grabs"

  41. fanquake commented at 12:19 PM on April 26, 2022: member

    Closing given #24865 is open.

  42. fanquake closed this on Apr 26, 2022
  43. fanquake removed the label Up for grabs on Apr 26, 2022
  44. fanquake removed the label Needs rebase on Apr 26, 2022
  45. achow101 referenced this in commit 66c08e741d on Dec 16, 2022
  46. DrahtBot locked this on Apr 26, 2023
Contributors
promaglaanwjMarcoFalkegmaxwellDrahtBotSjorsluke-jrfanquake
Labels
FeatureWalletRPC/REST/ZMQ
Linked (view graph)
#24865 rpc: Enable wallet import on pruned nodes and add test
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-02 12:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me