rpc: Enable wallet import on pruned nodes

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.

laanwj added the label RPC/REST/ZMQ on May 16, 2019

laanwj added the label Wallet on May 16, 2019

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.

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!

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?

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.

promag force-pushed on May 16, 2019

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

promag force-pushed on May 17, 2019

promag commented at 2:31 pm on May 17, 2019: member

Updated to include the same check in importmulti.

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.

promag force-pushed on May 17, 2019

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.

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.

MarcoFalke commented at 5:51 pm on May 17, 2019: member

utACK 9066b21cc45fdd9749d6e2d461d125e64182c963

Good stuff

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?

DrahtBot commented at 5:18 am on June 28, 2019: 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:

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

DrahtBot added the label Needs rebase on Jul 26, 2019

promag force-pushed on Jul 28, 2019

DrahtBot removed the label Needs rebase on Jul 29, 2019

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”

rpc: Compute nLowestTimestamp on the first iteration in importmulti 23cf377170

rpc: Compute nTimeBegin on the first iteration in importwallet db3256e35a

rpc: Add EnsureBlockDataFromTime utility 0791e6488e

rpc: Enable wallet import on pruned nodes cffd615a1b

promag force-pushed on Aug 6, 2019

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

promag commented at 0:02 am on August 7, 2019: member

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

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.

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

luke-jr changes_requested

laanwj added the label Feature on Sep 30, 2019

DrahtBot added the label Needs rebase on Oct 28, 2019

DrahtBot commented at 2:39 pm on October 28, 2019: member

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.

DrahtBot commented at 11:22 am on December 15, 2021: member

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.

DrahtBot commented at 1:07 pm on March 21, 2022: member

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.

MarcoFalke added the label Up for grabs on Mar 21, 2022

MarcoFalke commented at 1:49 pm on March 21, 2022: member

Marked “up for grabs”

fanquake commented at 12:19 pm on April 26, 2022: member

Closing given #24865 is open.

fanquake closed this on Apr 26, 2022

fanquake removed the label Up for grabs on Apr 26, 2022

fanquake removed the label Needs rebase on Apr 26, 2022

achow101 referenced this in commit 66c08e741d on Dec 16, 2022

DrahtBot locked this on Apr 26, 2023

rpc: Enable wallet import on pruned nodes #16037

Conflicts