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

achow101 · 2024-11-07T18:32:07Z

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 · 2024-11-07T18:32:10Z

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.

maflcko

left a question/nit

test/functional/wallet_migration.py

test/get_previous_releases.py

test/functional/wallet_migration.py

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.

rkrux

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!

test/functional/wallet_migration.py

rkrux · 2024-11-13T09:45:37Z

test/functional/wallet_migration.py

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

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

rkrux · 2024-11-13T09:56:04Z

test/functional/wallet_migration.py

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

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

rkrux · 2024-11-13T10:30:39Z

test/functional/wallet_migration.py

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

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

rkrux · 2024-11-13T10:42:35Z

test/functional/wallet_migration.py

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

Yes, no test relies on the time.

rkrux · 2024-11-13T11:00:56Z

test/functional/wallet_migration.py

rkrux · 2024-11-13T11:52:12Z

test/functional/wallet_migration.py

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)

test/functional/wallet_migration.py

maflcko

Nothing blocking

ACK a76ad56a80d9c9a60352bb98b363131e359a383b 🌆

Show signature

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

test/functional/wallet_migration.py

maflcko · 2024-11-20T08:06:18Z

Only trivial style-only changes

re-ACK 55347a5 🥊

Show signature

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

rkrux

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 · 2024-12-05T19:33:12Z

@ryanofsky Do you think this is rfm?

ryanofsky · 2024-12-05T20:24:05Z

@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

DrahtBot added the Tests label Nov 7, 2024

This was referenced Nov 7, 2024

Proposed Timeline for Legacy Wallet and BDB removal #20160

Closed

wallet: Disable creating and loading legacy wallets #31250

Merged

maflcko reviewed Nov 8, 2024

View reviewed changes

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

achow101 force-pushed the migratewallet-prev-rels branch from 87e8845 to 0bbc4f6 Compare November 8, 2024 17:19

DrahtBot added the CI failed label Nov 8, 2024

maflcko reviewed Nov 9, 2024

View reviewed changes

test/get_previous_releases.py Outdated Show resolved Hide resolved

achow101 force-pushed the migratewallet-prev-rels branch from 0bbc4f6 to 0d26167 Compare November 9, 2024 17:53

DrahtBot removed the CI failed label Nov 9, 2024

DrahtBot mentioned this pull request Nov 10, 2024

Remove the legacy wallet and BDB dependency #28710

Merged

maflcko reviewed Nov 11, 2024

View reviewed changes

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

wallet: Check specified wallet exists before migration

f42ec0f

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.

achow101 force-pushed the migratewallet-prev-rels branch from 0d26167 to a76ad56 Compare November 13, 2024 02:19

rkrux approved these changes Nov 13, 2024

View reviewed changes

maflcko approved these changes Nov 14, 2024

View reviewed changes

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

maflcko added this to the 29.0 milestone Nov 14, 2024

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

55347a5

achow101 force-pushed the migratewallet-prev-rels branch from a76ad56 to 55347a5 Compare November 19, 2024 16:59

DrahtBot requested a review from rkrux November 20, 2024 08:06

rkrux approved these changes Nov 22, 2024

View reviewed changes

maflcko mentioned this pull request Dec 2, 2024

ci: Test cross-built Windows executables on Windows natively #31176

Merged

DrahtBot mentioned this pull request Dec 3, 2024

test: Call generate RPCs through test framework only #31403

Merged

ryanofsky self-assigned this Dec 5, 2024

ryanofsky merged commit 2eccb8b into bitcoin:master Dec 5, 2024
16 checks passed

furszy mentioned this pull request Dec 6, 2024

wallet: fix crash during watch-only wallet migration #31374

Merged

achow101 mentioned this pull request Dec 7, 2024

wallet: Remove IsMine from migration code #30328

Closed

rkrux mentioned this pull request Dec 13, 2024

wallet: migration, avoid loading legacy wallet after failure when BDB isn't compiled #31451

Merged

sedited added a commit to sedited/rust-bitcoinkernel that referenced this pull request Jan 17, 2025

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

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

Uh oh!

Conversation

achow101 commented Nov 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Nov 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rkrux left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Nov 20, 2024

Uh oh!

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

maflcko commented Dec 5, 2024

Uh oh!

ryanofsky commented Dec 5, 2024

Uh oh!

Uh oh!

achow101 commented Nov 7, 2024 •

edited

Loading

DrahtBot commented Nov 7, 2024 •

edited

Loading

rkrux left a comment •

edited

Loading