Use mathematical notation for semantic policy Display by febyeji · Pull Request #914 · rust-bitcoin/rust-miniscript

febyeji · 2026-04-01T04:40:38Z

Summary

Change semantic::Policy Display impl to use mathematical notation:
- and(a,b) → (a ∧ b)
- or(a,b) → (a ∨ b)
- thresh(k,a,b,c) → #{a, b, c} = k

Close #926

apoelstra · 2026-04-02T16:17:01Z

thresh(k,a,b,c) → #{a, b, c} ≥ k

The correct inequality is =, not ≥. If you provide too many satisfactions your witness will be invalid. But concept ACK using this brace/comma notation. I think it will be easier to read in a large expression than my notation was.

thresh requires exactly k satisfactions to be valid, not k or more. Providing too many satisfactions makes the witness invalid. Addresses apoelstra's review feedback on PR rust-bitcoin#914.

febyeji · 2026-04-02T17:31:38Z

Thanks for the review! Changed ≥ to = and added a doc comment to Display noting the notation and that it's not parseable.

apoelstra · 2026-04-02T17:48:27Z

Can you squash these commits, so that there isn't one modifying code introduced by the other?

tcharding · 2026-04-21T05:35:36Z

Do we want to close the issue if this merges?

tcharding

ACK 73ee818

trevarj · 2026-04-21T13:49:13Z

Do we want to close the issue if this merges?

@tcharding Don't close #885 until issues are made for these at least

We rename policy::Concrete to policy::Policy, leaving a type alias to minimize code breakage.

We move policy::Semantic into its own module so it becomes semantic::Semantic (we can also leave a type alias at the old location but we should deprecate it). We also move the Lift trait, which I think is in policy/mod.rs, to this own module (we can re-export it at the old location, but sadly we cannot deprecate re-exports).

We stop referring to policy::Semantic as a "semantic policy" or a "policy" at all anywhere in our docs

We change the Display impl for Semantic to output something goofy, like instead of and(a, b, c) it does (a ∧ b ∧ c). Whoever PRs to do this should provide several examples so we can debate this, especially the threshold notation. (Maybe we want (2 = a + b + c) for thresh(2, a, b, c)?)

Furthermore, I propose adding a method to "uncompile" from a Miniscript to a Policy (or even a Semantic to a Policy), but we should move that to a separate issue because I'm quite conflicted about what to do about percentages.

apoelstra · 2026-04-21T17:00:34Z

No, to close the issue we've also got to do the renames and doc changes to stop referring to semantic policies as "policies".

tcharding · 2026-04-21T22:46:18Z

@febyeji I've changed your PR description to close the new #926. Please check you are happy with that.

febyeji · 2026-04-22T09:53:56Z

@tcharding Looks good, thanks for updating the PR body. Happy with that.

apoelstra · 2026-04-22T20:30:06Z

In eb6011e:

This parser is gratuitously inefficient. You should be able to just parse the new syntax directly. It is a simple CFG and I don't think it even needs lookahead.

You definitely do not need to rewrite the string into the old syntax, and even if you did, this manages to be both recursive (we should not be introducing any new recursive code) and still manages to rescan the entire string on every iteration.

…ss parser Replace the recursive `rewrite_math_notation` that rescanned the input at every depth with a single linear pass that builds `Policy<Pk>` directly using an explicit frame stack. Matches `expression::Tree`'s non-recursive conventions. Addresses apoelstra's review on rust-bitcoin#914.

febyeji · 2026-04-24T05:29:25Z

I simplified the parser and added hybrid rejection. The math parser now refuses atoms whose root name is and/or/thresh, so strings like (a ∧ or(b, c)) and similar won't parse.

apoelstra · 2026-04-24T16:43:24Z

That solution is insane. I feel like I'm just arguing with a LLM by proxy here. I am not going to provide detailed feedback anymore.

febyeji · 2026-04-27T05:30:03Z

@apoelstra Apologies for the back-and-forth. I wrote the parser myself, but without carefully understanding the codebase. I was trying to be conservative and reuse expression::Tree, which compounded the mess since I had also misunderstood your intent and feedback.

I've rewritten the code. If the result isn't what you had in mind, please feel free to close the PR. I will just leave one note in case it helps resolving the issue later.

In the current code, FromStr accepts the legacy form as well to avoid breaking existing tests and users, but I'm not sure if FromStr should accept only the new format.

trevarj

Leaving some starting feedback on the parser. If the suggestions get implemented then I will do another review and see if we can refine it more.

You may want to not force push and start stacking some commits in case Andrew or someone else doesn't like a change and then you can drop that commit and rebase later.

trevarj · 2026-04-27T07:11:01Z

+    }
+}
+
+/// Grammar:


