test: Remove `BOOST_FAIL` and add info in descriptor_tests #36772

1. 
   rustaceanrob commented at 10:10 AM on November 5, 2026: member

   The BOOST_FAIL here is used to exit the test based on a condition, but we can use REQUIRE directly. The context string is only printed on failure to my knowledge and this is not called on a child thread so there is no behavior change.

   The descriptor_tests uses BOOST_TEST_INFO to add the descriptor if the next condition fails, but there are some other places that also print the public or private descriptor. Seems useful throughout this whole funciton body to attach the private and public descriptor as context.

   Found in #35713 as low usage macros

   ref:

   • https://www.boost.org/doc/libs/latest/libs/test/doc/html/boost_test/utf_reference/testout_reference/test_output_macro_info.html
   • https://www.boost.org/doc/libs/latest/libs/test/doc/html/boost_test/utf_reference/testout_reference/test_output_macro_context_sticky.html
   • https://www.boost.org/doc/libs/latest/libs/test/doc/html/boost_test/utf_reference/testing_tool_ref/assertion_boost_fail.html
2. DrahtBot added the label Tests on Nov 5, 2026
3. 
   DrahtBot commented at 10:11 AM on November 5, 2026: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--006a51241073e994b41acfe9ec718e94-->

   External sites

   • Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36772
   • Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36772.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36772#author-ralph

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

   See the guideline and AI policy for information on the review process.

   Type	Reviewers
   ACK	l0rinc, maflcko, sedited
   Stale ACK	ismaelsadeeq

   If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

   <!--174a7506f384e20aa4161008e828411d-->

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #36442 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36442.svg">} (descriptor: Represent multipath descriptors in a single Descriptor object by achow101)
   • #36013 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36013.svg">} (test: Descriptor roundtrip and raw()/ addr() coverage by pablomartin4btc)

   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.

   <!--5faf32d7da4f0f540f40219e4f7537a3-->

