test: Rework wallet_migration.py to use previous releases

achow101 commented at 6:32 pm on November 7, 2024: member

This PR reworks wallet_migration.py to use previous releases to produce legacy wallets for testing so that the test will continue to work once legacy wallets are removed.

Split from #28710

DrahtBot commented at 6:32 pm on November 7, 2024: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31248.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	rkrux, maflcko

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31250 (wallet: Disable creating and loading legacy wallets 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 Tests on Nov 7, 2024

in test/functional/wallet_migration.py:120 in 87e88459b0 outdated

122+        assert_equal(a, b)
123+
124+    def assert_balances_equal(self, a, b):
125+        a.pop("lastprocessedblock", None)
126+        b.pop("lastprocessedblock", None)
127+        assert_equal(a, b)

maflcko commented at 9:57 am on November 8, 2024:

I wonder if this diff can be removed by just using 28 as the previous release, with the -deprecatedrpc=create_bdb option enabled?

achow101 commented at 5:19 pm on November 8, 2024:

Indeed, removed.

maflcko commented at 9:58 am on November 8, 2024: member

left a question/nit

achow101 force-pushed on Nov 8, 2024

DrahtBot added the label CI failed on Nov 8, 2024

in test/get_previous_releases.py:102 in 0bbc4f6036 outdated

 97@@ -98,6 +98,15 @@
 98     "fe6e347a66043946920c72c9c4afca301968101e6b82fb90a63d7885ebcceb32": {"tag": "v25.0", "tarball": "bitcoin-25.0-riscv64-linux-gnu.tar.gz"},
 99     "5708fc639cdfc27347cccfd50db9b73b53647b36fb5f3a4a93537cbe8828c27f": {"tag": "v25.0", "tarball": "bitcoin-25.0-x86_64-apple-darwin.tar.gz"},
100     "33930d432593e49d58a9bff4c30078823e9af5d98594d2935862788ce8a20aec": {"tag": "v25.0", "tarball": "bitcoin-25.0-x86_64-linux-gnu.tar.gz"},
101+
102+    "e198461d981b675d39caaec4d5e69e79d7fd52c3d3508f5821e2effe7d4f12c2": {"tag": "v26.1", "tarball": "bitcoin-26.1-aarch64-linux-gnu.tar.gz"},

maflcko commented at 12:06 pm on November 9, 2024:

should be 28?

achow101 commented at 5:53 pm on November 9, 2024:

Oops yes.

achow101 force-pushed on Nov 9, 2024

DrahtBot removed the label CI failed on Nov 9, 2024

in test/functional/wallet_migration.py:503 in 0d26167561 outdated

509-        bals = wallet.getbalances()
510-
511-        wallet.unloadwallet()
512-
513-        assert_raises_rpc_error(-8, "RPC endpoint wallet and wallet_name parameter specify different wallets", wallet.migratewallet, "someotherwallet")
514-        assert_raises_rpc_error(-8, "Either RPC endpoint wallet or wallet_name parameter must be provided", self.nodes[0].migratewallet)

maflcko commented at 10:28 am on November 11, 2024:

Looks like this coverage should be preserved?

achow101 commented at 2:19 am on November 13, 2024:

Done. Also added a test for non-existent wallets and this uncovered a bug.

wallet: Check specified wallet exists before migration

The previous error message for non-existent wallets of "Already a
descriptor wallet" is misleading. Return a more specific error when a
non-existent wallet is specified.

f42ec0f3bf

test: Rework migratewallet to use previous releases a76ad56a80

achow101 force-pushed on Nov 13, 2024

in test/functional/wallet_migration.py:186 in a76ad56a80

186         basic1_restored_wi = basic1_restored.getwalletinfo()
187         assert_equal(basic1_restored_wi['balance'], bal)
188         assert_equal(basic1_restored.listaddressgroupings(), addr_gps)
189         self.assert_list_txs_equal(basic1_restored.listtransactions(), txs)
190 
191         # restart node and verify that everything is still there

rkrux commented at 9:18 am on November 13, 2024:

Nit

0# restart master node and verify that everything is still there

achow101 commented at 4:42 pm on November 13, 2024:

Will do if I need to retouch.

in test/functional/wallet_migration.py:213 in a76ad56a80

205@@ -193,12 +206,12 @@ def test_basic(self):
206             send_value = random.randint(1, 4)
207             default.sendtoaddress(addr, send_value)
208             basic2_balance += send_value
209-            self.generate(self.nodes[0], 1)
210+            self.generate(self.master_node, 1)
211             assert_equal(basic2.getbalance(), basic2_balance)
212         basic2_txs = basic2.listtransactions()
213 
214         # Now migrate and test that we still see have the same balance/transactions

rkrux commented at 9:23 am on November 13, 2024:

Not in the diff but still

0- # Now migrate and test that we still see have the same balance/transactions
1+ # Now migrate and test that we still have the same balance/transactions

achow101 commented at 4:42 pm on November 13, 2024:

Will do if I need to retouch.

in test/functional/wallet_migration.py:62 in a76ad56a80

63             file_magic = f.read(16)
64             assert_equal(file_magic, b'SQLite format 3\x00')
65-        assert_equal(self.nodes[0].get_wallet_rpc(wallet_name).getwalletinfo()["format"], "sqlite")
66+        assert_equal(self.master_node.get_wallet_rpc(wallet_name).getwalletinfo()["format"], "sqlite")
67 
68     def create_legacy_wallet(self, wallet_name, **kwargs):

rkrux commented at 9:45 am on November 13, 2024:

Should this not be called create_legacy_wallet_and_get_rpc now highlighting that it returns the wallet RPC from the old node?

achow101 commented at 4:37 pm on November 13, 2024:

It returned RPC originally, and I don’t think that adding a rename to the diff would be helpful.

in test/functional/wallet_migration.py:477 in a76ad56a80

489-        assert_raises_rpc_error(-4, "The passphrase contains a null character", wallet.migratewallet, None, "pass\0with\0null")
490-
491-        # Check the wallet is still active post-migration failure.
492-        # If not, it will throw an exception and abort the test.
493-        wallet.walletpassphrase("pass", 99999)
494-        wallet.getnewaddress()

rkrux commented at 9:56 am on November 13, 2024:

Don’t we need to check for these in the old node?

achow101 commented at 4:39 pm on November 13, 2024:

No, the master node migrates a copy of the wallet. The old node will have the original copy which remains unchanged regardless.

in test/functional/wallet_migration.py:511 in a76ad56a80

517-        info = wallet.getwalletinfo()
518-        assert_equal(info["descriptors"], True)
519-        assert_equal(info["format"], "sqlite")
520-        wallet.gettransaction(txid)
521-
522-        assert_equal(bals, wallet.getbalances())

rkrux commented at 10:30 am on November 13, 2024:

This notloaded wallet migration testing is removed because it is effectively tested in test_unloaded_by_path below?

achow101 commented at 4:41 pm on November 13, 2024:

notloaded is specifically tested because legacy wallets won’t be able to be loaded.

in test/functional/wallet_migration.py:557 in a76ad56a80

553+        backup_path = self.master_node.wallets_path / backup_filename
554         assert backup_path.exists()
555         assert_equal(str(backup_path), res['backup_path'])
556         assert {"name": backup_filename} not in walletdir_list["wallets"]
557 
558+        self.master_node.setmocktime(0)

rkrux commented at 10:42 am on November 13, 2024:

+1 for adding this. Is the reason for the absence of this unsetting here not causing any issue earlier when wallet.setmocktime(curr_time) was added that no subsequent test in this file below relies on mock time?

achow101 commented at 4:41 pm on November 13, 2024:

Yes, no test relies on the time.

in test/functional/wallet_migration.py:796 in a76ad56a80

797 
798         # The specific assertion in MarkConflicted being tested requires that the parent tx is already loaded
799-        # by the time the child tx is loaded. Since transactions end up being loaded in txid order due to how both
800-        # and sqlite store things, we can just grind the child tx until it has a txid that is greater than the parent's.
801+        # by the time the child tx is loaded. Since transactions end up being loaded in txid order due to how
802+        # sqlite stores things, we can just grind the child tx until it has a txid that is greater than the parent's.

rkrux commented at 11:00 am on November 13, 2024:

TIL!

in test/functional/wallet_migration.py:786 in a76ad56a80

784         self.log.info("Test migration when wallet contains conflicting transactions")
785-        def_wallet = self.nodes[0].get_wallet_rpc(self.default_wallet_name)
786+        def_wallet = self.master_node.get_wallet_rpc(self.default_wallet_name)
787 
788         wallet = self.create_legacy_wallet("conflicts")
789         def_wallet.sendtoaddress(wallet.getnewaddress(), 10)

rkrux commented at 11:52 am on November 13, 2024:

I have noticed this pattern in most of these tests that I found a little difficult to read. Slightly more verbose naming would make it easier to parse like below. I am not suggesting this change to be done in this PR because this pattern existed earlier as well but can be updated in the future if others find this helpful as well.

0- def_wallet.sendtoaddress(wallet.getnewaddress(), 10)
1+ def_descriptor_wallet.sendtoaddress(legacy_wallet.getnewaddress(), 10)

in test/functional/wallet_migration.py:50 in a76ad56a80

49+        self.skip_if_no_previous_releases()
50+
51+    def setup_nodes(self):
52+        self.add_nodes(self.num_nodes, versions=[
53+            None,
54+            280000,

rkrux commented at 11:58 am on November 13, 2024:

Would be nice to have the commit message mention this instead of use previous releases, which gives an impression multiple previous releases are being used. Couple suggestions:

0- test: Rework migratewallet to use previous releases
1+ test: Rework migratewallet to use previous release
2+ test: Rework migratewallet to use v28 release

achow101 commented at 4:42 pm on November 13, 2024:

Will do if I need to retouch.

rkrux approved

rkrux commented at 11:59 am on November 13, 2024: none

tACK a76ad56a80d9c9a60352bb98b363131e359a383b

Certainly useful because it reflects how the end users would do wallet migration after the upcoming release.

Make is successful but around 15 functional tests are failing in my system that seem unrelated to this change because they are failing in master as well. All of the failures are due to timeouts, probably an issue with my setup.

Asked few questions and left suggestions. Found few really good tests in this file in the end!

in test/functional/wallet_migration.py:226 in a76ad56a80

225@@ -213,7 +226,7 @@ def test_basic(self):
226         assert_raises_rpc_error(-4, "Error: This wallet is already a descriptor wallet", self.nodes[0].migratewallet, "basic2")

maflcko commented at 7:12 pm on November 14, 2024:

nit in a76ad56a80d9c9a60352bb98b363131e359a383b: Forgot to use the node alias?

maflcko approved

maflcko commented at 7:15 pm on November 14, 2024: member

Nothing blocking

ACK a76ad56a80d9c9a60352bb98b363131e359a383b 🌆

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: ACK a76ad56a80d9c9a60352bb98b363131e359a383b 🌆
3HCc+p5XHbLShm3UVysnAgJ/8rST3S/1pk8jnPZ3GPMEsR3fcUEEInTLmM4hfPEkuoPZDBo5hxMjqw+XscDFVDA==

maflcko added this to the milestone 29.0 on Nov 14, 2024

test: Rework wallet_migration.py to use previous releases #31248

Code Coverage & Benchmarks

Reviews

Conflicts