Conversation
KyrylR
marked this pull request as ready for review
September 30, 2026 10:49
KyrylR
marked this pull request as draft
September 30, 2026 10:55
Contributor
Author
|
With this PR I learned more about parsing descriptors, I believe only tests from this PR are relevant due to the proposed solution here: #108 (comment) Closing this PR in favor of first PR that will follow steps proposed here: #108 (comment) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes: #105
Overall with this PR the #105 will be fixed, there are separate issues mentioned in the end that are worth checking
In comparison to before the code became simpler
Rational (created with a help of GPT Astra)
Consider:
The new
new_count == 0check here: https://github.com/KyrylR/elements-miniscript/blob/fa1725eea6d03a45735440da0c46ef8944f13598/src/expression.rs#L117 separates the first closing parenthesis inpk(A)and the next closingeltr.“Brace mode” also works for a single leaf without braces. It preserves the leaf’s parentheses as part of its text while looking for tree separators.
In https://github.com/KyrylR/elements-miniscript/blob/fa1725eea6d03a45735440da0c46ef8944f13598/src/descriptor/tr.rs#L693, the standalone parser becomes:
Line by line:
That makes the old
parse_tr_treeand its customsplit_onceunnecessary. Their responsibilities move to shared parsing: recognizing arguments, checking delimiters, rejecting leftover input, and constructing the expression tree. The removed imports were only needed by those helpers.Note
One subtle behavior change deserves care: the old parser could accept
eltr(A,)with permissiveStringkeys by treating"A,"as the key. Real public-key types already rejected that text. The new rejection removes that generic-key quirk.In https://github.com/KyrylR/elements-miniscript/blob/fa1725eea6d03a45735440da0c46ef8944f13598/src/descriptor/mod.rs#L1191, the second commit moves extension selection into
from_tree:Tr::<Pk, NoExt>first tries ordinary Miniscript.Ok(tr)preserves the normalDescriptor::Trvariant.Err(_)triggers another attempt using extension typeT.Descriptor::TrExt.?returns an error if that attempt also fails.This fallback previously lived only in
Descriptor::from_str. Moving it intofrom_treemeans confidential wrappers reach it too.Then https://github.com/KyrylR/elements-miniscript/blob/fa1725eea6d03a45735440da0c46ef8944f13598/src/descriptor/mod.rs#L1214-L1216 give ordinary descriptors one parsing sequence: checksum, expression tree, typed descriptor. The special
eltrstring-prefix branch disappears.There are two follow-up issues that probably should be addressed separately from the parser changes.
Validate confidential descriptors before ELIP151 derivation. The confidential parser enters through
from_tree, bypassing the standalone multipath-length check. ELIP151 code then assumes expansion succeeds usingexpect. Invalid multipath combinations can therefore panic instead of returning an error. The defensive fix is to validate the inner descriptor before deriving the view key and propagate expansion errors.Reject unsupported Taproot pegin descriptors before use. Pegin parsing accepts ordinary descriptors through the shared converter, but later consumers call
explicit_script().expect(...), including dynafed pegin and legacy pegin. Taproot deliberately returnsTrNoScriptCodefrom that method. Reject unsupportedTrandTrExtdescriptors at the pegin boundary, or make the consumers return an appropriate error.Both problems predate this branch. The new parser makes additional tree forms reach them.