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

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.

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
ACK	hodlinator, PeterWrighten
Concept ACK	l0rinc

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31244 (descriptors: MuSig2 by achow101)

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.

DrahtBot added the label Refactoring on Jun 13, 2025

rkrux force-pushed on Jun 13, 2025

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.

rkrux marked this as ready for review on Jun 13, 2025

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:

rkrux force-pushed on Jun 14, 2025

rkrux renamed this:
~~scripted-diff: Replace `DeriveType` with `RangedDeriveType`~~
scripted-diff: Update DeriveType enum values to mention ranged derivations
on Jun 14, 2025

in src/script/descriptor.cpp:1 in d60b0088e8

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

Commit message nits:

Could include # to more closely conform to https://github.com/bitcoin/bitcoin/blob/d60b0088e81e2ef81b9cfe2d4b808ed819a52962/CONTRIBUTING.md?plain=1#L127-L129
Correct method name
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.

hodlinator commented at 9:22 pm on June 14, 2025: contributor

Code reviewed d60b0088e81e2ef81b9cfe2d4b808ed819a52962

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.

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` command 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-

dce5ed53bb

rkrux force-pushed on Jun 16, 2025

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.

hodlinator approved

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

DrahtBot requested review from PeterWrighten on Jun 16, 2025

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

PeterWrighten commented at 3:40 pm on June 16, 2025: none

Code Reviewed dce5ed5

DrahtBot requested review from PeterWrighten on Jun 16, 2025

PeterWrighten commented at 3:43 pm on June 16, 2025: none

ACK dce5ed5

LGTM.

DrahtBot requested review from l0rinc on Jun 16, 2025

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

Code Coverage & Benchmarks

Reviews

Conflicts