fuzz: fix checks on number of required sigs and keys in multisig scripts #23091

1. 
   mjdietzx commented at 0:06 am on September 25, 2021: contributor

   Fuzz test coverage was added around the ExtractDestination(s) functions in a29f522ba4aa71582b54025c5682b4c1687ae9f3. This commit contained an incorrect assertion that the number of required signatures in a multisig script was equal to the number of addresses. This is incorrect, as for an m-of-n multisig, m <= n.

   a29f522ba4aa71582b54025c5682b4c1687ae9f3 also had an incorrect assertion on the maximum number of public keys per multisig. It checked that the number of keys was less than or equal to 16. This is incorrect, as it should be <= 20 (see MAX_PUBKEYS_PER_MULTISIG).

   Both of these incorrect assertions are fixed in this commit accordingly.

   Note: this is sort of moot because this behavior is deprecated and these fuzz tests are slated for removal in v23 when the deprecation period for -deprecatedrpc=addresses ends. However, for correctness and just in case, it’s fixed here.

   Noticed here: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=39152 Moot because this PR #22650 gets rid of all this code anyways

2. fanquake added the label Tests on Sep 25, 2021
3. 
   darosior commented at 7:17 am on September 25, 2021: member

   The line just above asserts:

   0assert(required_ret >= 1 && required_ret <= 16);
   

   This is wrong, too. You could have up to 20 keys in a CHECKMULTISIG. Could you correct it too while you’re at it?

4. fuzz: fix checks on number of required sigs and keys in multisig scripts

   Fuzz test coverage was added around the ExtractDestination(s) functions in
   a29f522ba4aa71582b54025c5682b4c1687ae9f3. This commit contained an incorrect
   assertion that the number of required signatures in a multisig script was equal
   to the number of addresses. This is incorrect, as for an m-of-n multisig, m <= n.
   
   a29f522ba4aa71582b54025c5682b4c1687ae9f3 also had an incorrect assertion on
   the maximum number of public keys per multisig. It checked that the number of
   keys was less than or equal to 16. This is incorrect, as it should be <= 20
   (see MAX_PUBKEYS_PER_MULTISIG).
   
   Both of these incorrect assertions are fixed in this commit accordingly.
   
   Note: this is sort of moot because this behavior is deprecated and these
   fuzz tests are slated for removal in v23 when the deprecation period for
   -deprecatedrpc=addresses ends. However, for correctness and just in case,
   it's fixed here.

   09d29b52c8
5. mjdietzx force-pushed on Sep 25, 2021
6. mjdietzx renamed this:
   fuzz: fix assertion on the number of required sigs in a bare multisig script
   fuzz: fix checks on number of required sigs and keys in multisig scripts
   on Sep 25, 2021
7. 
   mjdietzx commented at 2:18 pm on September 25, 2021: contributor

   You could have up to 20 keys in a CHECKMULTISIG. Could you correct it too while you’re at it?

   Nice catch @darosior, I fixed as suggested in this commit

8. 
   DrahtBot commented at 2:56 pm on September 25, 2021: member

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #22650 (Remove -deprecatedrpc=addresses flag and corresponding code/logic by mjdietzx)

   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.

9. 
   darosior commented at 10:22 am on September 26, 2021: member

   utACK 09d29b52c836a409b19c38be7f8f34b1bac95c91
10. 
    DrahtBot commented at 10:12 pm on September 28, 2021: member

    🐙 This pull request conflicts with the target branch and needs rebase.

    Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

11. DrahtBot added the label Needs rebase on Sep 28, 2021
12. 
    mjdietzx commented at 10:35 pm on September 28, 2021: contributor

    PR #22650 got merged before this, which deleted all of the code this would have corrected, therefore this PR is unnecessary and I’m closing it
13. mjdietzx closed this on Sep 28, 2021

    ---

14. mjdietzx deleted the branch on Sep 28, 2021
15. DrahtBot locked this on Oct 30, 2022

Contributors
mjdietzx darosior DrahtBot

Labels
Tests Needs rebase