Follows up on unresolved suggestions from #22838. In order of priority:
for-loop nit [^3] and also limits variable scope.struct rather than comments.[^1]: #22838 (review) [^2]: #22838 (review) [^3]: #22838 (review) [^4]: #22838 (review) [^5]: #22838 (review)
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32134.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | Prabhat1308, l0rinc, mabu44, rkrux |
| Stale ACK | glozow |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
CC @glozow & @jonatack since these follow-ups are mostly based on your comments in the original PR (see references in PR description).
I ran into the broken formatting and 2/3 typo while looking at another PR and traced them back to 22838. Don't feel strongly about the two refactoring commits. Fine with dropping either/both to not hold the doc-commit hostage.
294 | - multi(2,xpub.../0/0/*,xpub.../2/*) 295 | - multi(2,xpub.../1/0/*,xpub.../3/*) 296 | - multi(2,xpub.../2/0/*,xpub.../4*) 297 | + multi(2,xpub.../0/0/*,xpub.../2/*) 298 | + multi(2,xpub.../1/0/*,xpub.../3/*) 299 | + multi(2,xpub.../2/0/*,xpub.../4*)
shouldn't the wildcard have its own position in the hierarchy?
multi(2,xpub.../2/0/*,xpub.../4/*)
Yes, I believe it should.
Thread https://github.com/bitcoin/bitcoin/pull/32134/files#r2011731171:
Good catch! (Edit: Just found a local branch from February where I had actually fixed that too. :sweat_smile:)
1495 | + if (!multipath) { 1496 | out.emplace_back(std::move(path)); 1497 | } else { 1498 | // Replace the multipath placeholder with each value while generating paths 1499 | - for (auto value : multipath_values) { 1500 | + for (auto value : multipath->values) {
nit: I had to check the type of value to make sure this isn't needlessly copied - to make it slightly simpler to review, consider:
for (const auto& value : multipath->values) {
Thread https://github.com/bitcoin/bitcoin/pull/32134/files#r2011736752:
Going with this for now:
for (uint32_t value : multipath->values) {
1466 | @@ -1464,6 +1467,7 @@ std::optional<uint32_t> ParseKeyPathNum(std::span<const char> elem, bool& apostr 1467 | return false; 1468 | } 1469 | 1470 | + multipath.emplace();
I don't like this third commit (nor the original implementation), we're looping over the splits, assigning a multipath if found, checking if it was already found during previous iterations, setting errors to output parameters and returning early, if we didn't return we assume it was correct, etc - it's too stateful, we're mixing not-strictly-related operations in a single loop, i.e. validation with usage, we should be able to separate the two concerns (it's a tiny loop).
I like the suggested change, it's cleaner and easier to follow. There's a bit of validation in the second loop as well but I guess there is no way to completely separate the validation from parsing. The alternative would be to parse the multipath nums in the first loop as well but I would prefer this implementation over that.
A comment before the first loop would be nice stating Validate multipath key path specifiers
Thread https://github.com/bitcoin/bitcoin/pull/32134/files#r2011759617:
Splitting validation from usage sounds good, but l0rincs version introduces more state and as rkrux mentioned still validates the parsing of the numbers in the "non-validating" loop. It's a bit more drastic than I was aiming for and still feels incomplete.
That version also undoes the move to keep the multipath data together towards the end, see list item 3 in the PR description. In the beginning it also uses std::pair with unnamed components instead of a struct with explanatory fields.
It's a bit more drastic than I was aiming for and still feels incomplete.
Even though I like the suggested change because of its intent, there are trade-offs, and as mentioned it's more than what the author originally intended to do in this pull. So, I will leave its fate to the author.
1481 | + multipath->values.emplace_back(*op_num); 1482 | } 1483 | 1484 | path.emplace_back(); // Placeholder for multipath segment 1485 | - multipath_segment_index = path.size()-1; 1486 | + multipath->segment_index = path.size()-1;
nit: since we're already touching this line we might as well format it
Could we separate the concerns of ParseKeyPath (validation from parsing)?
And I think there may be a typo in the doc.
<details> <summary>Suggestions</summary>
diff --git a/doc/descriptors.md b/doc/descriptors.md
index d5e845273a..60d3e0159f 100644
--- a/doc/descriptors.md
+++ b/doc/descriptors.md
@@ -288,11 +288,11 @@ For example, a descriptor of the form:
multi(2,xpub.../<0;1;2>/0/*,xpub.../<2;3;4>/*)
-will expand to the 3 descriptors
+will expand to the following descriptors:
multi(2,xpub.../0/0/*,xpub.../2/*)
multi(2,xpub.../1/0/*,xpub.../3/*)
- multi(2,xpub.../2/0/*,xpub.../4*)
+ multi(2,xpub.../2/0/*,xpub.../4/*)
When this tuple contains only two elements, wallet implementations can use the
first descriptor for receiving addresses and the second descriptor for change addresses.
diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
index d2224263d0..7a594f6e75 100644
--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cpp
@@ -1439,37 +1439,35 @@ std::optional<uint32_t> ParseKeyPathNum(std::span<const char> elem, bool& apostr
**/
[[nodiscard]] bool ParseKeyPath(const std::vector<std::span<const char>>& split, std::vector<KeyPath>& out, bool& apostrophe, std::string& error, bool allow_multipath)
{
- KeyPath path;
- struct Multipath {
- size_t segment_index;
- std::vector<uint32_t> values;
- };
- std::optional<Multipath> multipath;
-
- for (size_t i = 1; i < split.size(); ++i) {
- const std::span<const char>& elem = split[i];
-
+ std::optional<std::pair<size_t, std::vector<std::span<const char>>>> multipath_data;
+ for (size_t i{1}; i < split.size(); ++i) {
// Check if element contains multipath specifier
- if (!elem.empty() && elem.front() == '<' && elem.back() == '>') {
+ if (const auto& elem{split[i]}; elem.size() >= 2 && elem.front() == '<' && elem.back() == '>') {
if (!allow_multipath) {
error = strprintf("Key path value '%s' specifies multipath in a section where multipath is not allowed", std::string(elem.begin(), elem.end()));
return false;
- }
- if (multipath) {
+ } else if (multipath_data) {
error = "Multiple multipath key path specifiers found";
return false;
}
-
- // Parse each possible value
- std::vector<std::span<const char>> nums = Split(std::span(elem.begin()+1, elem.end()-1), ";");
- if (nums.size() < 2) {
+ auto spans{Split(std::span(elem.begin() + 1, elem.end() - 1), ";")};
+ if (spans.size() < 2) {
error = "Multipath key path specifiers must have at least two items";
return false;
}
- multipath.emplace();
+ multipath_data = {i, spans};
+ }
+ }
+
+ KeyPath path;
+ std::optional<size_t> multipath_segment_index;
+ std::vector<uint32_t> multipath_values;
+
+ for (size_t i{1}; i < split.size(); ++i) {
+ if (multipath_data && i == multipath_data->first) {
std::unordered_set<uint32_t> seen_multipath;
- for (const auto& num : nums) {
+ for (const auto& num : multipath_data->second) {
const auto& op_num = ParseKeyPathNum(num, apostrophe, error);
if (!op_num) return false;
auto [_, inserted] = seen_multipath.insert(*op_num);
@@ -1477,25 +1475,25 @@ std::optional<uint32_t> ParseKeyPathNum(std::span<const char> elem, bool& apostr
error = strprintf("Duplicated key path value %u in multipath specifier", *op_num);
return false;
}
- multipath->values.emplace_back(*op_num);
+ multipath_values.emplace_back(*op_num);
}
path.emplace_back(); // Placeholder for multipath segment
- multipath->segment_index = path.size()-1;
+ multipath_segment_index = path.size() - 1;
} else {
- const auto& op_num = ParseKeyPathNum(elem, apostrophe, error);
+ const auto& op_num = ParseKeyPathNum(split[i], apostrophe, error);
if (!op_num) return false;
path.emplace_back(*op_num);
}
}
- if (!multipath) {
+ if (!multipath_segment_index) {
out.emplace_back(std::move(path));
} else {
// Replace the multipath placeholder with each value while generating paths
- for (auto value : multipath->values) {
+ for (const auto& value : multipath_values) {
KeyPath branch_path = path;
- branch_path[multipath->segment_index] = value;
+ branch_path[*multipath_segment_index] = value;
out.emplace_back(std::move(branch_path));
}
}
</details>
Thanks for reviewing! Pushed improvements based on feedback.
ACK bd34c6008aa455d5b294db8977be471d1bc837df - not yet sure about the last commit, let's see what others think
1463 | @@ -1465,6 +1464,7 @@ std::optional<uint32_t> ParseKeyPathNum(std::span<const char> elem, bool& apostr 1464 | return false; 1465 | } 1466 | 1467 | + std::unordered_set<uint32_t> seen_multipath;
Good catch here, makes sense to reduce the life of this variable here. This set is not cleared anywhere in the function that led me to consider whether the older implementation was buggy but it was not because the multipath key specifier can only appear once in the descriptor as per the check on line 1456/1455.
The naming of this variable (not a new change) feels incomplete though. It's a set of derivation indexes/values but this term is not mentioned anywhere in the function. So, something like seen_multipath_nums (to keep it consistent with the existing terminology here) would be ok instead of what it is now.
Thanks for pointing this out. Context implies "multipath" now that the lifetime has been narrowed, making the naming poorer.
"nums" feels quite general though. Renamed to seen_substitutes in latest push. Made me modify an additional line but I think it might be worth it.
1439 | @@ -1440,8 +1440,11 @@ std::optional<uint32_t> ParseKeyPathNum(std::span<const char> elem, bool& apostr 1440 | [[nodiscard]] bool ParseKeyPath(const std::vector<std::span<const char>>& split, std::vector<KeyPath>& out, bool& apostrophe, std::string& error, bool allow_multipath) 1441 | { 1442 | KeyPath path; 1443 | - std::optional<size_t> multipath_segment_index; 1444 | - std::vector<uint32_t> multipath_values; 1445 | + struct Multipath { 1446 | + size_t segment_index; 1447 | + std::vector<uint32_t> values;
Again these are derivation indexes/values, so derivation_values can be an option instead of just values, which feels bland.
I would conceptually call them derivation path components... :) But like shorter names, changed to substitutes. How about that?
Also changed segment_index to placeholder_index, inspired by the code comment
// Replace the multipath placeholder with each value while generating paths
I'd have used multipath_substitutes at the cost of being a bit more verbose but this is also fine.
+1 for using placeholder, makes it easier to follow.
1439 | @@ -1440,8 +1440,11 @@ std::optional<uint32_t> ParseKeyPathNum(std::span<const char> elem, bool& apostr 1440 | [[nodiscard]] bool ParseKeyPath(const std::vector<std::span<const char>>& split, std::vector<KeyPath>& out, bool& apostrophe, std::string& error, bool allow_multipath) 1441 | { 1442 | KeyPath path; 1443 | - std::optional<size_t> multipath_segment_index; 1444 | - std::vector<uint32_t> multipath_values; 1445 | + struct Multipath {
Using struct here seems reasonable to me in order to tie these two related variables together.
crACK 1e1f11e8946126180f4a93d6d99d5981ecfc7cd3
I shared couple naming suggestions though non-blocking.
Attempted to improve naming in latest push in response to #32134#pullrequestreview-2721049447.
1439 | @@ -1440,20 +1440,22 @@ std::optional<uint32_t> ParseKeyPathNum(std::span<const char> elem, bool& apostr 1440 | [[nodiscard]] bool ParseKeyPath(const std::vector<std::span<const char>>& split, std::vector<KeyPath>& out, bool& apostrophe, std::string& error, bool allow_multipath) 1441 | { 1442 | KeyPath path; 1443 | - std::optional<size_t> multipath_segment_index; 1444 | - std::vector<uint32_t> multipath_values; 1445 | - std::unordered_set<uint32_t> seen_multipath; 1446 | + struct Multipath { 1447 | + size_t placeholder_index; 1448 | + std::vector<uint32_t> substitutes;
I don't like the name substitutes. It implies that it is an alternative to a one major path which I don't think is the case ?
Pushed new version that hopefully clarifies things: https://github.com/bitcoin/bitcoin/blob/7aa397a7d45ce0ae1b83cc6f3a65db0e41b21ee8/src/script/descriptor.cpp#L1442-L1447
Using substitutes seems reasonable to me as the calculated derivation path indices are indeed substituting the multipath specifier nums.
crACK 78327f9
1496 | + if (!substitutes) { 1497 | out.emplace_back(std::move(path)); 1498 | } else { 1499 | // Replace the multipath placeholder with each value while generating paths 1500 | - for (size_t i = 0; i < multipath_values.size(); i++) { 1501 | + for (uint32_t subtitute : substitutes->values) {
nit: substitute
Thanks! Fixed.
* Range-for avoids ++i/i++ debate and decreases linecount.
* seen_multipath is only used if multipath_segment_index hasn't already been set. Rename it to seen_substitutes to better describe what it does, now that the context implies its involved in multipath.
re-ACK 56f271e
1463 | @@ -1465,10 +1464,11 @@ std::optional<uint32_t> ParseKeyPathNum(std::span<const char> elem, bool& apostr 1464 | return false; 1465 | } 1466 | 1467 | + std::unordered_set<uint32_t> seen_substitutes;
commit message still contains seen_multipath
It also contains:
Rename it to seen_substitutes to better describe what it does, now that the context implies its involved in multipath.
ok, if that's deliberate, please resolve this
1466 | @@ -1464,6 +1467,7 @@ std::optional<uint32_t> ParseKeyPathNum(std::span<const char> elem, bool& apostr 1467 | return false; 1468 | } 1469 | 1470 | + substitutes.emplace();
this implicit empty constructor was always weird to me, a more explicit and uniform alternative could be:
substitutes = {};
To me .emplace() appears more specific. Assigning {} seems to make it std::nullopt, leading to test failures.
My mistake, thanks for checking
utACK 56f271e9b9c82f40054d63d4b638584bd2faef00
tACK 56f271e9b9c82f40054d63d4b638584bd2faef00
<details> <summary>Testing procedure</summary>
./bitcoind -regtest
./bitcoin-cli createwallet pr32134 false true
./bitcoin-cli importdescriptors '[{"desc":"wpkh(tprv8ZgxMBicQKsPdGJed5SjAaeKBLjKoYKbp4E8855hsG3HNiFuaHNprjb4UmuovVsvAQ7Hwa2MdBsrgyhePCyVgMPnTgQdigFFZLCJTkNm7CK/<0;1>/*)#p5mvj4c3","timestamp":0,"range":[0,999],"active":true}]'
Output:
[
{
"success": true
}
]
./bitcoin-cli listdescriptors
Output:
{
"wallet_name": "pr32134",
"descriptors": [
{
"desc": "wpkh(tpubD6NzVbkrYhZ4WjLSWj7KZzJRkNFFxsWWPMpuQb81HXqgDCWgCgCR3ECvetieSgGdd4DWgzikbbG8jHHSxkrVD1TjpaRoBHuJaXQibLpF5qQ/0/*)#979qycjr",
"timestamp": 1,
"active": true,
"internal": false,
"range": [
0,
999
],
"next": 0,
"next_index": 0
},
{
"desc": "wpkh(tpubD6NzVbkrYhZ4WjLSWj7KZzJRkNFFxsWWPMpuQb81HXqgDCWgCgCR3ECvetieSgGdd4DWgzikbbG8jHHSxkrVD1TjpaRoBHuJaXQibLpF5qQ/1/*)#52qpedzm",
"timestamp": 1,
"active": true,
"internal": true,
"range": [
0,
999
],
"next": 0,
"next_index": 0
}
]
}
Also moved some regtest coins around and it works as expected.
</details>
crACK 56f271e9b9c82f40054d63d4b638584bd2faef00
Overall, I believe this change leaves the code in a more readable state imho.