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

aureleoules · 2022-04-15T15:38:30Z

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.

test/functional/wallet_pruning.py

DrahtBot · 2022-04-16T06:33:39Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	kouloumos, furszy, w0xlt, achow101
Concept ACK	Xekyo, theStack
Stale ACK	ryanofsky

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

murchandamus

Did a light code-review, concept ACK

test/functional/wallet_pruning.py

src/wallet/rpc/backup.cpp

kouloumos

I think that wallet_pruning.py is not testing properly this added feature. This is due to the testing logic but mainly because of the default setup of the testing framework that is being used in this test.

This is possible because the dump format includes key timestamps.

The importwallet rpc calculates the min nTimeBegin timestamp for which findFirstBlockWithTimeAndHeight in the newly created function EnsureBlockDataFromTime will find the earliest block with timestamp equal or greater to.

As you can verify from the wallet_*.dat dumps of this test

# node1 wallet dump
...
cUxsWyKyZ9MAQTaAhUQWJmBbSvHMwSmuv59KgxQV7oZQU3PXN3KE 1970-01-01T00:00:01Z label=coinbase # addr=msX6jQXvxiNhx3Q62PKeLPrhrqZQdSimTg,2MtBq31Chq2pntJ81PQLR4q4sMEG7K46wsQ,bcrt1qsw5g6ehh439vurfyhdh93d66hw0kf9085wy7ma
...

the deterministic private keys that are imported to each test node as part of the testing framework setup

bitcoin/test/functional/test_framework/test_framework.py

Line 441 in 1d89fc6

    
           n.importprivkey(privkey=n.get_deterministic_priv_key().key, label='coinbase', rescan=True)

have a 1970-01-01T00:00:01Z timestamp which means that EnsureBlockDataFromTime will always check for the existence of all blocks between genesis and the current height.

Even if that was not the case, because the nodes' wallets are initialized before we start generating blocks, the timestamp in the wallet dump is still before the first created block thus EnsureBlockDataFromTime checks for the existence of all blocks between the first block and the current height.

This behaviour will always result to missing blocks as soon as we start pruning. The reason that the first part of the test (wallet_import_pruned_test) is not failing is because it never reaches the prune state (height < PruntAfterHeight).

I believe that you can better test for the new feature by overriding setup_nodes and initializing node1 wallet mid-test

test/functional/wallet_pruning.py

aureleoules · 2022-07-20T07:25:56Z

Thank you for your thorough review @kouloumos !
I believe I have addressed all of your comments.

src/wallet/rpc/backup.cpp

kouloumos · 2022-12-07T16:48:53Z

It also runs as part of Win64 native [vs2022]. I can see that the extended tests run there and wallet_pruning.py runs successfully.

maflcko · 2022-12-07T16:50:39Z

Ah thx. Maybe there should be a skip_if_not_running_extended 😅 ?

Also, I think this needs a rebase, see upcoming CI failure in https://cirrus-ci.com/task/6513158118965248

Co-authored-by: João Barbosa <[email protected]> Co-authored-by: Ryan Ofsky <[email protected]>

aureleoules · 2022-12-08T12:53:34Z

Rebased to address silent conflicts.

achow101 · 2022-12-14T22:10:55Z

ACK 63493d778f443554059d77bd8c9b1a926685a892

test/functional/wallet_pruning.py

Co-authored-by: Ryan Ofsky <[email protected]> Co-authored-by: Andreas Kouloumos <[email protected]>

aureleoules · 2022-12-15T08:54:14Z

Thanks @kouloumos I addressed your comments.

kouloumos · 2022-12-15T08:58:10Z

ACK 564b580

furszy

ACK 564b580
left few non-blocking nits.

furszy · 2022-12-15T18:54:58Z

src/wallet/rpc/backup.cpp

+    }
+
+    int height{0};
+    const bool found{chain.findFirstBlockWithTimeAndHeight(timestamp - TIMESTAMP_WINDOW, 0, FoundBlock().height(height))};


In e6906fc:

tiny nit:
(only if you have to re-touch)

Suggested change

const bool found{chain.findFirstBlockWithTimeAndHeight(timestamp - TIMESTAMP_WINDOW, 0, FoundBlock().height(height))};

const bool found{chain.findFirstBlockWithTimeAndHeight(timestamp - TIMESTAMP_WINDOW, /*min_height=*/ 0, FoundBlock().height(height))};

