wallet: bugfix, disallow migration of invalid scripts #28125

furszy · 2023-07-22T14:02:24Z

The legacy wallet allows to import any raw script (#28126), without
checking if it was valid or not. Appending it to the watch-only set.

This causes a crash in the migration process because we are only
expecting to find valid scripts inside the legacy spkm.

These stored scripts internally map to ISMINE_NO (same as if they
weren't stored at all..).

So we need to check for these special case, and take into account that
the legacy spkm could be storing invalid not watched scripts.

Which, in code words, means IsMineInner() returning
IsMineResult::INVALID for them.

Note:
To verify this, can run the test commit on top of master.
wallet_migration.py will crash without the bugfix commit.

DrahtBot · 2023-07-22T14:02:26Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	achow101
Concept ACK	MarcoFalke

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:

#26836 (wallet: simplify addressbook migration process by furszy)
#26596 (wallet: Migrate legacy wallets to descriptor wallets without requiring BDB by achow101)
#26008 (wallet: cache IsMine scriptPubKeys to improve performance of descriptor 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.

maflcko

Concept ACK

test/functional/wallet_migration.py

achow101 · 2023-07-27T19:52:48Z

ACK 26f91a5

test/functional/wallet_migration.py

furszy

Feedback tackled, thanks achow101.

Expanded test coverage with the requested label check and few more validations. Now the test:

Verifies that neither the invalid double sh script label, nor the addr(<script_hash_addr>) descriptor, are contained by any of the migrated wallets.
Verifies that the valid sh script (the original single sh script that is imported at the same time that the invalid one) and its address book record are contained by the migrated watch-only wallet.

Also, while was doing this. Corrected another behavior; the invalid script label was being retained in the main wallet (not on the watch-only one), which was not intended.

test/functional/wallet_migration.py

fanquake · 2023-08-01T12:45:18Z

@MarcoFalke does this fix #28057 in it's current state?

maflcko · 2023-08-01T13:04:55Z

It did fix the crash when I left the Concept ACK #28125 (review)

Happy to re-test after the next ACK or after merge.

achow101 · 2023-08-10T07:59:40Z

src/wallet/wallet.cpp

+                bool skip_record = false;
+                for (const auto& script : not_migrated_scripts) {
+                    CTxDestination dest;
+                    ExtractDestination(script, dest);
+                    if (IsValidDestination(dest) && dest == addr_pair.first) {
+                        skip_record = true;
+                        dests_to_delete.push_back(addr_pair.first);
+                        break;
+                    }
+                }
+                if (skip_record) continue; // Skip record linked to a script that we will not migrate


In 0112666 "wallet: disallow migration of invalid or not-watched scripts"

Instead of iterating over not_migrated_scripts for every address book record that gets here, we could instead convert the address book destination to a script and just do a lookup in the set.

Suggested change

bool skip_record = false;

for (const auto& script : not_migrated_scripts) {

CTxDestination dest;

ExtractDestination(script, dest);

if (IsValidDestination(dest) && dest == addr_pair.first) {

skip_record = true;

dests_to_delete.push_back(addr_pair.first);

break;

}

}

if (skip_record) continue; // Skip record linked to a script that we will not migrate

CScript addr_script = GetScriptForDestination(addr_pair.first);

if (!addr_script.empty() && not_migrated_scripts.count(script) > 0) {

dests_to_delete.push_back(addr_pair.first);

continue;

}

In 0112666 "wallet: disallow migration of invalid or not-watched scripts"

Instead of iterating over not_migrated_scripts for every address book record that gets here, we could instead convert the address book destination to a script and just do a lookup in the set.

I don't think that will work for p2pk scripts.

We store the p2pk script as PKHash destinations inside the address book. So, by calling GetScriptForDestination(), the resulting script will be a p2pkh. Which will not match the watched p2pk inside not_migrated_scripts.

With the current ExtractDestination() call, we are going in the other way around: we convert the p2pk script into a PKHash destination, which matches the destination stored in the address book.

What could do is create a not_migrated_destinations set from the not_migrated_scripts outside of the loop (by using the ExtractDestination() call), and use it here.

EDIT: Squashed this last point inside 1de8a23. Code should be a bit more readable now.

test/functional/wallet_migration.py

The legacy wallet allowed to import any raw script, without checking if it was valid or not. Appending it to the watch-only set. This causes a crash in the migration process because we are only expecting to find valid scripts inside the legacy spkm. These stored scripts internally map to `ISMINE_NO` (same as if they weren't stored at all..). So we need to check for these special case, and take into account that the legacy spkm could be storing invalid not watched scripts. Which, in code words, means IsMineInner() returning IsMineResult::INVALID for them.

The migration process must skip any invalid script inside the legacy spkm and all the addressbook records linked to them. These scripts are not being watched by the current wallet, nor should be watched by the migrated one. IsMine() returns ISMINE_NO for them.

furszy

Updated per feedback. Thanks achow.

furszy · 2023-08-10T13:37:06Z

src/wallet/wallet.cpp

+                bool skip_record = false;
+                for (const auto& script : not_migrated_scripts) {
+                    CTxDestination dest;
+                    ExtractDestination(script, dest);
+                    if (IsValidDestination(dest) && dest == addr_pair.first) {
+                        skip_record = true;
+                        dests_to_delete.push_back(addr_pair.first);
+                        break;
+                    }
+                }
+                if (skip_record) continue; // Skip record linked to a script that we will not migrate


In 0112666 "wallet: disallow migration of invalid or not-watched scripts"

Instead of iterating over not_migrated_scripts for every address book record that gets here, we could instead convert the address book destination to a script and just do a lookup in the set.

I don't think that will work for p2pk scripts.

We store the p2pk script as PKHash destinations inside the address book. So, by calling GetScriptForDestination(), the resulting script will be a p2pkh. Which will not match the watched p2pk inside not_migrated_scripts.

With the current ExtractDestination() call, we are going in the other way around: we convert the p2pk script into a PKHash destination, which matches the destination stored in the address book.

What could do is create a not_migrated_destinations set from the not_migrated_scripts outside of the loop (by using the ExtractDestination() call), and use it here.

EDIT: Squashed this last point inside 1de8a23. Code should be a bit more readable now.

achow101 · 2023-08-10T13:51:02Z

ACK 8e7e3e6

maflcko · 2023-09-15T08:03:11Z

re-checked that this no longer crashes my wallet.dat on migratewallet. Didn't review or otherwise check.

jonesk7734

Ok

The legacy wallet allowed to import any raw script, without checking if it was valid or not. Appending it to the watch-only set. This causes a crash in the migration process because we are only expecting to find valid scripts inside the legacy spkm. These stored scripts internally map to `ISMINE_NO` (same as if they weren't stored at all..). So we need to check for these special case, and take into account that the legacy spkm could be storing invalid not watched scripts. Which, in code words, means IsMineInner() returning IsMineResult::INVALID for them. Github-Pull: bitcoin#28125 Rebased-From: 1de8a23

The migration process must skip any invalid script inside the legacy spkm and all the addressbook records linked to them. These scripts are not being watched by the current wallet, nor should be watched by the migrated one. IsMine() returns ISMINE_NO for them. Github-Pull: bitcoin#28125 Rebased-From: 8e7e3e6

fanquake · 2023-09-20T13:36:04Z

Backported to 25.x in #28487.

…ripts 8e7e3e6 test: wallet, verify migration doesn't crash for an invalid script (furszy) 1de8a23 wallet: disallow migration of invalid or not-watched scripts (furszy) Pull request description: Fixing bitcoin#28057. The legacy wallet allows to import any raw script (bitcoin#28126), without checking if it was valid or not. Appending it to the watch-only set. This causes a crash in the migration process because we are only expecting to find valid scripts inside the legacy spkm. These stored scripts internally map to `ISMINE_NO` (same as if they weren't stored at all..). So we need to check for these special case, and take into account that the legacy spkm could be storing invalid not watched scripts. Which, in code words, means `IsMineInner()` returning `IsMineResult::INVALID` for them. Note: To verify this, can run the test commit on top of master. `wallet_migration.py` will crash without the bugfix commit. ACKs for top commit: achow101: ACK 8e7e3e6 Tree-SHA512: c2070e8ba78037a8f573b05bf6caa672803188f05429adf5b93f9fc1493faedadecdf018dee9ead27c656710558c849c5da8ca5f6f3bc9c23b3c4275d2fb50c7

The legacy wallet allowed to import any raw script, without checking if it was valid or not. Appending it to the watch-only set. This causes a crash in the migration process because we are only expecting to find valid scripts inside the legacy spkm. These stored scripts internally map to `ISMINE_NO` (same as if they weren't stored at all..). So we need to check for these special case, and take into account that the legacy spkm could be storing invalid not watched scripts. Which, in code words, means IsMineInner() returning IsMineResult::INVALID for them. Github-Pull: bitcoin#28125 Rebased-From: 1de8a23

The migration process must skip any invalid script inside the legacy spkm and all the addressbook records linked to them. These scripts are not being watched by the current wallet, nor should be watched by the migrated one. IsMine() returns ISMINE_NO for them. Github-Pull: bitcoin#28125 Rebased-From: 8e7e3e6

45a5fcb http: bugfix: track closed connection (stickies-v) 752a456 http: log connection instead of request count (stickies-v) ae86ada http: refactor: use encapsulated HTTPRequestTracker (stickies-v) f31899d gui: macOS, make appMenuBar part of the main app window (furszy) 64ffa94 gui: macOS, do not process dock icon actions during shutdown (furszy) e270f3f depends: fix unusable memory_resource in macos qt build (fanquake) a668394 build, macos: Fix `qt` package build with new Xcode 15 linker (Hennadii Stepanov) b3517cb test: Test loading wallets with conflicts without a chain (Andrew Chow) d63478c wallet: Check last block and conflict height are valid in MarkConflicted (Andrew Chow) 5e51a9c ci: Nuke Android APK task, Use credits for tsan (MarcoFalke) 910c362 test: ensure old fee_estimate.dat not read on restart and flushed (ismaelsadeeq) 37764d3 tx fees, policy: read stale fee estimates with a regtest-only option (ismaelsadeeq) 16bb916 tx fees, policy: do not read estimates of old fee_estimates.dat (ismaelsadeeq) c4dd598 tx fees, policy: periodically flush fee estimates to fee_estimates.dat (ismaelsadeeq) c36770c test: wallet, verify migration doesn't crash for an invalid script (furszy) 0d2a33e wallet: disallow migration of invalid or not-watched scripts (furszy) 2c51a07 Do not use std::vector = {} to release memory (Pieter Wuille) Pull request description: Further backports for the `25.x` branch. Currently: * #27622 * #27834 * #28125 * #28452 * #28542 * #28543 * #28551 * #28571 * bitcoin-core/gui#751 ACKs for top commit: hebasto: re-ACK 45a5fcb, only #28551 has been backported with since my recent [review](#28487 (review)). dergoegge: reACK 45a5fcb willcl-ark: reACK 45a5fcb Tree-SHA512: 0f5807aa364b7c2a2039fef11d5cd5e168372c3bf5b0e941350fcd92e7db4a1662801b97bb4f68e29788c77d24bbf97385a483c4501ca72d93fa25327d5694fa

DrahtBot added the Wallet label Jul 22, 2023

This was referenced Jul 23, 2023

wallet: batch and simplify addressbook migration process #26836

Merged

wallet: Migrate legacy wallets to descriptor wallets without requiring BDB #26596

Merged

wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets #26008

Merged

maflcko reviewed Jul 25, 2023

View reviewed changes

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

furszy force-pushed the 2023_wallet_bugfix_migration_invalid_scripts branch from 2a6825f to 26f91a5 Compare July 25, 2023 18:46

achow101 reviewed Jul 27, 2023

View reviewed changes

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

furszy force-pushed the 2023_wallet_bugfix_migration_invalid_scripts branch from 26f91a5 to 21ac6b2 Compare July 29, 2023 01:44

furszy commented Jul 29, 2023

View reviewed changes

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

fanquake added this to the 25.1 milestone Aug 1, 2023

fanquake added the Needs backport (25.x) label Aug 1, 2023

achow101 reviewed Aug 10, 2023

View reviewed changes

furszy added 2 commits August 10, 2023 10:35

furszy force-pushed the 2023_wallet_bugfix_migration_invalid_scripts branch from 21ac6b2 to 8e7e3e6 Compare August 10, 2023 13:42

furszy commented Aug 10, 2023

View reviewed changes

achow101 requested a review from Sjors September 5, 2023 18:24

fanquake modified the milestones: 25.1, 26.0 Sep 14, 2023

achow101 merged commit abe4fed into bitcoin:master Sep 19, 2023

furszy deleted the 2023_wallet_bugfix_migration_invalid_scripts branch September 19, 2023 17:41

jonesk7734 reviewed Sep 20, 2023

View reviewed changes

fanquake removed the Needs backport (25.x) label Sep 20, 2023

fanquake mentioned this pull request Sep 20, 2023

[25.1] Final backports #28487

Merged

bitcoin locked and limited conversation to collaborators Sep 19, 2024

wallet: bugfix, disallow migration of invalid scripts #28125

wallet: bugfix, disallow migration of invalid scripts #28125

Uh oh!

Conversation

furszy commented Jul 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jul 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

achow101 commented Jul 27, 2023

Uh oh!

Uh oh!

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

Uh oh!

fanquake commented Aug 1, 2023

Uh oh!

maflcko commented Aug 1, 2023

Uh oh!

achow101 Aug 10, 2023

Choose a reason for hiding this comment

Uh oh!

furszy Aug 10, 2023 • 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!

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

furszy Aug 10, 2023 • 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!

achow101 commented Aug 10, 2023

Uh oh!

maflcko commented Sep 15, 2023

Uh oh!

jonesk7734 left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake commented Sep 20, 2023

Uh oh!

Uh oh!

furszy commented Jul 22, 2023 •

edited

Loading

DrahtBot commented Jul 22, 2023 •

edited

Loading

furszy left a comment •

edited

Loading

furszy Aug 10, 2023 •

edited

Loading

furszy Aug 10, 2023 •

edited

Loading