scripted-diff: Update DeriveType enum values to mention ranged derivations #32745

1. 
   rkrux commented at 10:56 am on June 13, 2025: contributor

   While reviewing the MuSig2 descriptors PR #31244, I realized that the enum DeriveType here logically refers to the derive type for ranged descriptors. This became evident to me while going through the implementations of IsRange & IsHardened functions of BIP32PubkeyProvider, and the ParsePubkeyInner function. Initially I got confused by reading IsRange translating to != DeriveType::NO, but later realised it specifically referred to the presence of ranged derivations. I propose explicitly mentioning “ranged” in the values of the DeriveType enum would make it easier to parse the descriptors code.

   This enum is used in one file only - script/descriptors.cpp. That’s why I explicitly passed it as the argument in the sed command in the script.

2. 
   DrahtBot commented at 10:56 am on June 13, 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/32745.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept ACK	l0rinc
   Stale ACK	hodlinator, PeterWrighten

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

   Conflicts

   No conflicts as of last run.

3. DrahtBot added the label Refactoring on Jun 13, 2025
4. rkrux force-pushed on Jun 13, 2025
5. 
   PeterWrighten commented at 11:28 am on June 13, 2025: none

   Good catch! But I consider you should better check other codes which also use this DeriveType, to make sure it is only used for Ranged descriptors.