furszy · 2022-12-15T19:06:45Z

test/functional/test_framework/blocktools.py

    if pubkey is not None:
        coinbaseoutput.scriptPubKey = key_to_p2pk_script(pubkey)
+    elif script_pubkey is not None:
+        coinbaseoutput.scriptPubKey = script_pubkey


In 71d9a7c:

atm is just a non-blocking nit:

it's weird to add this new ´script_pubkey´ arg that is only set if the ´pubkey´ arg is not provided. It is not stated anywhere.

Ideally, should unify both into a single arg. So there is no confusion over which of them take precedence over the other.

furszy · 2022-12-15T19:09:08Z

test/functional/wallet_pruning.py

+            # Important for matching a timestamp with a block +- some window
+            self.nTime += 600
+            for n in self.nodes:
+                if n.running:


In 71d9a7c:
why the node would not be running?

Right, in a previous version of the test it could crash without this check.

w0xlt

ACK 564b580

achow101 · 2022-12-16T22:19:12Z

reACK 564b580

…d test

maflcko · 2022-12-19T17:06:04Z

test/functional/wallet_pruning.py

+        previousblockhash = int(best_block["hash"], 16)
+        big_script = CScript([OP_RETURN] + [OP_TRUE] * 950000)
+        for _ in range(n):
+            block = create_block(hashprev=previousblockhash, ntime=self.nTime, coinbase=create_coinbase(height, script_pubkey=big_script))


nit: It is possible to do this without blocktools. See

diff --git a/test/functional/wallet_pruning.py b/test/functional/wallet_pruning.py index 6d8475ce8d..d768130776 100755 --- a/test/functional/wallet_pruning.py +++ b/test/functional/wallet_pruning.py @@ -40,20 +40,10 @@ class WalletPruningTest(BitcoinTestFramework): def mine_large_blocks(self, node, n): # Get the block parameters for the first block best_block = node.getblock(node.getbestblockhash()) - height = int(best_block["height"]) + 1 self.nTime = max(self.nTime, int(best_block["time"])) + 1 - previousblockhash = int(best_block["hash"], 16) - big_script = CScript([OP_RETURN] + [OP_TRUE] * 950000) + big_script = "raw({})".format(bytes([OP_RETURN] + [OP_TRUE] * 950000).hex()) for _ in range(n): - block = create_block(hashprev=previousblockhash, ntime=self.nTime, coinbase=create_coinbase(height, script_pubkey=big_script)) - block.solve() - - # Submit to the node - node.submitblock(block.serialize().hex()) - - previousblockhash = block.sha256 - height += 1 - + self.generateblock(node, big_script, []) # Simulate 10 minutes of work time per block # Important for matching a timestamp with a block +- some window self.nTime += 600

Also, might be good to double check the runtime in light of 6c872d5

DrahtBot added RPC/REST/ZMQ Wallet labels Apr 15, 2022

vincenzopalazzo suggested changes Apr 15, 2022

View reviewed changes

test/functional/wallet_pruning.py Outdated Show resolved Hide resolved

test/functional/wallet_pruning.py Outdated Show resolved Hide resolved

aureleoules force-pushed the 2022-04-importwallet-pruned branch from 3543290 to 5dabfed Compare April 16, 2022 12:49

DrahtBot mentioned this pull request Apr 16, 2022

Improve Indices on pruned nodes via prune blockers #21726

Merged

fanquake mentioned this pull request Apr 26, 2022

rpc: Enable wallet import on pruned nodes #16037

Closed

DrahtBot added the Needs rebase label Apr 26, 2022

aureleoules force-pushed the 2022-04-importwallet-pruned branch from 5dabfed to f8aa5b0 Compare April 26, 2022 22:20

DrahtBot removed the Needs rebase label Apr 26, 2022

aureleoules force-pushed the 2022-04-importwallet-pruned branch 2 times, most recently from bd36a43 to 46b7f73 Compare May 14, 2022 17:09

murchandamus reviewed May 31, 2022

View reviewed changes

test/functional/wallet_pruning.py Outdated Show resolved Hide resolved

test/functional/wallet_pruning.py Outdated Show resolved Hide resolved

achow101 reviewed Jun 20, 2022

View reviewed changes

src/wallet/rpc/backup.cpp Outdated Show resolved Hide resolved

