wallet legacy: bugfix, disallow importing invalid scripts via importaddress #28126
Conversation
E.g. we're allowing to import scripts with several sh levels. These scripts are not being watched by the wallet; IsMine return ISMINE_NO for them. So, there is no reason to accept them in the first place.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
Also, fixed feature_segwit.py that was checking against a never thrown RPC exception message. The "The wallet already contains the private key for this address or script" error is part of the importdescriptors RPC command, not importaddress.
furszy
force-pushed
the
2023_bugfix_wallet_importaddress
branch
from
e67de5a
to
60019df
Compare
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 #28057. 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. ACKs for top commit: achow101: ACK 8e7e3e6 Tree-SHA512: c2070e8ba78037a8f573b05bf6caa672803188f05429adf5b93f9fc1493faedadecdf018dee9ead27c656710558c849c5da8ca5f6f3bc9c23b3c4275d2fb50c7
|
Needs rebase if still relevant |
|
I wonder if this is useful. The bad behaviour was allowed for years and now that the legacy wallet will be removed soon, I wonder if it makes sense to change the bad behavior for this short time. But no strong opinion, if others want this or think that there is a use-case for real users. |
There was a problem hiding this comment.
I wonder if this is useful.
The bad behaviour was allowed for years and now that the legacy wallet will be removed soon, I wonder if it makes sense to change the bad behavior for this short time.
But no strong opinion, if others want this or think that there is a use-case for real users.
I mostly agree. I made it mostly to prevent users from doing nasty things on their wallets. The import of invalid scripts might have other consequences. Bugs in the legacy wallet that we haven't (and probably will never) discover.
But in any case, I'm not that strong here anyway. Happy to hear other opinions. Another "let's not do it" comment and will close the PR.
…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
…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
E.g. we're currently allowing to import scripts with several
sh levels.
These scripts are not being watched by the wallet;
IsMinereturnsISMINE_NOfor them (same as if theyweren't stored at all..).
So, there is no reason to accept them in the first
place.
Note:
To verify this, can run the test commit on top of master.
wallet_basic.py --legacy-walletwill fail without thebugfix commit.
Prior to this PR, let's focus on #28125.