4. in src/test/descriptor_tests.cpp:209 in 6fe87a36e7 outdated

    205 | @@ -204,11 +206,11 @@ void DoCheck(std::string prv, std::string pub, const std::string& norm_pub, int
    206 |      BOOST_CHECK(max_sat_nonmaxsig <= max_sat_maxsig);
    207 |      const auto max_elems{parse_priv->MaxSatisfactionElems()};
    208 |      const bool is_input_size_info_set{max_sat_maxsig && max_sat_nonmaxsig && max_elems};
    209 | -    BOOST_CHECK_MESSAGE(is_input_size_info_set || is_nontop_or_nonsolvable, prv);
    210 | +    BOOST_CHECK(is_input_size_info_set || is_nontop_or_nonsolvable);
   

   ---

   ---

   

   l0rinc commented at 10:51 AM on November 5, 2026:

   Changing BOOST_CHECK_MESSAGE to remove failure context is surprising in a PR framed around removing BOOST_FAIL and adding info.

   To include the parsing checks in the context too, we could wrap the whole DoCheck body in BOOST_TEST_CONTEXT("Priv desc: " << prv, "Pub desc: " << pub).

   ---

   

   rustaceanrob commented at 11:10 AM on November 5, 2026:

   These are in the scope of BOOST_TEST_INFO_SCOPE above so both descriptors are printed on failure. The prv addition would be redundant in this line but I can move the INFO_SCOPE above the Parse if you prefer.

5. in src/test/descriptor_tests.cpp:213 in 6fe87a36e7

    210 | +    BOOST_CHECK(is_input_size_info_set || is_nontop_or_nonsolvable);
    211 |  
    212 |      // The ScriptSize() must match the size of the Script string. (ScriptSize() is set for all descs but 'combo()'.)
    213 |      const bool is_combo{!parse_priv->IsSingleType()};
    214 | -    BOOST_CHECK_MESSAGE(is_combo || parse_priv->ScriptSize() == scripts[0][0].size() / 2, "Invalid ScriptSize() for " + prv);
    215 | +    BOOST_CHECK_MESSAGE(is_combo || parse_priv->ScriptSize() == scripts[0][0].size() / 2, "Invalid ScriptSize()");
   

   ---

   ---

   

   l0rinc commented at 10:52 AM on November 5, 2026:

   The message now just restates the assertion. We could use a conditional BOOST_CHECK_EQUAL instead:

   -    BOOST_CHECK_MESSAGE(is_combo || parse_priv->ScriptSize() == scripts[0][0].size() / 2, "Invalid ScriptSize()");
   +    if (!is_combo) BOOST_CHECK_EQUAL(parse_priv->ScriptSize().value(), scripts[0][0].size() / 2);
   

   (.value() also makes a missing size fail explicitly.)

   ---

   

   rustaceanrob commented at 9:54 AM on November 6, 2026:

   Took that thanks

6. 
   ismaelsadeeq commented at 10:53 AM on November 5, 2026: member

   ACK 6fe87a36e7d23a50b1155bc267786985acf23d5a

   Seems fine to change BOOST_FAIL to BOOST_REQUIRE_MESSAGE.

   I think the BOOST_TEST_INFO addition is not necessary, but also won't hurt so fine with me.

7. in src/test/descriptor_tests.cpp:472 in 6fe87a36e7 outdated

    468 | @@ -469,7 +469,7 @@ void DoCheck(std::string prv, std::string pub, const std::string& norm_pub, int
    469 |                      sigdata.ripemd160_preimages = preimages;
    470 |                      sigdata.hash160_preimages = preimages;
    471 |                      const auto prod_sig_res = ProduceSignature(FlatSigningProvider{keys_priv}.Merge(FlatSigningProvider{script_provider}), creator, spks[n], sigdata);
    472 | -                    BOOST_CHECK_MESSAGE(prod_sig_res == !(flags & SIGNABLE_FAILS), prv);
    473 | +                    BOOST_CHECK_EQUAL(prod_sig_res, !(flags & SIGNABLE_FAILS));
   

   ---

   ---

   

   l0rinc commented at 10:56 AM on November 5, 2026:

   Wouldn't BOOST_CHECK_NE be simpler here?

   ---

   

   rustaceanrob commented at 11:25 AM on November 5, 2026:

   The ! is implicitly casting the int to a bool on the RHS so removing !(...) and using NE doesn't have the same behavior if I replaced it with flags & SIGNABLE_FAILS. I can replace it with bool(flags & SIGNABLE_FAILS) to use NE but not sure if that makes things any simpler

   ---

   

   l0rinc commented at 8:58 AM on November 6, 2026:

   You're right, having clean bools on both sides is simpler, please resolve

8. l0rinc changes_requested
9. 
   l0rinc commented at 11:01 AM on November 5, 2026: contributor

   Left a few suggestions to extend descriptor context to the parsing checks and simplify assertions.

10. l0rinc approved
11. 
    l0rinc commented at 9:40 AM on November 6, 2026: contributor

    ACK 6fe87a36e7d23a50b1155bc267786985acf23d5a

    My previous quick review turned out to be a bit shallow: the new scoped context correctly covers the removed failure info. The ScriptSize() suggestion still stands, but it's just a nit. If you touch this again, moving INFO_SCOPE above the Parse calls would be useful too, though the current version already preserves the existing failure context.

12. test: Remove `BOOST_FAIL` and add info in descriptor_tests

    The `BOOST_FAIL` here is used to exit the test based on a condition, but
    we can use `REQUIRE` directly. The context string is only printed on
    failure to my knowledge and this is not called on a child thread so
    there is no behavior change.
    
    The `descriptor_tests` uses `BOOST_TEST_INFO` to add the descriptor if
    the next condition fails, but there are some other places that also
    print the public or private descriptor. Seems useful throughout this
    whole funciton body to attach the private and public descriptor as
    context.
    
    Found in 35713 as low usage macros
    
    ref:
    - https://www.boost.org/doc/libs/latest/libs/test/doc/html/boost_test/utf_reference/testout_reference/test_output_macro_info.html
    - https://www.boost.org/doc/libs/latest/libs/test/doc/html/boost_test/utf_reference/testout_reference/test_output_macro_context_sticky.html
    - https://www.boost.org/doc/libs/latest/libs/test/doc/html/boost_test/utf_reference/testing_tool_ref/assertion_boost_fail.html

    f5a887c199
13. rustaceanrob force-pushed on Nov 6, 2026
14. 
    rustaceanrob commented at 9:55 AM on November 6, 2026: member

    f5a887c199613be9ddc8a13fbb7934af614fa579 moved above parse, also suggestions either resolved or taken

15. 
    l0rinc commented at 10:01 AM on November 6, 2026: contributor

    ACK f5a887c199613be9ddc8a13fbb7934af614fa579

16. DrahtBot requested review from ismaelsadeeq on Nov 6, 2026
17. 
    maflcko commented at 3:04 PM on November 6, 2026: member

    review ACK f5a887c199613be9ddc8a13fbb7934af614fa579 🎲

    <details><summary>Show signature</summary>

    Signature:

    untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
    RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
    trusted comment: review ACK f5a887c199613be9ddc8a13fbb7934af614fa579 🎲
    o1VDtiDphvH08yY3jf1qVaHoUQNhD/jAKGX0ikHPIGhxeItcSia95ZWjnYQ0v7I1dI2naxv+khSmCjObOn3aAg==
    

    </details>

18. sedited approved
19. 
    sedited commented at 4:47 PM on November 6, 2026: contributor

    ACK f5a887c1996

20. sedited merged this on Nov 6, 2026
21. sedited closed this on Nov 6, 2026

    ---

22. rustaceanrob deleted the branch on Nov 8, 2026

Contributors

rustaceanrobDrahtBotl0rincismaelsadeeqmaflckosedited

Review Requested
ismaelsadeeq