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

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	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.

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

achow101 force-pushed on Nov 13, 2024

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

# 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

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

- # Now migrate and test that we still see have the same balance/transactions
+ # 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

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.

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.

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.

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.

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!

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.

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

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:

- test: Rework migratewallet to use previous releases
+ test: Rework migratewallet to use previous release
+ 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

rkrux approved

rkrux commented at 11:59 AM on November 13, 2024: contributor

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 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

maflcko approved

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

Nothing blocking

ACK a76ad56a80d9c9a60352bb98b363131e359a383b 🌆

<details><summary>Show signature</summary>

Signature:

untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK a76ad56a80d9c9a60352bb98b363131e359a383b 🌆
HCc+p5XHbLShm3UVysnAgJ/8rST3S/1pk8jnPZ3GPMEsR3fcUEEInTLmM4hfPEkuoPZDBo5hxMjqw+XscDFVDA==

</details>

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

test: Rework migratewallet to use previous release (v28.0) 55347a5018

achow101 force-pushed on Nov 19, 2024

maflcko commented at 8:06 AM on November 20, 2024: member

Only trivial style-only changes

re-ACK 55347a5018b2c252c56548f0a6dc1e88e42f66b6 🥊

<details><summary>Show signature</summary>

Signature:

untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 55347a5018b2c252c56548f0a6dc1e88e42f66b6 🥊
MpkIAt2IslqUp6iGZwemtEtxGbYa3VS9/DYQNG/mmqspN0jFhAArSyIr+Y9WjIHMT3dtzucXcRiti0dnGrX9AQ==

</details>

DrahtBot requested review from rkrux on Nov 20, 2024

rkrux approved

rkrux commented at 10:38 AM on November 22, 2024: contributor

re-ACK 55347a5

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

git range-diff a76ad56...55347a5

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

maflcko commented at 7:33 PM on December 5, 2024: member

@ryanofsky Do you think this is rfm?

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

ryanofsky assigned ryanofsky on Dec 5, 2024

ryanofsky merged this on Dec 5, 2024

ryanofsky closed this on Dec 5, 2024

sedited referenced this in commit 230a439a4a on Jan 17, 2025

stickies-v referenced this in commit d760fd3dda on Mar 17, 2025

stickies-v referenced this in commit cc83553352 on Mar 17, 2025

stickies-v referenced this in commit 2614933f06 on Mar 17, 2025

stickies-v referenced this in commit b70418c5fc on Mar 17, 2025

stickies-v referenced this in commit 69f8a1fe50 on Mar 17, 2025

hebasto referenced this in commit 4a679936bb on Apr 1, 2025

fanquake referenced this in commit 16b084f88d on Apr 2, 2025

janus referenced this in commit 29947528f9 on Sep 7, 2025

bug-castercv502 referenced this in commit 44b075fe71 on Sep 28, 2025

bitcoin locked this on Dec 5, 2025