test: descriptor: cover invalid multi/multi_a cases #32504

1. 
   brunoerg commented at 8:27 pm on May 14, 2025: contributor

   This PR adds test coverage for invalid multi() and multi_a() cases, see:

   1. https://github.com/bitcoin/bitcoin/blob/53eb5593f0a1a8ae5cf0fabea58e2f22193a5c55/src/script/descriptor.cpp#L1819-L1821

   2. https://github.com/bitcoin/bitcoin/blob/53eb5593f0a1a8ae5cf0fabea58e2f22193a5c55/src/script/descriptor.cpp#L1835-L1837

   3. https://github.com/bitcoin/bitcoin/blob/53eb5593f0a1a8ae5cf0fabea58e2f22193a5c55/src/script/descriptor.cpp#L1838-L1840

   We could also exercise to exceed the number of keys - 20 for multi and 999 for multi_a.

2. 
   DrahtBot commented at 8:27 pm on May 14, 2025: contributor

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

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32504.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	maflcko
   Concept ACK	sipa

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

3. DrahtBot added the label Tests on May 14, 2025
4. 
   maflcko commented at 5:50 am on May 15, 2025: member

   Nice, but two commits for adding three lines of related tests seems like it could be squashed?
5. test: descriptor: cover invalid multi/multi_a cases 58e55b17e6
6. brunoerg force-pushed on May 15, 2025
7. 
   brunoerg commented at 11:18 am on May 15, 2025: contributor

   Nice, but two commits for adding three lines of related tests seems like it could be squashed?

   Done.

8. 
   maflcko commented at 11:48 am on May 15, 2025: member

   lgtm ACK 58e55b17e632dbd4425dd64825b087f242ac4b7b
9. in src/test/descriptor_tests.cpp:930 in 58e55b17e6

   926@@ -927,6 +927,9 @@ BOOST_AUTO_TEST_CASE(descriptor_test)
   927     CheckUnparsable("multi(+1,L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1,5KYZdUEo39z3FPrtuX2QbbwGnNP5zTd7yyr2SC1j299sBCnWjss)", "multi(+1,03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd,04a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd5b8dec5235a0fa8722476c7709c02559e3aa73aa03918ba2d492eea75abea235)", "Multi threshold '+1' is not valid"); // Invalid threshold
   928     CheckUnparsable("multi(0,L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1,5KYZdUEo39z3FPrtuX2QbbwGnNP5zTd7yyr2SC1j299sBCnWjss)", "multi(0,03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd,04a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd5b8dec5235a0fa8722476c7709c02559e3aa73aa03918ba2d492eea75abea235)", "Multisig threshold cannot be 0, must be at least 1"); // Threshold of 0
   929     CheckUnparsable("multi(3,L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1,5KYZdUEo39z3FPrtuX2QbbwGnNP5zTd7yyr2SC1j299sBCnWjss)", "multi(3,03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd,04a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd5b8dec5235a0fa8722476c7709c02559e3aa73aa03918ba2d492eea75abea235)", "Multisig threshold cannot be larger than the number of keys; threshold is 3 but only 2 keys specified"); // Threshold larger than number of keys
   930+    CheckUnparsable("", "multi(0)()", "Multi: expected ',', got ')'");
   

   ---

   ---

   

   sipa commented at 11:51 am on May 15, 2025:

   Strange, GitHub renders this line in the diff view as indended by 4 spaces compared to the line above. I don’t see anything strange in the code itself, though.

   ---

   

   brunoerg commented at 12:22 pm on May 15, 2025:

   Yes, this is weird. I don’t know why.

   ---

   

   maflcko commented at 12:27 pm on May 15, 2025:

   The other lines in this test are excessively long. It would be better to use c-style multi-line string:

   0CheckUnparsable("multi("
   1  "bla,"
   2  "foo)"
   3);
   

   However, this seems unrelated to the changes here and would require reformatting the whole file.

10. 
    sipa commented at 11:51 am on May 15, 2025: member

    Concept ACK

Contributors
brunoerg DrahtBot maflcko sipa

Labels
Tests