Change Parse descriptor argument to string

Sjors commented at 11:32 am on November 20, 2025: member

While investigating a silent merge conflict in #33135 I noticed that #32983 changed the descriptor Parse function signature from const std::string& descriptor to std::span<const char> descriptor.

Calling that new version of Parse with a string literal will trigger a confusing “Invalid characters in payload” due to the trailing “\0”.

It can be worked around by having (the test) wrap string literals in std::string(), but that’s easy to forget.

Using string_view is easier and more compact than (as a previous version of this PR did) checking for trailing \0.

Also add a test.

DrahtBot commented at 11:32 am on November 20, 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/33914.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

in src/script/descriptor.h:186 in 06be8d79bc

177@@ -177,6 +178,17 @@ struct Descriptor {
178  */
179 std::vector<std::unique_ptr<Descriptor>> Parse(std::span<const char> descriptor, FlatSigningProvider& out, std::string& error, bool require_checksum = false);
180 
181+/** Convenience overload for string literals that trims the trailing null byte. */
182+template <size_t N>
183+std::vector<std::unique_ptr<Descriptor>> Parse(const char (&descriptor)[N], FlatSigningProvider& out, std::string& error, bool require_checksum = false)
184+{
185+    std::span sp{descriptor, N};
186+    if (!sp.empty() && sp.back() == '\0') {

Sjors commented at 11:55 am on November 20, 2025:

An alternative approach could be to not bother with this overload and simply have Parse do the removal at the top.

in src/script/descriptor.h:191 in 06be8d79bc

186+    if (!sp.empty() && sp.back() == '\0') {
187+        sp = sp.first(sp.size() - 1);
188+    }
189+    return ::Parse(sp, out, error, require_checksum);
190+}
191+

maflcko commented at 11:56 am on November 20, 2025:

This seems like a lot of bloat to re-implement std::string_view behavior? (Was this LLM generated? 😅 )

The correct fix seems to just change the Parse( function to accept a string_view instead. I guess this requires restoring the std::span<const char> sp{descriptor}; line, but this is just one line, and seems more acceptable than this here?

Sjors commented at 12:18 pm on November 20, 2025:

I could also just = delete the overload, since afaik only test code deals with string literal descriptors. See fe655448e2a3585683f312de71592acebf77267b.

Sjors commented at 12:26 pm on November 20, 2025:

Actually string_view seems to just work(tm), I’ll switch to that.

maflcko commented at 12:27 pm on November 20, 2025:

delete

Sure, but that doesn’t have any benefits, is still bloated, and doesn’t even fix the bug:

https://godbolt.org/z/6EeYvvarr

Sjors commented at 12:40 pm on November 20, 2025:

Interesting, on macOS the compiler refused Parse("pk(0279...798)" for me with a clear warning that the method was deleted. But anyway, string_view is better.

maflcko added this to the milestone 31.0 on Nov 20, 2025

maflcko added the label Bug on Nov 20, 2025

maflcko added the label Refactoring on Nov 20, 2025

Sjors force-pushed on Nov 20, 2025

Sjors renamed this:
~~Add string literal convenience overload to Parse()~~
Change Parse descriptor argument to string_view
on Nov 20, 2025

Sjors commented at 12:34 pm on November 20, 2025: member

Changed the approach to using string_view as suggested by @maflcko.

in src/script/descriptor.h:180 in 9d4c05d466

174@@ -175,7 +175,9 @@ struct Descriptor {
175  * If a parse error occurs, or the checksum is missing/invalid, or anything
176  * else is wrong, an empty vector is returned.
177  */
178-std::vector<std::unique_ptr<Descriptor>> Parse(std::span<const char> descriptor, FlatSigningProvider& out, std::string& error, bool require_checksum = false);
179+std::vector<std::unique_ptr<Descriptor>> Parse(const std::string_view descriptor, FlatSigningProvider& out, std::string& error, bool require_checksum = false);
180+
181+/** Disallow string literal due to its trailing null byte */

maflcko commented at 12:35 pm on November 20, 2025:

stale comment?

Sjors commented at 12:40 pm on November 20, 2025:

Dropped.

in src/script/descriptor.h:179 in 9d4c05d466 outdated

174@@ -175,7 +175,9 @@ struct Descriptor {
175  * If a parse error occurs, or the checksum is missing/invalid, or anything
176  * else is wrong, an empty vector is returned.
177  */
178-std::vector<std::unique_ptr<Descriptor>> Parse(std::span<const char> descriptor, FlatSigningProvider& out, std::string& error, bool require_checksum = false);
179+std::vector<std::unique_ptr<Descriptor>> Parse(const std::string_view descriptor, FlatSigningProvider& out, std::string& error, bool require_checksum = false);
180+

maflcko commented at 12:36 pm on November 20, 2025:

string_view is already on const char and the const here can be removed. See also a bit related: https://github.com/bitcoin/bitcoin/pull/31650

in src/script/descriptor.cpp:2749 in 9d4c05d466 outdated

2742@@ -2743,12 +2743,13 @@ bool CheckChecksum(std::span<const char>& sp, bool require_checksum, std::string
2743     return true;
2744 }
2745 
2746-std::vector<std::unique_ptr<Descriptor>> Parse(std::span<const char> descriptor, FlatSigningProvider& out, std::string& error, bool require_checksum)
2747+std::vector<std::unique_ptr<Descriptor>> Parse(const std::string_view descriptor, FlatSigningProvider& out, std::string& error, bool require_checksum)
2748 {
2749-    if (!CheckChecksum(descriptor, require_checksum, error)) return {};
2750+    std::span<const char> sp{descriptor};
2751+    if (!CheckChecksum(sp, require_checksum, error)) return {};

maflcko commented at 12:37 pm on November 20, 2025:

nit: I wonder how hard it would be to update most of those to use string_view instead of a span?

Sjors force-pushed on Nov 20, 2025

in src/test/descriptor_tests.cpp:1268 in 9d4c05d466

1261@@ -1262,4 +1262,15 @@ BOOST_AUTO_TEST_CASE(descriptor_test)
1262     CheckUnparsable("tr(musig(tuus(oldepk(gg)ggggfgg)<,z(((((((((((((((((((((st)", "tr(musig(tuus(oldepk(gg)ggggfgg)<,z(((((((((((((((((((((st)","tr(): Too many ')' in musig() expression");
1263 }
1264 
1265+BOOST_AUTO_TEST_CASE(descriptor_literal_null_byte)
1266+{
1267+    // Trailing '\0' string literal should be ignored.
1268+    {

maflcko commented at 12:37 pm on November 20, 2025:

nit: unused/double scope?

Sjors commented at 12:42 pm on November 20, 2025:

This used to be two tests, I’ll drop the extra scope.

DrahtBot added the label CI failed on Nov 20, 2025

Change Parse descriptor argument to string_view

Commit b3bf18f0bac0ffe18206ee20642e11264ba0c99d changed the function
signature from Parse(const std::string& descriptor,...) to
Parse(std::span<const char> descriptor,...).

Calling this new version of Parse with a string literal will trigger
a confusing "Invalid characters in payload" due to the trailing "\0".

Switch to string_view and add a test.

Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>

c0bfe72f6e

Sjors force-pushed on Nov 20, 2025

Sjors commented at 12:45 pm on November 20, 2025: member

Addressed nits.

I’ll look into if we can also use string_view in one or more of the calls we make, see #33914 (review). Update: meh, I’ll leave that up for grabs :-)

DrahtBot removed the label CI failed on Nov 20, 2025

in src/test/descriptor_tests.cpp:1270 in c0bfe72f6e

1261@@ -1262,4 +1262,13 @@ BOOST_AUTO_TEST_CASE(descriptor_test)
1262     CheckUnparsable("tr(musig(tuus(oldepk(gg)ggggfgg)<,z(((((((((((((((((((((st)", "tr(musig(tuus(oldepk(gg)ggggfgg)<,z(((((((((((((((((((((st)","tr(): Too many ')' in musig() expression");
1263 }
1264 
1265+BOOST_AUTO_TEST_CASE(descriptor_literal_null_byte)
1266+{
1267+    // Trailing '\0' string literal should be ignored.
1268+    FlatSigningProvider keys;
1269+    std::string err;
1270+    auto descs = Parse("pk(0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798)", keys, err, /*require_checksum=*/false);

ajtowns commented at 3:37 pm on November 20, 2025:

FYI, you could also make this a string_view literal, and leave the function as expecting a span<const char>, with

0#include <string_view>
1
2using namespace std::string_view_literals;
3
4auto descs = Parse("pk(0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798)"sv, keys, err, /*require_checksum=*/false);

Using string_view in the function declaration seems better though.

Change Parse descriptor argument to string_view #33914

Code Coverage & Benchmarks

Reviews