wallet: bugfix, disallow migration of invalid scripts

furszy commented at 2:02 pm on July 22, 2023: member

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 commented at 2:02 pm on July 22, 2023: contributor

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.

DrahtBot added the label Wallet on Jul 22, 2023

in test/functional/wallet_migration.py:681 in 2a6825f952 outdated

673@@ -674,6 +674,12 @@ def send_to_script(script, amount):
674         wallet.rpc.importaddress(address=script_wsh_pkh.hex(), label="raw_spk2", rescan=True, p2sh=False)
675         assert_equal(wallet.getbalances()['watchonly']['trusted'], 5)
676 
677+        # Import sh(pkh()) script with the p2sh flag enabled.
678+        # This will wrap the script under another sh level, which is invalid!, and store it inside the wallet.
679+        # The migration process must skip the invalid scripts and the addressbook records linked to them.
680+        # They are not being watched by the current wallet, nor should be watched by the migrated one.
681+        wallet.rpc.importaddress(address=script_sh_pkh.hex(), label="boom_script", rescan=True, p2sh=True)

MarcoFalke commented at 5:01 pm on July 25, 2023:

0        wallet.rpc.importaddress(address=script_sh_pkh.hex(), label="boom_script", rescan=False, p2sh=True)

nit: Could skip rescan, since there are no matches?

furszy commented at 6:47 pm on July 25, 2023:

yep. Pushed.

MarcoFalke commented at 5:08 pm on July 25, 2023: member

Concept ACK

furszy force-pushed on Jul 25, 2023

achow101 commented at 7:52 pm on July 27, 2023: member

ACK 26f91a56afd01ce11245944c66361da9aaa6a71e

in test/functional/wallet_migration.py:681 in 26f91a56af outdated

673@@ -674,6 +674,12 @@ def send_to_script(script, amount):
674         wallet.rpc.importaddress(address=script_wsh_pkh.hex(), label="raw_spk2", rescan=True, p2sh=False)
675         assert_equal(wallet.getbalances()['watchonly']['trusted'], 5)
676 
677+        # Import sh(pkh()) script with the p2sh flag enabled.
678+        # This will wrap the script under another sh level, which is invalid!, and store it inside the wallet.
679+        # The migration process must skip the invalid scripts and the addressbook records linked to them.
680+        # They are not being watched by the current wallet, nor should be watched by the migrated one.
681+        wallet.rpc.importaddress(address=script_sh_pkh.hex(), label="boom_script", rescan=False, p2sh=True)

achow101 commented at 7:55 pm on July 27, 2023:

Perhaps check that this label doesn’t appear in any of the migrated wallets?

furszy commented at 1:59 am on July 29, 2023:

Perhaps check that this label doesn’t appear in any of the migrated wallets?

Sure, expanded the test to cover it, and also added few other additional cases.

furszy force-pushed on Jul 29, 2023

furszy commented at 2:11 am on July 29, 2023: member

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.

fanquake added this to the milestone 25.1 on Aug 1, 2023

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

fanquake commented at 12:45 pm on August 1, 2023: member

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

MarcoFalke commented at 1:04 pm on August 1, 2023: member

It did fix the crash when I left the Concept ACK #28125#pullrequestreview-1545962463

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

in src/wallet/wallet.cpp:4044 in 01126667b1 outdated

4039+                        skip_record = true;
4040+                        dests_to_delete.push_back(addr_pair.first);
4041+                        break;
4042+                    }
4043+                }
4044+                if (skip_record) continue; // Skip record linked to a script that we will not migrate

achow101 commented at 7:59 am on August 10, 2023:

In 01126667b16991b11766bbd17a73bee2505ae56f “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.

0                CScript addr_script = GetScriptForDestination(addr_pair.first);
1                if (!addr_script.empty() && not_migrated_scripts.count(script) > 0) {
2                    dests_to_delete.push_back(addr_pair.first);
3                    continue;
4                }

furszy commented at 1:37 pm on August 10, 2023:

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.

in test/functional/wallet_migration.py:689 in 21ac6b2932 outdated

684+        script_sh_pkh = script_to_p2sh_script(script_pkh)
685+        addy_script_sh_pkh = script_to_p2sh(script_pkh)  # valid script address
686+        addy_script_double_sh_pkh = script_to_p2sh(script_sh_pkh)  # invalid script address
687+
688+        # Note: 'importaddress()' will add two scripts, a valid one sh(pkh()) and an invalid one 'sh(sh(pkh()))'.
689+        #       Both of them will be stored with the same addressbook label. And only the former one should

achow101 commented at 8:02 am on August 10, 2023:

In 21ac6b2932b0f5ea9da4c251c65f4b88dfe8e983 “test: wallet, verify migration doesn’t crash for an invalid script”

s/former/latter

furszy commented at 1:46 pm on August 10, 2023:

done

wallet: disallow migration of invalid or not-watched scripts

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.

1de8a2372a

test: wallet, verify migration doesn't crash for an invalid script

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.

8e7e3e6149

furszy force-pushed on Aug 10, 2023

furszy commented at 1:45 pm on August 10, 2023: member

Updated per feedback. Thanks achow.

achow101 commented at 1:51 pm on August 10, 2023: member

ACK 8e7e3e614955e60d3bf9e9a481ef8916bf9e22d9

achow101 requested review from Sjors on Sep 5, 2023

fanquake removed this from the milestone 25.1 on Sep 14, 2023

fanquake added this to the milestone 26.0 on Sep 14, 2023

MarcoFalke commented at 8:03 am on September 15, 2023: member

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

achow101 merged this on Sep 19, 2023

achow101 closed this on Sep 19, 2023

furszy deleted the branch on Sep 19, 2023

jonesk7734 commented at 4:19 am on September 20, 2023: none

Ok

fanquake referenced this in commit 09f0fd4a55 on Sep 20, 2023

fanquake referenced this in commit ba9f6c9e52 on Sep 20, 2023

fanquake commented at 1:36 pm on September 20, 2023: member

Backported to 25.x in #28487.

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

wallet: bugfix, disallow migration of invalid scripts #28125

Reviews

Conflicts