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

pull achow101 wants to merge 2 commits into bitcoin:master from achow101:migratewallet-prev-rels changing 3 files +174 −166
  1. 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

  2. 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 maflcko, rkrux

    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:

    • #31403 (test: Call generate RPCs through test framework only by maflcko)

    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.

  3. DrahtBot added the label Tests on Nov 7, 2024
  4. 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.
  5. maflcko commented at 9:58 am on November 8, 2024: member
    left a question/nit
  6. achow101 force-pushed on Nov 8, 2024
  7. DrahtBot added the label CI failed on Nov 8, 2024
  8. 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.
  9. achow101 force-pushed on Nov 9, 2024
  10. DrahtBot removed the label CI failed on Nov 9, 2024
  11. 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?

    See also https://corecheck.dev/bitcoin/bitcoin/pulls/31248

    achow101 commented at 2:19 am on November 13, 2024:
    Done. Also added a test for non-existent wallets and this uncovered a bug.
  12. 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
  13. achow101 force-pushed on Nov 13, 2024
  14. in test/functional/wallet_migration.py:186 in a76ad56a80 outdated 
    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.
    achow101 commented at 4:59 pm on November 19, 2024:
    Done
  15. in test/functional/wallet_migration.py:213 in a76ad56a80 outdated 
    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.
    achow101 commented at 4:59 pm on November 19, 2024:
    Done
  16. in test/functional/wallet_migration.py:62 in a76ad56a80 outdated 
    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.
  17. in test/functional/wallet_migration.py:477 in a76ad56a80 outdated 
    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.
  18. in test/functional/wallet_migration.py:511 in a76ad56a80 outdated 
    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.
  19. in test/functional/wallet_migration.py:557 in a76ad56a80 outdated 
    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.
  20. in test/functional/wallet_migration.py:796 in a76ad56a80 outdated 
    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!
  21. in test/functional/wallet_migration.py:786 in a76ad56a80 outdated 
    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)
  22. in test/functional/wallet_migration.py:50 in a76ad56a80 outdated 
    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.
    achow101 commented at 4:59 pm on November 19, 2024:
    Done
  23. rkrux approved
  24. 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!

  25. in test/functional/wallet_migration.py:226 in a76ad56a80 outdated 
    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?
    achow101 commented at 4:59 pm on November 19, 2024:
    Fixed
  26. maflcko approved
  27. 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==
  28. maflcko added this to the milestone 29.0 on Nov 14, 2024
  29. test: Rework migratewallet to use previous release (v28.0) 55347a5018
  30. achow101 force-pushed on Nov 19, 2024
  31. maflcko commented at 8:06 am on November 20, 2024: member

    Only trivial style-only changes

    re-ACK 55347a5018b2c252c56548f0a6dc1e88e42f66b6 🥊

    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: re-ACK 55347a5018b2c252c56548f0a6dc1e88e42f66b6 🥊
3MpkIAt2IslqUp6iGZwemtEtxGbYa3VS9/DYQNG/mmqspN0jFhAArSyIr+Y9WjIHMT3dtzucXcRiti0dnGrX9AQ==
  32. DrahtBot requested review from rkrux on Nov 20, 2024
  33. rkrux approved
  34. rkrux commented at 10:38 am on November 22, 2024: none

    re-ACK 55347a5

    Reviewed the range diff - only minor comment, commit message, renaming changes. Ran cmake again.

    0git range-diff a76ad56...55347a5

    The PR title and description can be updated similar to the commit message of 55347a5.

  35. maflcko commented at 7:33 pm on December 5, 2024: member
    @ryanofsky Do you think this is rfm?
  36. ryanofsky commented at 8:24 pm on December 5, 2024: contributor

    @ryanofsky Do you think this is rfm?

    Looks like it, only 2 acks, but this is mostly a test change, and the nontest change is pretty obvious. Will run tests and merge

  37. ryanofsky assigned ryanofsky on Dec 5, 2024
  38. ryanofsky merged this on Dec 5, 2024
  39. ryanofsky closed this on Dec 5, 2024


achow101 DrahtBot maflcko rkrux ryanofsky

Labels
Tests

Milestone
29.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-18 21:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me