wallet legacy: bugfix, disallow importing invalid scripts via importaddress #28126

1. 
   furszy commented at 2:04 PM on July 22, 2023: member

   E.g. we're currently allowing to import scripts with several sh levels.

   These scripts are not being watched by the wallet; IsMine returns ISMINE_NO for them (same as if they weren'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-wallet will fail without the bugfix commit.

   Prior to this PR, let's focus on #28125.

2. wallet: legacy, disallow importing invalid scripts

   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.

   ec5f408034
3. 
   DrahtBot commented at 2:04 PM on July 22, 2023: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

   See the guideline for information on the review process. A summary of reviews will appear here.

   <!--174a7506f384e20aa4161008e828411d-->

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #27034 (rpc: make importaddress compatible with descriptors wallet by furszy)
   • #26840 (refactor: importpubkey, importprivkey, importaddress, importmulti, and importdescriptors rpc by KolbyML)

   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.

4. test: wallet, coverage for importaddress invalid scripts

   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.

   60019df4b0
5. furszy force-pushed on Jul 22, 2023
6. DrahtBot added the label CI failed on Jul 22, 2023
7. DrahtBot removed the label CI failed on Jul 22, 2023
8. DrahtBot added the label CI failed on Aug 9, 2023
9. DrahtBot removed the label CI failed on Aug 18, 2023
10. achow101 referenced this in commit abe4fedab7 on Sep 19, 2023
11. DrahtBot added the label CI failed on Sep 20, 2023
12. achow101 requested review from Sjors on Sep 20, 2023
13. achow101 requested review from josibake on Sep 20, 2023
14. achow101 requested review from achow101 on Sep 20, 2023
15. 
    maflcko commented at 10:52 AM on September 21, 2023: member

    Needs rebase if still relevant

16. 
    maflcko commented at 10:54 AM on September 21, 2023: member

    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.

17. 
    furszy commented at 11:05 AM on September 21, 2023: member

    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.

18. achow101 closed this on Sep 21, 2023

    ---

19. Frank-GER referenced this in commit c851b748b2 on Sep 25, 2023
20. sidhujag referenced this in commit acdd6fb31d on Sep 26, 2023
21. bitcoin locked this on Sep 20, 2024

Review Requested
Sjorsachow101josibake