1. 
   cuiweixie commented at 12:08 pm on February 28, 2026: none

   Motivation

   This patch fixes undefined behavior in Clone() in src/script/descriptor.cpp. When std::transform is used with providers.begin() or subdescs.begin() as the output iterator, the vectors have been reserve()d but have size 0. Writing through begin() in that case writes past the logical end of the vector, which is undefined behavior.

2. script: Fix undefined behavior in Clone() -- std::transform writes past end of empty vector 44feab23a7
3. DrahtBot added the label Consensus on Feb 28, 2026
4. 
   DrahtBot commented at 12:08 pm on February 28, 2026: contributor

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

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	maflcko, rkrux

   If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

5. cuiweixie renamed this:
   script: Fix undefined behavior in Clone() -- std::transform writes pat end of empty vector
   script: Fix undefined behavior in Clone() -- std::transform writes past end of empty vector
   on Feb 28, 2026
6. 
   pinheadmz commented at 12:59 pm on February 28, 2026: member

   Can you provide a unit test that triggers the UB before the patch is applied? That will help with review.
7. 
   cuiweixie commented at 1:47 pm on February 28, 2026: none

   Can you provide a unit test that triggers the UB before the patch is applied? That will help with review. @pinheadmz Done

8. 
   maflcko commented at 2:38 pm on February 28, 2026: member

   Was this LLM generated? What are the steps to test this? What is the output before and after the changes here?
9. 
   cuiweixie commented at 2:48 pm on February 28, 2026: none

   build test_bitcoin

   0cmake --build build -j$(sysctl -n hw.ncpu) --target test_bitcoin
   

   with this patch output ok like:

   0➜  bitcoin git:(bugfix) ./build/bin/test_bitcoin --run_test=descriptor_clone_tests 
   1Running 2 test cases...
   2
   3*** No errors detected
   

   without this patch output like:

    0➜  bitcoin git:(bugfix) ✗ ./build/bin/test_bitcoin --run_test=descriptor_clone_tests      
    1Running 2 test cases...
    2test/descriptor_tests.cpp:1392: error: in "descriptor_clone_tests/multisig_descriptor_clone": check clone->ToString(false) == descs[0]->ToString() has failed [wsh(multi(2))#ma2fnlvs != wsh(multi(2,03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd,0379e45b3cf75f9c5f9befd8e9506fb962f6a9d185ac87001ec44a8d3df8d4a9e3))#flt5crve]
    3Assertion failed: (m_subdescriptor_args.size() == m_depths.size()), function TRDescriptor, file descriptor.cpp, line 1515.
    4******** errors disabling the alternate stack:
    5        #error:22
    6        Invalid argument
    7unknown location:0: fatal error: in "descriptor_clone_tests/tr_descriptor_clone": signal: SIGABRT (application abort requested)
    8test/descriptor_tests.cpp:1409: last checkpoint
    9
   10*** 2 failures are detected in the test module "Bitcoin Core Test Suite"
   

10. 
    cuiweixie commented at 2:59 pm on February 28, 2026: none

    Mostly, I am writing golang. https://github.com/golang/go/commits?author=cuiweixie most pr is before LLM got popular.
    I am just using LLM to give me some tips(like test example) to understand the code base.
    And most code of this pr iis finish by myself.
    But I do use LLM to assit me, It’s something like google that I used to use mostly before LLM exists.
11. 
    maflcko commented at 4:16 pm on February 28, 2026: member

    Ok, I see.

    This is actually dead/unreachable code, so the fix isn’t urgent.

    About the test: I think it is a bit too spicy to include the c++ file in such a way to test this. I’d say it is fine to drop the commit, but no strong opinion.

12. maflcko removed the label Consensus on Feb 28, 2026
13. DrahtBot added the label Consensus on Feb 28, 2026
14. maflcko removed the label Consensus on Feb 28, 2026
15. maflcko added the label Refactoring on Feb 28, 2026
16. maflcko added the label Descriptors on Feb 28, 2026
17. cuiweixie force-pushed on Feb 28, 2026
18. 
    cuiweixie commented at 4:34 pm on February 28, 2026: none

    Ok, I see.

    This is actually dead/unreachable code, so the fix isn’t urgent.

    About the test: I think it is a bit too spicy to include the c++ file in such a way to test this. I’d say it is fine to drop the commit, but no strong opinion.

    Drop the unittest case commit.

19. 
    maflcko commented at 5:51 pm on February 28, 2026: member

    lgtm ACK 44feab23a7c6060a3b432c04e3f952c5a7104325

    Seems fine to fix UB in dead and unreachable code as a small style cleanup.

    However, for the pull description, I’d recommend to remove the sections

    • Changes: If anyone wanted to look at the changes, they could just look at them directly
    • Testing: This should say that it is dead/unreachable code. Maybe mention the test commit from earlier?
20. 
    cuiweixie commented at 1:08 am on March 1, 2026: none

    lgtm ACK 44feab2

    Seems fine to fix UB in dead and unreachable code as a small style cleanup.

    However, for the pull description, I’d recommend to remove the sections

    • Changes: If anyone wanted to look at the changes, they could just look at them directly
    • Testing: This should say that it is dead/unreachable code. Maybe mention the test commit from earlier?

    Done

21. 
    rkrux commented at 1:52 pm on March 2, 2026: contributor

    ACK 44feab23a7c6060a3b432c04e3f952c5a7104325 because it gets rid of the possible undefined behaviour.

Contributors
cuiweixie DrahtBot pinheadmz maflcko rkrux

Labels
Refactoring Descriptors