descriptors: Reject + sign while parsing unsigned #32365

1. 
   maflcko commented at 3:18 pm on April 28, 2025: member

   As a follow-up to #30577, reject + for unsigned values in key-path parsing and multi threshold parsing as well.

   Both of those are using unsigned, and Bitcoin Core would never serialize a descriptor string with a stray +. Accepting stray + signs could lead to checksum mismatches, or other incompatibilities later on.

   Just like #30577, both changes are breaking changes on the RPC interface, but hopefully no one should be relying on this behavior in production. Similarly, both changes should be fine for the wallet, because it normalizes the strings on import, see #30577#pullrequestreview-2218436014.

2. descriptors: Reject + sign in ParseKeyPathNum fa6f77ed3c
3. descriptors: Reject + sign when parsing multi threshold fa55dd01df
4. test: [refactor] Use ToIntegral in CheckInferDescriptor fa655da159
5. 
   DrahtBot commented at 3:18 pm on April 28, 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/32365.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	achow101, brunoerg, janb84

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

6. DrahtBot renamed this:
   descriptors: Reject + sign while parsing unsigned
   descriptors: Reject + sign while parsing unsigned
   on Apr 28, 2025
7. DrahtBot added the label Descriptors on Apr 28, 2025
8. 
   brunoerg commented at 5:17 pm on April 28, 2025: contributor

   Concept ACK
9. in src/test/descriptor_tests.cpp:570 in fa6f77ed3c outdated

   566@@ -567,6 +567,7 @@ BOOST_AUTO_TEST_CASE(descriptor_test)
   567     CheckUnparsable("combo([012345678]xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc)", "combo([012345678]xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL)", "combo(): Fingerprint is not 4 bytes (9 characters instead of 8 characters)"); // Too long key fingerprint
   568     CheckUnparsable("pkh(xprv9s21ZrQH143K31xYSDQpPDxsXRTUcvj2iNHm5NUtrGiGG5e2DtALGdso3pGz6ssrdK4PFmM8NSpSBHNqPqm55Qn3LqFtT2emdEXVYsCzC2U/2147483648)", "pkh(xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB/2147483648)", "pkh(): Key path value 2147483648 is out of range"); // BIP 32 path element overflow
   569     CheckUnparsable("pkh(xprv9s21ZrQH143K31xYSDQpPDxsXRTUcvj2iNHm5NUtrGiGG5e2DtALGdso3pGz6ssrdK4PFmM8NSpSBHNqPqm55Qn3LqFtT2emdEXVYsCzC2U/1aa)", "pkh(xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB/1aa)", "pkh(): Key path value '1aa' is not a valid uint32"); // Path is not valid uint
   570+    CheckUnparsable("pkh(xprv9s21ZrQH143K31xYSDQpPDxsXRTUcvj2iNHm5NUtrGiGG5e2DtALGdso3pGz6ssrdK4PFmM8NSpSBHNqPqm55Qn3LqFtT2emdEXVYsCzC2U/+1)", "pkh(xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB/+1)", "pkh(): Key path value '+1' is not a valid uint32"); // Path is not valid uint
   

   ---

   ---

   

   achow101 commented at 9:17 pm on April 28, 2025:

   In fa6f77ed3c152e0ff695c36b7a4ebb2c2efe916f “descriptors: Reject + sign in ParseKeyPathNum”

   Perhaps also a test for -?

10. 
    achow101 commented at 9:18 pm on April 28, 2025: member

    ACK fa655da159876861251d1149a5c31a830bd35596
11. DrahtBot requested review from brunoerg on Apr 28, 2025
12. brunoerg approved
13. 
    brunoerg commented at 10:32 am on April 29, 2025: contributor

    code review ACK fa655da159876861251d1149a5c31a830bd35596
14. 
    janb84 commented at 12:11 pm on April 29, 2025: contributor

    tACK fa655da
15. achow101 merged this on Apr 29, 2025
16. achow101 closed this on Apr 29, 2025

    ---

17. maflcko deleted the branch on Apr 29, 2025

Contributors
maflcko DrahtBot brunoerg achow101 janb84

Labels
Descriptors