Andrew said that we don't need a BNF grammar in the doc comment.
It's a pretty simple grammar, but I prefer a more simplified notation anyway (like regex style for human readability).

I don't like how the grammar includes whitespace. I think it would be better to always skip whitespace to accept pk(A) ∧ pk(B).

Thanks for the review. I changed the code to accept whitespaces except for the digraph #{. I dropped the BNF grammar from the doc comment as well.

trevarj · 2026-04-27T07:13:13Z

+        kind: Kind,
+    }
+
+    fn match_sep(s: &str) -> Option<Op> {


Drop whitespace and then you can have a impl From<&str> for Op and have the _ case be unreachable!() instead of this.

I refactored the code, but used From<char> instead of From<&str> since Op are of a single literal and we can avoid constructing &str from the call site. Happy to switch &str if it's a better option to consider.

You are correct, should be char!

trevarj · 2026-04-27T07:26:34Z

+        }
+    }
+
+    let bytes = s.as_bytes();


I would prefer using a s.chars().peekable() (or char_indices()) Iterator.
I think it will make things more readable and you won't have to index into the byte slice and skip utf8 bytes. It's not a huge problem, but I think it's just nicer Rust.

After that, then you can have really nice looking match statements on the main while loop. It's more idiomatic Rust IMO.

You'll probably need some "traditional" parser/lexer helper functions, like skipping whitespace and reading until some char, so it may be worth it to wrap the iterator in a MathParser struct or something.

Thank you for the guidance. I refactored the code to wrap the parser state in a MathParser struct with a few helpers like peek, bump, .... Most of the byte-index manipulation is now hidden, though some match arms may still not be ergonomic to read. Please let me know if there's room to push this further.

trevarj · 2026-04-27T07:29:55Z

