This PR adds tests for the newly merged upgradewallet RPC.
Additionally, it expands test_framework/util.py by adding the function adjust_bitcoin_conf_for_pre_17 to support nodes that don’t parse configuration sections.
This test uses two older node versions, v0.15.2 and v0.16.3, to create older wallet versions to be used by upgradewallet.
49+ ], binary_cli=[
50+ self.options.bitcoincli,
51+ releases_path + "/v0.16.3/bin/bitcoin-cli",
52+ ])
53+
54+ self.start_nodes()
[regtest] from the bitcoin.conf in the 16.3 datadir before starting the nodes
Yes, if you want, you can create a generally available util method like
0def adjust_bitcoin_conf_for_pre_16_3(conf_file):
1 ...
50+ ], binary_cli=[
51+ self.options.bitcoincli,
52+ releases_path + "/v0.16.3/bin/bitcoin-cli",
53+ ])
54+ # older bitcoind's like v0.16.3 don't recognize config sections
55+ subprocess.call(["sed","-i", "-e", "s/\[regtest\]/ /g", self.nodes[1].bitcoinconf])
179@@ -180,6 +180,7 @@
180 'mempool_expiry.py',
181 'wallet_import_rescan.py',
182 'wallet_import_with_label.py',
183+ 'wallet_upgradewallet',
0 'wallet_upgradewallet.py',
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
No conflicts as of last run.
325@@ -326,6 +326,16 @@ def initialize_datadir(dirname, n, chain):
326 os.makedirs(os.path.join(datadir, 'stdout'), exist_ok=True)
327 return datadir
328
329+def adjust_bitcoin_conf_for_pre_16_3(conf_file):
330+ conf = open(conf_file,'r', encoding='utf8')
with context manager should simplify this
37+
38+ releases_path = os.getenv("PREVIOUS_RELEASES_DIR") or os.getcwd() + "/releases"
39+ if not os.path.isdir(releases_path):
40+ if os.getenv("TEST_PREVIOUS_RELEASES") == "true":
41+ raise AssertionError("TEST_PREVIOUS_RELEASES=1 but releases missing: " + releases_path)
42+ raise SkipTest("This test requires binaries for previous releases")
def skip_if_no_previous_release(self)? Maybe a future pull?
feature_backwards_compatibility, so I will later try it out with skip_if_no_previous_release(self).
70+ node_master.unloadwallet("")
71+
72+ # Copy the 0.16.3 wallet to the last Bitcoin Core version and open it:
73+ shutil.rmtree(node_master_wallets_dir)
74+ shutil.copytree(
75+ v16_3_wallets_dir,
41+ raise AssertionError("TEST_PREVIOUS_RELEASES=1 but releases missing: " + releases_path)
42+ raise SkipTest("This test requires binaries for previous releases")
43+
44+ self.add_nodes(self.num_nodes, extra_args=self.extra_args, versions=[
45+ None,
46+ 160300
-usehd=0 can be passed to the wallet?
get_nodes_least_version, because the framework couldn’t start the v0.15.2 node. The error was thrown in sync_mempoolsand sync_blocks. It seems to have something to do with the call of syncwithvalidationinterfacequeue. This indirectly affected the getpeerinfofrom sync_blocks, but because 0.15.2 never needed any peers for these particular tests, I thought, maybe a small helper methode for filtering those old nodes would be sufficient. If not, please, let me know what alternatives we have here.
325@@ -326,6 +326,14 @@ def initialize_datadir(dirname, n, chain):
326 os.makedirs(os.path.join(datadir, 'stdout'), exist_ok=True)
327 return datadir
328
329+def adjust_bitcoin_conf_for_pre_16_3(conf_file):
330+ conf_data = ""
325@@ -326,6 +326,14 @@ def initialize_datadir(dirname, n, chain):
326 os.makedirs(os.path.join(datadir, 'stdout'), exist_ok=True)
327 return datadir
328
329+def adjust_bitcoin_conf_for_pre_16_3(conf_file):
0def adjust_bitcoin_conf_for_pre_17(conf_file):
Sorry, this should say pre 17
415@@ -408,13 +416,22 @@ def sync_blocks(rpc_connections, *, wait=1, timeout=60):
416 if best_hash.count(best_hash[0]) == len(rpc_connections):
417 return
418 # Check that each peer has at least one connection
419- assert (all([len(x.getpeerinfo()) for x in rpc_connections]))
420+ assert (all([len(x.getpeerinfo()) for x in get_nodes_least_version(rpc_connections)]))
442@@ -426,8 +443,8 @@ def sync_mempools(rpc_connections, *, wait=1, timeout=60, flush_scheduler=True):
443 pool = [set(r.getrawmempool()) for r in rpc_connections]
444 if pool.count(pool[0]) == len(rpc_connections):
445 if flush_scheduler:
446- for r in rpc_connections:
447- r.syncwithvalidationinterfacequeue()
448+ for r in get_nodes_least_version(rpc_connections):
449+ r.syncwithvalidationinterfacequeue()
78+ v16_3_wallets_dir = os.path.join(v16_3_node.datadir, "regtest/wallets")
79+ v15_2_wallets_dir = os.path.join(v15_2_node.datadir, "regtest")
80+ # stop all nodes
81+ self.stop_node(0)
82+ self.stop_node(1)
83+ self.stop_node(2)
self.stop_nodes instead of listing each individually?
88+ v16_3_wallets_dir,
89+ node_master_wallets_dir
90+ )
91+ self.restart_node(0, ['-nowallet'])
92+
93+ node_master.loadwallet(node_master_wallets_dir)
loadwallet('') should be sufficient.
74+
75+ self.log.info("Test upgradewallet RPC...")
76+ # Prepare for copying of the older wallet
77+ node_master_wallets_dir = os.path.join(node_master.datadir, "regtest/wallets")
78+ v16_3_wallets_dir = os.path.join(v16_3_node.datadir, "regtest/wallets")
79+ v15_2_wallets_dir = os.path.join(v15_2_node.datadir, "regtest")
We should be be testing the specific version upgrades too, but this is a good place to start with. Those can be added in later.
ACK 84267045efbfbe2a035e7f34851f1b3bc9964be8
421@@ -415,7 +422,6 @@ def sync_blocks(rpc_connections, *, wait=1, timeout=60):
422 "".join("\n {!r}".format(b) for b in best_hash),
423 ))
424
425-
325@@ -326,6 +326,13 @@ def initialize_datadir(dirname, n, chain):
326 os.makedirs(os.path.join(datadir, 'stdout'), exist_ok=True)
327 return datadir
328
329+def adjust_bitcoin_conf_for_pre_17(conf_file):
330+ with open(conf_file,'r', encoding='utf8') as conf:
331+ conf_data = conf.read()
332+ with open(conf_file, 'w+', encoding='utf8') as conf:
333+ conf_data_changed = re.sub(r'\[regtest\]', ' ', conf_data)
0 conf_data_changed = conf_data.replace('[regtest]', '')
Should be doable without relying on the regex module
70+ assert_equal(res['blocks'], 101)
71+ node_master = self.nodes[0]
72+ v16_3_node = self.nodes[1]
73+ v15_2_node = self.nodes[2]
74+
75+ self.log.info("Test upgradewallet RPC...")
More precisely, this is the place where the error gets thrown.
0 File "/test/functional/test_framework/test_framework.py", line 499, in sync_blocks
1 sync_blocks(nodes or self.nodes, **kwargs)
2 File "/test/functional/test_framework/util.py", line 418, in sync_blocks
3 assert (all([len(x.getpeerinfo()) for x in rpc_connections]))
We have seen this already. So, the question is, do we want to work on “onboarding” of old nodes so their converted wallets can be tested here, or should we rather open a dedicated PR for this?
So syncing the blockchain is the issue?
Can be done in a really dumb way:
0def dumb_sync_blocks(...):
1 node_from = select node with max height
2 for i in range(max(blockheight)):
3 b = node_from.getblock(lbockhash=node_from.getblockhash(i), verbose=0)
4 for n in nodes:
5 n.submitblock(b)
Thanks. Have tried it, but the v0.15.2 node doesn’t expand its chain with submitted blocks. Interestingly, in its blockchaininfo the header count gets increased while block count remains at 0. The other node, v0.16.3 is working fine. Here my variant of dumb_sync_blocks
0def dumb_sync_blocks(self):
1 node_from = self.nodes[0]
2 to_height = node_from.getblockheader(node_from.getbestblockhash())['height']
3 for n in range(1, len(self.nodes)):
4 height = self.nodes[n].getblockheader(self.nodes[n].getbestblockhash())['height']
5 for i in range(height, to_height+1):
6 b = node_from.getblock(blockhash=node_from.getblockhash(i), verbose=0)
7 self.nodes[n].submitblock(b)
8 new_height = self.nodes[n].getblockheader(self.nodes[n].getbestblockhash())['height']
9 print("node: {}, new best height: {}".format(n, new_height))
The output is like this:
0node: 1, new best height: 101
1node: 2, new best height: 0
Here their respective blockchaininfo jsons:
v0.16.3
0{
1'chain': 'regtest',
2'blocks': 101,
3'headers': 101,
4'bestblockhash': '1a2f4021b111f20b12071bb68c92dc0037a2237ae034b345b002971dbb9f5e16', }
v0.15.2
0{
1'chain': 'regtest',
2'blocks': 0,
3'headers': 101,
4'bestblockhash': '0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206' }
In the test code I am sending some coins to both wallets and then calling dumb_sync_blocks to update all chains.
0node_master.generatetoaddress(101, v16_3_address)
1self.dumb_sync_blocks()
It might be possible to apply a hack and either:
Well, I have managed to sync the chains, but there is no way to make proper transactions. I have stolen a few things from p2p_segwit, like build_next_block for manual block generation and syncing:
0node: 1, new best height: 101
1node: 2, new best height: 101
2{
3'chain': 'regtest',
4'blocks': 101,
5'headers': 101,
6'bestblockhash': '0f2cfae3d63464c070eea6a8305f6f02245e69f41987c186da1c7f8af2bb1547', }
7{
8'chain': 'regtest',
9'blocks': 101,
10'headers': 101,
11'bestblockhash': '0f2cfae3d63464c070eea6a8305f6f02245e69f41987c186da1c7f8af2bb1547'}
However, I see no way how to update balance of v0.15.2 as I’d need to build a transaction that would have to have the witness field.
I will now push the latest changes.
117+ # calling upgradewallet with explicit version number
118+ # should return nothing if successful
119+ assert_equal(wallet.upgradewallet(169900), "")
120+ new_version = wallet.getwalletinfo()["walletversion"]
121+ # upgraded wallet should have version 169900
122+ assert_equal(new_version, 169900)
ACK
some more suggestions
325@@ -326,6 +326,13 @@ def initialize_datadir(dirname, n, chain):
326 os.makedirs(os.path.join(datadir, 'stdout'), exist_ok=True)
327 return datadir
328
329+def adjust_bitcoin_conf_for_pre_17(conf_file):
330+ with open(conf_file,'r', encoding='utf8') as conf:
331+ conf_data = conf.read()
332+ with open(conf_file, 'w+', encoding='utf8') as conf:
0 with open(conf_file, 'w', encoding='utf8') as conf:
no need for the read flag
72+ to test version upgrade and master hash key presence.
73+ v0.16.3 is being used to test version upgrade and balances.
74+ Further info: https://github.com/bitcoin/bitcoin/pull/18774#discussion_r416967844
75+ """
76+ node_from = self.nodes[0]
77+ to_height = node_from.getblockheader(node_from.getbestblockhash())['height']
0 to_height = node_from.getblockcount()
74+ Further info: https://github.com/bitcoin/bitcoin/pull/18774#discussion_r416967844
75+ """
76+ node_from = self.nodes[0]
77+ to_height = node_from.getblockheader(node_from.getbestblockhash())['height']
78+ for n in range(1, len(self.nodes)):
79+ height = self.nodes[n].getblockheader(self.nodes[n].getbestblockhash())['height']
0 height = self.nodes[n].getblockcount()
78+ for n in range(1, len(self.nodes)):
79+ height = self.nodes[n].getblockheader(self.nodes[n].getbestblockhash())['height']
80+ for i in range(height, to_height+1):
81+ b = node_from.getblock(blockhash=node_from.getblockhash(i), verbose=0)
82+ self.nodes[n].submitblock(b)
83+
0 assert_equal(self.nodes[n].getblockcount(), to_height)
Labels
Tests