aureleoules force-pushed the 2022-04-importwallet-pruned branch 3 times, most recently from 2b36593 to f76b309 Compare July 5, 2022 12:11

aureleoules mentioned this pull request Jul 5, 2022

test: Move helper function create_large_block #25546

Closed

aureleoules force-pushed the 2022-04-importwallet-pruned branch 5 times, most recently from 926f40c to b45e54b Compare July 5, 2022 16:50

kouloumos reviewed Jul 13, 2022

View reviewed changes

aureleoules force-pushed the 2022-04-importwallet-pruned branch 2 times, most recently from a8d8f89 to 45435bc Compare July 19, 2022 18:04

kouloumos reviewed Sep 1, 2022

View reviewed changes

src/wallet/rpc/backup.cpp Outdated Show resolved Hide resolved

aureleoules force-pushed the 2022-04-importwallet-pruned branch 2 times, most recently from ea532ba to 0aedf6f Compare September 6, 2022 10:32

aureleoules force-pushed the 2022-04-importwallet-pruned branch from 8e87433 to 25969f3 Compare December 7, 2022 20:37

rpc: Enable wallet import on pruned nodes

e6906fc

Co-authored-by: João Barbosa <[email protected]> Co-authored-by: Ryan Ofsky <[email protected]>

aureleoules force-pushed the 2022-04-importwallet-pruned branch from 25969f3 to 63493d7 Compare December 8, 2022 11:39

kouloumos reviewed Dec 15, 2022

View reviewed changes

test/functional/wallet_pruning.py Outdated Show resolved Hide resolved

test/functional/wallet_pruning.py Outdated Show resolved Hide resolved

aureleoules and others added 2 commits December 15, 2022 09:53

test: Wallet imports on pruned nodes

71d9a7c

Co-authored-by: Ryan Ofsky <[email protected]> Co-authored-by: Andreas Kouloumos <[email protected]>

test: Introduce MIN_BLOCKS_TO_KEEP constant

564b580

aureleoules force-pushed the 2022-04-importwallet-pruned branch from 63493d7 to 564b580 Compare December 15, 2022 08:54

aureleoules requested review from achow101, furszy, kouloumos, ryanofsky and w0xlt and removed request for furszy, kouloumos, ryanofsky and w0xlt December 15, 2022 08:54

furszy approved these changes Dec 15, 2022

View reviewed changes

w0xlt approved these changes Dec 16, 2022

View reviewed changes

achow101 merged commit 66c08e7 into bitcoin:master Dec 16, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 17, 2022

Merge bitcoin#24865: rpc: Enable wallet import on pruned nodes and ad…

59bb1c8

…d test

maflcko reviewed Dec 19, 2022

View reviewed changes

aureleoules deleted the 2022-04-importwallet-pruned branch January 12, 2023 11:51

bitcoin locked and limited conversation to collaborators Jan 12, 2024

	const bool found{chain.findFirstBlockWithTimeAndHeight(timestamp - TIMESTAMP_WINDOW, 0, FoundBlock().height(height))};
	const bool found{chain.findFirstBlockWithTimeAndHeight(timestamp - TIMESTAMP_WINDOW, /min_height=/ 0, FoundBlock().height(height))};

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

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

Uh oh!

Conversation

aureleoules commented Apr 15, 2022 • edited by achow101 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Apr 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

murchandamus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kouloumos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aureleoules commented Jul 20, 2022

Uh oh!

Uh oh!

kouloumos commented Dec 7, 2022

Uh oh!

maflcko commented Dec 7, 2022

Uh oh!

aureleoules commented Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Dec 14, 2022

Uh oh!

Uh oh!

Uh oh!

aureleoules commented Dec 15, 2022

Uh oh!

kouloumos commented Dec 15, 2022

Uh oh!

furszy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

furszy Dec 15, 2022

Choose a reason for hiding this comment

Uh oh!

furszy Dec 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

furszy Dec 15, 2022

Choose a reason for hiding this comment

Uh oh!

aureleoules Dec 16, 2022

Choose a reason for hiding this comment

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Dec 16, 2022

Uh oh!

maflcko Dec 19, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

aureleoules commented Apr 15, 2022 •

edited by achow101

Loading

DrahtBot commented Apr 16, 2022 •

edited

Loading

aureleoules commented Dec 8, 2022 •

edited

Loading

furszy left a comment •

edited

Loading

furszy Dec 15, 2022 •

edited

Loading