+            frame.subs.push(cur.take().unwrap());
+            i += 5; // " ∧ " / " ∨ " in UTF-8
+        } else if bytes[i] == b',' && bytes.get(i + 1) == Some(&b' ') {
+            if !matches!(frame.kind, Kind::Thresh) {


Derive PartialEq, Eq on Kind and you can get rid of matches! like this and just use != here and in the other areas.

Thanks, Kind now derives the traits and directly uses == / !=.

trevarj · 2026-04-27T07:36:26Z

+            }
+            i += 3;
+            let k_start = i;
+            while i < bytes.len() && bytes[i].is_ascii_digit() {


For example, this is copied a couple times and could be replaced with some helper to consume until some char, I think.

For this one, I added consume_while(predicate) helper to MathParser

trevarj

I think the parser is turning out pretty good, I like it.
Probably my last round of review since everything else seems ok to me.
Thanks!

trevarj · 2026-04-28T07:43:42Z

+    loop {
+        parser.skip_ws();
+        let c = match parser.peek() {
+            Some(c) => c,
+            None => break,
+        };


Does this still work if it's changed to?:

parser.skip_ws(); while let Some(c) = parser.peek() { ... parser.skip_ws(); // <- last statementin block }

I'm not actually sure what is better...just a nit and can ignore.

I've thought about it and would work if we put parser.skip_ws() before continue as well. For now, I kept as it is to have loop -> skip_ws -> peek-or-break format.

trevarj · 2026-04-28T07:45:21Z

-    enum Op {
-        And,
-        Or,
+struct MathParser<'a> {


Can we get doc comments here and in the impl? Thanks!

trevarj · 2026-04-28T07:46:39Z

+}
+
+/// Parses the mathematical-notation form produced by `Display`.
+fn parse_math_policy<Pk: FromStrKey>(s: &str) -> Result<Policy<Pk>, Error> {


Let's move this and the other parse_* functions into MathParser.
Then we can give it a good doc comment explaining the high level overview of how the parser works.

trevarj · 2026-04-28T07:50:14Z

+}

-    let bytes = s.as_bytes();
+struct Frame<Pk: MiniscriptKey> {


Another doc comment here explaining what the use of Frame is used for.

trevarj · 2026-04-28T07:53:03Z

+        .map_err(Error::Parse)
+}
+
+fn malformed_math() -> Error {


I think it would be better to add a context: &str arg and display that so the user knows exactly where the policy is malformed. Need to update callers appropriately.

Thanks, I updated this along with expect.

trevarj · 2026-04-28T07:56:06Z

+
+    fn peek(&mut self) -> Option<char> { self.iter.peek().map(|&(_, c)| c) }
+
+    fn bump(&mut self) -> Option<char> { self.iter.next().map(|(_, c)| c) }


nit: I'd rather have next() unless you think it's better to use bump. Some people use chomp...

Suggested change

fn bump(&mut self) -> Option<char> { self.iter.next().map(|(_, c)| c) }

fn next(&mut self) -> Option<char> { self.iter.next().map(|(_, c)| c) }

trevarj · 2026-04-28T07:59:21Z

+        }
+    }
+
+    fn try_consume(&mut self, want: char) -> bool {


Suggested change

fn try_consume(&mut self, want: char) -> bool {

fn expect(&mut self, want: char) -> Result<(), Error> {

This way you can get rid of the return Err(malformed_math()); that is scattered around and just use parser.expect('(')?;

Right, it's much clearer. Thanks!

trevarj · 2026-04-28T12:46:41Z

+            return Err(malformed_math("expected terminal name"));
+        }
+
+        if !self.try_consume('(') {


I would remove the try_consume function fully and then use:

Suggested change

if !self.try_consume('(') {

if let Err(e) = self.expect('(') {

Oh, thank you. I missed this line.

trevarj

ACK 511e81b

Don't forget to squash commits later. Probably last 5 into a single commit?

febyeji force-pushed the semantic-policy-display branch from a0d28b3 to aee1d82 Compare April 1, 2026 05:50

febyeji force-pushed the semantic-policy-display branch from b796520 to 73ee818 Compare April 2, 2026 18:09

tcharding approved these changes Apr 21, 2026

View reviewed changes

apoelstra reviewed Apr 21, 2026

View reviewed changes

Comment thread fuzz/fuzz_targets/regression_descriptor_parse.rs Outdated

tcharding mentioned this pull request Apr 21, 2026

Fix Display impl for Semantic #926

Open

apoelstra reviewed Apr 22, 2026

View reviewed changes

Comment thread src/policy/semantic.rs Outdated

febyeji force-pushed the semantic-policy-display branch 2 times, most recently from 5a0282a to eb6011e Compare April 22, 2026 14:06

febyeji force-pushed the semantic-policy-display branch from eb6011e to 9852620 Compare April 23, 2026 11:12

trevarj reviewed Apr 23, 2026

View reviewed changes

Comment thread src/policy/semantic.rs Outdated

febyeji force-pushed the semantic-policy-display branch 2 times, most recently from 9201b03 to 357554e Compare April 24, 2026 05:04

febyeji marked this pull request as draft April 27, 2026 03:23

feat(policy): change semantic::Policy Display to mathematical notation

1384586

febyeji force-pushed the semantic-policy-display branch from 357554e to 1384586 Compare April 27, 2026 05:29

trevarj reviewed Apr 27, 2026

View reviewed changes

febyeji added 2 commits April 28, 2026 15:21

refactor(policy): simplify semantic Policy parser

ca896a7

test(policy): add roundtrip test for semantic Display

3dfc3d4

trevarj suggested changes Apr 28, 2026

View reviewed changes

febyeji added 2 commits April 28, 2026 20:27

refactor(policy): clean up semantic Policy math parser

58a42f7

style(policy): apply nightly fmt

e6a2ba6

trevarj reviewed Apr 28, 2026

View reviewed changes

refactor(policy): drop try_consume helper

511e81b

trevarj approved these changes Apr 28, 2026

View reviewed changes


		fn peek(&mut self) -> Option<char> { self.iter.peek().map(\|&(_, c)\| c) }

		fn bump(&mut self) -> Option<char> { self.iter.next().map(\|(_, c)\| c) }

	fn bump(&mut self) -> Option<char> { self.iter.next().map(\|(_, c)\| c) }
	fn next(&mut self) -> Option<char> { self.iter.next().map(\|(_, c)\| c) }

	fn try_consume(&mut self, want: char) -> bool {
	fn expect(&mut self, want: char) -> Result<(), Error> {

	if !self.try_consume('(') {
	if let Err(e) = self.expect('(') {

Conversation

febyeji commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

apoelstra commented Apr 2, 2026

Uh oh!

febyeji commented Apr 2, 2026

Uh oh!

apoelstra commented Apr 2, 2026

Uh oh!

tcharding commented Apr 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

trevarj commented Apr 21, 2026

Uh oh!

apoelstra commented Apr 21, 2026

Uh oh!

tcharding commented Apr 21, 2026

Uh oh!

febyeji commented Apr 22, 2026

Uh oh!

Uh oh!

apoelstra commented Apr 22, 2026

Uh oh!

Uh oh!

febyeji commented Apr 24, 2026

Uh oh!

apoelstra commented Apr 24, 2026

Uh oh!

febyeji commented Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trevarj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

febyeji Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trevarj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

febyeji commented Apr 1, 2026 •

edited

Loading

tcharding commented Apr 21, 2026 •

edited

Loading

febyeji commented Apr 27, 2026 •

edited

Loading

febyeji Apr 28, 2026 •

edited

Loading