rpc: Enable wallet import on pruned nodes #16037

promag · 2019-05-16T18:37:41Z

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.

src/wallet/rpcdump.cpp

gmaxwell · 2019-05-16T18:55:50Z

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 · 2019-05-16T19:31:56Z

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

maflcko · 2019-05-16T19:40:35Z

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 · 2019-05-17T14:31:18Z

Updated to include the same check in importmulti.

maflcko · 2019-05-17T14:54:32Z

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 · 2019-05-17T17:35:44Z

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

Now also checking before importing on importmulti.

maflcko

utACK 9066b21

Good stuff

src/wallet/rpcdump.cpp

promag · 2019-05-17T21:14:02Z

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 · 2019-06-28T05:18:03Z

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.

Sjors · 2019-08-06T14:56:27Z

Concept ACK. Travis unhappy.

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

promag · 2019-08-07T00:02:50Z

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

luke-jr · 2019-08-19T21:55:49Z

src/wallet/rpcdump.cpp

+static void EnsureBlockDataFromTime(interfaces::Chain::Lock& locked_chain, int64_t timestamp)
+{
+    const Optional<int> height = locked_chain.findFirstBlockWithTimeAndHeight(timestamp - TIMESTAMP_WINDOW, 0, nullptr);
+    if (height && !locked_chain.haveBlockOnDisk(*height)) {


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

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

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.

luke-jr · 2019-08-19T22:00:26Z

src/wallet/rpcdump.cpp

                scripts.push_back(std::pair<CScript, int64_t>(script, birth_time));
            }
        }
        file.close();
+        if (pwallet->chain().havePruned()) {


Should just check this inside EnsureBlockDataFromTime

DrahtBot · 2019-10-28T14:39:24Z

Needs rebase

fanquake · 2021-03-15T02:08:20Z

src/wallet/rpcdump.cpp

@@ -101,6 +101,14 @@ static void RescanWallet(CWallet& wallet, const WalletRescanReserver& reserver,
    }
 }