6. rkrux marked this as ready for review on Jun 13, 2025
7. in src/script/descriptor.cpp:358 in f64880672b outdated

   351@@ -352,7 +352,7 @@ class ConstPubkeyProvider final : public PubkeyProvider
   352     }
   353 };
   354 
   355-enum class DeriveType {
   356+enum class RangedDeriveType {
   357     NO,
   358     UNHARDENED,
   359     HARDENED,
   

   ---

   ---

   

   hodlinator commented at 12:40 pm on June 13, 2025:

   Agree that based on the IsRange() implementation, this enum could be clearer. But would lean towards only changing the values, not the name in this case:

   0enum class DeriveType {
   1    SINGLE,
   2    UNHARDENED_RANGE,
   3    HARDENED_RANGE,
   

   ---

   

   rkrux commented at 11:46 am on June 14, 2025:

   Accepted the suggestion with some modification by using NON_RANGED instead of SINGLE, which felt incorrect to me. I do like reading DeriveType::NON_RANGED instead of the original DeriveType::NO, which made me feel like there was no child derivation at all and prompted me to raise the PR.

   I had to experiment with the sed command in the scripted diff a bit to get the right order working but it was fun.

   ---

   

   hodlinator commented at 9:20 pm on June 14, 2025:

   Happy with your variation. :+1:
8. rkrux force-pushed on Jun 14, 2025
9. rkrux renamed this:
   scripted-diff: Replace `DeriveType` with `RangedDeriveType`
   scripted-diff: Update DeriveType enum values to mention ranged derivations
   on Jun 14, 2025
10. in src/script/descriptor.cpp:1 in d60b0088e8

    ---

    ---

    

    hodlinator commented at 9:17 pm on June 14, 2025:

    Commit message nits:

    1. Could include # to more closely conform to https://github.com/bitcoin/bitcoin/blob/d60b0088e81e2ef81b9cfe2d4b808ed819a52962/CONTRIBUTING.md?plain=1#L127-L129
    2. Correct method name
    3. Adjust narrowness of script in both direction through relying more on word boundaries (\b), ending up with 2 lines instead of 3.

     0  scripted-diff: Update `DeriveType` enum values to mention ranged derivations
     1
     2- While reviewing the MuSig2 descriptors PR 31244, I realized that the enum
     3+ While reviewing the MuSig2 descriptors PR [#31244](/bitcoin-bitcoin/31244/), I realized that the enum
     4  `DeriveType` here logically refers to the derive type for ranged descriptors.
     5- This became evident to me while going through the implementations of `IsRanged`
     6+ This became evident to me while going through the implementations of `IsRange`
     7  & `IsHardened` functions of `BIP32PubkeyProvider`, and the `ParsePubkeyInner`
     8  function. Initially I got confused by reading `IsRange` translating to
     9  `!= DeriveType::NO`, but later realised it specifically referred to the presence
    10  of ranged derivations. I propose explicitly mentioning "ranged" in the values
    11  of the `DeriveType` enum would make it easier to parse the descriptors code.
    12
    13  This enum is used in one file only - `script/descriptors.cpp`. That's why I
    14  explicitly passed it as the argument in the `sed` command in the script below.
    15
    16  -BEGIN VERIFY SCRIPT-
    17- sed -i 's/HARDENED/HARDENED_RANGED/g' src/script/descriptor.cpp
    18+ sed -i 's/HARDENED\b/HARDENED_RANGED/g' src/script/descriptor.cpp
    19- sed -i 's/NO,/NON_RANGED,/g' src/script/descriptor.cpp
    20- sed -i 's/DeriveType::NO/DeriveType::NON_RANGED/g' src/script/descriptor.cpp
    21+ sed -i 's/\bNO\b/NON_RANGED/g' src/script/descriptor.cpp
    22  -END VERIFY SCRIPT-
    

    ---

    

    l0rinc commented at 7:41 am on June 15, 2025:

    Would seem even simpler if there was a single rename per commit. Not sure the scripted diff helps here and I wouldn’t repeat the commit message (especially the script) in the PR description.

    ---

    

    rkrux commented at 7:58 am on June 16, 2025:

    @hodlinator: Good suggestion regarding the word boundary, I have used it along with function name correction. Regarding adding #, it is intentional on my part to omit it in the commit message and add it in the PR description. Some time back, it was highlighted to me that adding a PR reference in a commit message leads to the presence of a new commit entry in the referred PR on Github every time the PR containing the commit is force-pushed, which can clutter the referred PR. Hence, I make it a point to add the other-PR references only in the PR description. @l0rinc: Thanks for highlighting, I have removed the script from the PR description. I usually let the commit message be present in the PR description to give context to the reviewers in case of single-commit PRs. Agree that it might feel repetitive in the commit history after the PR is merged. I can remove most of it from the description if you (or others) feel strongly about the message duplication in the commit log. Regarding single rename per commit and the possibility of not using scripted-diff: No strong opinion from my side, I was under the impression that multiple replacement changes are preferred to be done via scripted diffs and these changes didn’t seem big enough for me to split them into multiple commits along with the fact that all these values belong to the same enum.

    ---

    

    hodlinator commented at 9:28 am on June 16, 2025:

    Also think it might be slightly safer to not do it as a scripted diff as NO is pretty general, but it’s only run on one file, so fine either way.
11. 
    hodlinator commented at 9:22 pm on June 14, 2025: contributor

    Code reviewed d60b0088e81e2ef81b9cfe2d4b808ed819a52962
12. 
    PeterWrighten commented at 7:51 am on June 15, 2025: none

    Concept ACK d60b008 It’s a good refactoring for understanding, but perhaps you should still check and modify other documents to make sure it would not missing the explanation of this change in them.
13. rkrux force-pushed on Jun 16, 2025
14. 
    rkrux commented at 7:58 am on June 16, 2025: contributor

    Concept ACK d60b008 It’s a good refactoring for understanding, but perhaps you should still check and modify other documents to make sure it would not missing the explanation of this change in them.

    Yes, I had confirmed regarding this, and I did add a note in the commit message & the PR description that all these enum values are contained within one file only.

15. hodlinator approved
16. 
    hodlinator commented at 9:34 am on June 16, 2025: contributor

    ACK dce5ed53bb480c6232125e8e8480154b577a04f9

    Clearer names for enum values given BIP32PubkeyProvider::IsRange:

    https://github.com/bitcoin/bitcoin/blob/d91c718a686abe4865201bf3ef1db785c5677167/src/script/descriptor.cpp#L407

17. DrahtBot requested review from PeterWrighten on Jun 16, 2025
18. 
    l0rinc commented at 10:54 am on June 16, 2025: contributor

    Concept ACK, didn’t review it very thoroughly - ping me if you need a full review
19. 
    PeterWrighten commented at 3:40 pm on June 16, 2025: none

    Code Reviewed dce5ed5
20. DrahtBot requested review from PeterWrighten on Jun 16, 2025
21. 
    PeterWrighten commented at 3:43 pm on June 16, 2025: none

    ACK dce5ed5

    LGTM.

22. DrahtBot requested review from l0rinc on Jun 16, 2025
23. DrahtBot added the label Needs rebase on Jul 31, 2025
24. scripted-diff: Update `DeriveType` enum values to mention ranged derivations

    While reviewing the MuSig2 descriptors PR 31244, I realized that the enum
    `DeriveType` here logically refers to the derive type for ranged descriptors.
    This became evident to me while going through the implementations of `IsRange`
     & `IsHardened` functions of `BIP32PubkeyProvider`, and the `ParsePubkeyInner`
    function. Initially I got confused by reading `IsRange` translating to
    `!= DeriveType::NO`, but later realised it specifically referred to the presence
     of ranged derivations. I propose explicitly mentioning "RANGED" in the values
    of the `DeriveType` enum would make it easier to parse the descriptors code.
    
    This enum is used in one file only - `script/descriptors.cpp`. That's why I
    explicitly passed it as the argument in the `sed` commands in the script below.
    
    -BEGIN VERIFY SCRIPT-
    sed -i 's/HARDENED\b/HARDENED_RANGED/g' src/script/descriptor.cpp
    sed -i 's/\bNO\b/NON_RANGED/g' src/script/descriptor.cpp
    -END VERIFY SCRIPT-

    a099655f2e
25. rkrux force-pushed on Aug 1, 2025
26. 
    rkrux commented at 12:08 pm on August 1, 2025: contributor

    Rebased over master to incorporate changes from #31244.
27. DrahtBot removed the label Needs rebase on Aug 1, 2025

Contributors
rkrux DrahtBot PeterWrighten hodlinator l0rinc

Review Requested
l0rinc

Labels
Refactoring