+static void EnsureBlockDataFromTime(interfaces::Chain::Lock& locked_chain, int64_t timestamp)
+{
+    const Optional<int> height = locked_chain.findFirstBlockWithTimeAndHeight(timestamp - TIMESTAMP_WINDOW, 0, nullptr);


Please use std::optional in new code.

DrahtBot · 2021-12-15T11:22:28Z

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.

DrahtBot · 2022-03-21T13:07:51Z

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.

maflcko · 2022-03-21T13:49:39Z

Marked "up for grabs"

fanquake · 2022-04-26T12:19:31Z

Closing given #24865 is open.

564b580 test: Introduce MIN_BLOCKS_TO_KEEP constant (Aurèle Oulès) 71d9a7c test: Wallet imports on pruned nodes (Aurèle Oulès) e6906fc rpc: Enable wallet import on pruned nodes (Aurèle Oulès) Pull request description: Reopens #16037 I have rebased the PR, addressed the comments of the original PR and added a functional test. > 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. For reviewers: `python test/functional/wallet_pruning.py --nocleanup` will generate a large blockchain (~700MB) that can be used to manually test wallet imports on a pruned node. Node0 is not pruned, while node1 is. ACKs for top commit: kouloumos: ACK 564b580 achow101: reACK 564b580 furszy: ACK 564b580 w0xlt: ACK 564b580 Tree-SHA512: b345a6c455fcb6581cdaa5f7a55d79e763a55cb08c81d66be5b12794985d79cd51b9b39bdcd0f7ba0a2a2643e9b2ddc49310ff03d16b430df2f74e990800eabf

laanwj added RPC/REST/ZMQ Wallet labels May 16, 2019

laanwj reviewed May 16, 2019

View reviewed changes

src/wallet/rpcdump.cpp Outdated Show resolved Hide resolved

promag force-pushed the 2019-05-importwallet-pruned branch from 90101d2 to 1b35d83 Compare May 16, 2019 22:52

promag changed the title ~~rpc: Fail importwallet only if a required block is pruned~~ rpc: Early fail import(wallet,multi) when a required block is pruned May 17, 2019

promag force-pushed the 2019-05-importwallet-pruned branch from 1b35d83 to 4a2285f Compare May 17, 2019 14:29

promag force-pushed the 2019-05-importwallet-pruned branch from 4a2285f to 9066b21 Compare May 17, 2019 17:34

maflcko reviewed May 17, 2019

View reviewed changes

src/wallet/rpcdump.cpp Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Jul 26, 2019

promag force-pushed the 2019-05-importwallet-pruned branch from 9066b21 to 6cd0add Compare July 28, 2019 22:11

DrahtBot removed the Needs rebase label Jul 29, 2019

promag added 4 commits August 6, 2019 16:01

rpc: Compute nLowestTimestamp on the first iteration in importmulti

23cf377

rpc: Compute nTimeBegin on the first iteration in importwallet

db3256e

rpc: Add EnsureBlockDataFromTime utility

0791e64

rpc: Enable wallet import on pruned nodes

cffd615

promag force-pushed the 2019-05-importwallet-pruned branch from 6cd0add to cffd615 Compare August 6, 2019 16:02

promag changed the title ~~rpc: Early fail import(wallet,multi) when a required block is pruned~~ rpc: Enable wallet import on pruned nodes Aug 6, 2019

luke-jr suggested changes Aug 19, 2019

View reviewed changes

laanwj added the Feature label Sep 30, 2019

promag mentioned this pull request Oct 15, 2019

Remove wallet access to some node arguments #17138

Merged

DrahtBot added the Needs rebase label Oct 28, 2019

fanquake reviewed Mar 15, 2021

View reviewed changes

maflcko added the Up for grabs label Mar 21, 2022

aureleoules mentioned this pull request Apr 15, 2022

rpc: Enable wallet import on pruned nodes and add test #24865

Merged

fanquake closed this Apr 26, 2022

fanquake removed Up for grabs Needs rebase labels Apr 26, 2022

bitcoin locked and limited conversation to collaborators Apr 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rpc: Enable wallet import on pruned nodes #16037

rpc: Enable wallet import on pruned nodes #16037

promag commented May 16, 2019

gmaxwell commented May 16, 2019

promag commented May 16, 2019 •

edited

Loading

maflcko commented May 16, 2019 •

edited

Loading

promag commented May 17, 2019

maflcko commented May 17, 2019

promag commented May 17, 2019

maflcko left a comment

promag commented May 17, 2019

DrahtBot commented Jun 28, 2019 •

edited

Loading

Sjors commented Aug 6, 2019

promag commented Aug 7, 2019

luke-jr Aug 19, 2019

promag Sep 9, 2019 •

edited

Loading

luke-jr Nov 4, 2019

luke-jr Aug 19, 2019

DrahtBot commented Oct 28, 2019

fanquake Mar 15, 2021

DrahtBot commented Dec 15, 2021

DrahtBot commented Mar 21, 2022

maflcko commented Mar 21, 2022

fanquake commented Apr 26, 2022

rpc: Enable wallet import on pruned nodes #16037

rpc: Enable wallet import on pruned nodes #16037

Conversation

promag commented May 16, 2019

gmaxwell commented May 16, 2019

promag commented May 16, 2019 • edited Loading

maflcko commented May 16, 2019 • edited Loading

promag commented May 17, 2019

maflcko commented May 17, 2019

promag commented May 17, 2019

maflcko left a comment

Choose a reason for hiding this comment

promag commented May 17, 2019

DrahtBot commented Jun 28, 2019 • edited Loading

Conflicts

Sjors commented Aug 6, 2019

promag commented Aug 7, 2019

luke-jr Aug 19, 2019

Choose a reason for hiding this comment

promag Sep 9, 2019 • edited Loading

Choose a reason for hiding this comment

luke-jr Nov 4, 2019

Choose a reason for hiding this comment

luke-jr Aug 19, 2019

Choose a reason for hiding this comment

DrahtBot commented Oct 28, 2019

fanquake Mar 15, 2021

Choose a reason for hiding this comment

DrahtBot commented Dec 15, 2021

DrahtBot commented Mar 21, 2022

maflcko commented Mar 21, 2022

fanquake commented Apr 26, 2022

promag commented May 16, 2019 •

edited

Loading

maflcko commented May 16, 2019 •

edited

Loading

DrahtBot commented Jun 28, 2019 •

edited

Loading

promag Sep 9, 2019 •

edited

Loading