Replace `OrdF64` type in the compiler with a first-class `PositiveF64` type by apoelstra · Pull Request #988 · rust-bitcoin/rust-miniscript

apoelstra · 2026-06-15T13:33:48Z

Supercedes #972.

The OrdF64 type in the compiler is weaker than it needs to be. Nominally it exists just to force an ordering on f64, with a code comment that it "will panic if given NaN". But it doesn't actually enforce that you can't put NaN into it, or any other value. There are also lots of places where we do reckless division that could plausibly turn into 0.0 / 0.0, and where we add values as u32s (which might overflow and panic) before converting to f64 or OrdF64. Plus there are 'as f64` casts all over the place, which is code smell.

In fact, all the probabilities used by the compiler, and parsed from policies, are required to be positive numbers (the policy values are even integers). So we can greatly improve this situation by renaming OrdF64 to PostiveF64, moving it to its own module, and enforcing that the contained value is always positive. This lets us:

freely add these values without worrying about cancellation
freely multiply and divide these values without worrying about zeroing them out or dividing by zero (although Inf is a permissible value for PositiveF64)
eliminate tons of casts

However, to do this we need to significantly refactor the compiler logic, which currently uses Option<f64> to represent "branch probabilities" and does so in an incoherent way, where None represents "not yet set or unneeded" and Some(0.0) roughly represents "required to be set but unneeded" and there are .expects all over the place. The compiler also stores these branch probabilities in the AstElemExt structure, which is totally unnecessary and results in a ton of mutation and arc-cloning.

Abeeujah

This is well executed, I identified a few nits which are optional or could be done in a follow up.

Abeeujah · 2026-06-15T20:00:31Z

+    /// Attempts to create a [`PositiveF64`] from an ordinary `f64`.
+    pub fn new(f: f64) -> Option<Self> {
+        // Can likely make this const in Rust 1.83
+        if f > 0.0 {


nit: Pending the MSRV upgrade for this to be made const, this can be rewritten to use the then_some method available on bool.

(f > 0.0).then_some(Self(f))

It would be better to use then rather than then_some, I suppose, because strictly speaking what you wrote results in a temporarily-existing invalid Self in the case that f <= 0. But good idea.

I believe, then_some returns Some(T) if the bool is true , or None otherwise; which looks very much like what the function is doing.

Clippy prefers then_some to then which I don't like, but okay, I'll use then_some..

😅 then is almost equivalent to just wriiting the if else block, whereas then_some is more intuitive for this usecase and fits everything nicely in one line.

Abeeujah · 2026-06-15T20:05:43Z

+    /// Returns the sum (or original value). Does not modify in-place.
+    #[must_use]
+    pub fn conditional_add(self, other: Option<Self>) -> Self {
+        if let Some(other) = other {


nit: This can also be written to make use of the map_or method available on Option

other.map_or(self, |i| i + self)

Abeeujah · 2026-06-15T20:13:53Z

+    {
+        // Compute the sum of all the items in the iterator. Because all items in
+        // the iterator are positive, this will be 0 iff the iterator is empty.
+        let sum = iter.clone().map(|x| x.0).sum::<f64>();


nit: iff -> if

No, iff is correct.

Yeah true, today's years old when I learned that.

Abeeujah · 2026-06-15T20:20:41Z

+    /// Returns `None` if the return value would be 0, which is impermissible for the
+    /// [`PositiveF64`] type.
+    pub fn one_minus_k_over_n<const MAX: usize, T>(t: &Threshold<T, MAX>) -> Option<Self> {
+        if t.is_and() {


nit: This could also use the then_some method, if adopted, the tradeoffs here would be conciseness over clarity

(!t.is_and()).then_some(Self(1.0 - (t.k() as f64 / t.n() as f64)))

Hah, I think this is too noisy.

Yeah, I thought as much

Abeeujah · 2026-06-15T21:03:07Z

+    let rw = PositiveF64::from(subs[1].0) / total;
+
+    //and-or
+    if let (Concrete::And(x), _) = (subs[0].1.as_ref(), subs[1].1.as_ref()) {


I think this pattern here, could be simplified to just take the reference being used in this if block, and the one unused should not be evaluated at all, same for Line 158 below

Sometimes we want to execute both sides of this. So there is no way to combine the two clauses into one.

Abeeujah · 2026-06-15T21:11:12Z

+            }
+        }
+    };
+    if let (_, Concrete::And(x)) = (&subs[0].1.as_ref(), subs[1].1.as_ref()) {


Same as stated above, the 0th value should not be evaluated since it is unused.

Abeeujah · 2026-06-15T21:13:01Z

+    }
+
+    Ok(())
+}


This function could benefit from Macros, it would greatly reduce the repetition.

The whole compiler needs to be rewritten to be non-recursive. So I don't want to make the code harder to modify ahead of doing that.

Great planning ahead, hopefully we get to eliminate a couple of the double nested loops too in the planned rewrite.

Abeeujah

.

Abeeujah

ACK 424f23b

yancyribbens · 2026-06-16T13:23:55Z

@Abeeujah can you stand on your head?

Abeeujah · 2026-06-16T13:26:51Z

@Abeeujah can you stand on your head?

I try, I try @yancyribbens

yancyribbens · 2026-06-16T13:55:41Z

@Abeeujah

How many digits of pi can you calculate?

Abeeujah · 2026-06-17T21:56:09Z

@Abeeujah

How many digits of pi can you calculate?

I usually just end up with a massive headache after 3.14 🤣

yancyribbens · 2026-06-17T22:55:05Z

I usually just end up with a massive headache after 3.14 🤣

Ha! ok, you pass the Turing test :P

apoelstra · 2026-06-18T15:40:04Z

@Abeeujah is not a LLM and has been contributing usefully to rust-bitcoin for quite some time.

yancyribbens · 2026-06-18T23:37:11Z

@Abeeujah is not a LLM and has been contributing usefully to rust-bitcoin for quite some time.

Yes, that is what I had meant by "passes Turing test", although maybe that was confusing.

My apologies @Abeeujah, we have an ongoing discussion in IRC to try to identify bot activity, and we thought your coding patterns might have been that of a bot/LLM.

Update the type name to better reflect its domain invariants. The underlying floats are guaranteed to be both positive and not-NaN, making `PositiveF64` a more accurate and descriptive name than `OrdF64`, which only highlighted the `Ord` trait implementation.

Code move only. Retain the public constructor. We will eliminate its use over the following commits. Also, publicly expose the type in src/lib.rs, since we are going to start enforcing invariants on it and making it generally useful.

Our parser only allows nonzero u32s, so just use the type. This adds a bit of noise to the current code, but when we move the probability computations from f64 to PositiveF64, it will pay dividends. Also eliminates a couple panic paths where we were using .sum or bare addition to add up probabilities; now we always convert to f64 before doing the addition, which will lose precision but not panic if the numbers get too big.

…values Once we remove the unchecked constructor(s) from PositiveF64, we won't be able to do these "sum everything then divide by sum" constructions. Instead, encapsulate them into PositiveF64::normalized_iter. This is mildly annoying to use, but I wasn't able to come up with a nicer solution that avoided gratuitous clones, gave sane compiler errors if you misuse them, and had readable code.

apoelstra · 2026-06-20T15:49:24Z

Rebased on master.

apoelstra · 2026-06-20T19:46:11Z

Addressed @Abeeujah's comments except the "should use a macro" one which I want to play with a bit.

Abeeujah · 2026-06-20T20:21:58Z

Addressed @Abeeujah's comments except the "should use a macro" one which I want to play with a bit.

You'd most likely have a lot of fun doing this, all the loop bodies are the same, only the provided arguments they evaluate differs.

Look ma, no type conversions!

By changing this one function and chasing compiler errors, I found I was able to (and required to) replace a *ton* of f64s with PositiveF64s. Amazingly, this did not require adding any new functionality to the PositiveF64 type, or any conversions. Instead, I was able to *delete* a ton of conversions, wrappers, unwrapping, etc. Now concrete.rs has literally no instances of 'f64' which is awesome. The next step is compiler.rs.

There is a ton of over-abstraction in AstElemeExt. Our original goal was to save on code by having one function, AstElemExt::type_check_common, which would match on a Terminal and do all the "fragment-specific" logic. But the consequence is that this needs to be really general to handle all the data that each fragment might need. In particular, disjunctions need "weights" which are provided by the user to indicate which branches are likely to be taken. No other kind of astelem needs extra data, but all the wrappers need infrastructure to carry this stuff around. By eliminating the terminal() function in place of direct constructors for each terminal type, we simplify the code and are able to also get rid of the type_check function. The next commits will eventually get rid of type_check_common, the branch_prob field of AstElemExt, a bunch of mutability and unwrap paths, and possibly more.

… functions Conjunctions don't need to set 'branch_prob', but we do set them, leading to confusing logic. By getting rid of these, we can make the conjunction logic simpler. Relatedly, we add an AstElemExt::and_n function rather than trying to force AstElem::and_or to produce an and_n. They're fundamentally different even though they compile to the same sort of script; and_n is a conjunction, can not be dissatisfied, and has no weights associated with it. By separating the two concepts, we eliminate a place where we were setting probabilities to 0.0, which is incorrect.

Had to move the logic into a separate function to avoid stack overflows, because we are recursing very large/complex functions. Other than that I tried to preserve the existing logic (and ran some local fuzztests quite extensively to verify). I removed all the unused code from AstElemExt::type_check, since it would no longer compile -- I changed the disjunction code to take the left/right weights as parameters rather than storing them in the AstElemExt object. The next commit will cause even more code to become dead; where possible, I have tried to prune dead code in separate commits to make the diffs easier to read. The goal of these commits is to remove the `branch_prob` field entirely, which will then let us make AstElemExt immutable, which will unlock some further refactorings. But until I'm done this, I have to just shuffle around the existing logic and the result is pretty big/ugly.

Update the CompExtData::and_or function to take weights, as for the other disjunctions, eliminating the last place where we read the branch_prob field. This totally eliminates the type_check method, and stops using the branch_prob field of AstElemExt. The next commit will delete a ton of unused stuff, which I didn't do here to minimize the amount of noise in this commit. Then I will start converting to use PositiveF64 rather than f64 everywhere.

Costs remain using f64, since they may be zero (e.g. the satisfactio cost of a timelock). But all probilities in the compiler should be positive: if they are zero, the corresponding path is impossible and we represent that with an option.

It is now impossible to create an invalid PositiveF64. This eliminates any possibility of division by 0 in the compiler.

apoelstra · 2026-06-20T21:46:52Z

Okay, used closures in place of macros and significantly reduced the code repetition. Also added the missing ops impls for PositiveF64, renamed and_or_0 to and_n which is the name used throughout the codebase, and cleaned up the history a bit.

I think this is ready to go now.

apoelstra · 2026-06-21T13:45:34Z

On 347c334 successfully ran local tests

Abeeujah

ACK 347c334 Successfully ran local tests

apoelstra mentioned this pull request Jun 15, 2026

Model OrdF64 as a non-NaN float #972

Closed

apoelstra force-pushed the 2026-06/ord-f64 branch 2 times, most recently from dae7ba4 to 424f23b Compare June 15, 2026 14:37

Abeeujah reviewed Jun 15, 2026

View reviewed changes

Abeeujah reviewed Jun 16, 2026

View reviewed changes

Abeeujah approved these changes Jun 16, 2026

View reviewed changes

Abeeujah and others added 4 commits June 20, 2026 15:45

move PositiveF64 to its own module

56d820a

Code move only. Retain the public constructor. We will eliminate its use over the following commits. Also, publicly expose the type in src/lib.rs, since we are going to start enforcing invariants on it and making it generally useful.

apoelstra force-pushed the 2026-06/ord-f64 branch from 424f23b to e6372d1 Compare June 20, 2026 15:49

apoelstra force-pushed the 2026-06/ord-f64 branch from e6372d1 to 5ea0546 Compare June 20, 2026 19:45

apoelstra added 8 commits June 20, 2026 21:45

concrete: use PositiveF64 directly in TapleafProbabilityIter

8fe2d4a

Look ma, no type conversions!

compiler: delete a ton of unused code

0c78c9b

compiler: use PositiveF64 for all probabilites

b6946af

Costs remain using f64, since they may be zero (e.g. the satisfactio cost of a timelock). But all probilities in the compiler should be positive: if they are zero, the corresponding path is impossible and we represent that with an option.

Remove the default PositiveF64 constructor

347c334

It is now impossible to create an invalid PositiveF64. This eliminates any possibility of division by 0 in the compiler.

apoelstra force-pushed the 2026-06/ord-f64 branch from 5ea0546 to 347c334 Compare June 20, 2026 21:45

Abeeujah approved these changes Jun 21, 2026

View reviewed changes

apoelstra mentioned this pull request Jun 21, 2026

miniscript: replace ExtParams, Ctx::check_* methods, and more, with ValidationParams #991

Open

+                  }
+                  Ok(())
+              }

Conversation

apoelstra commented Jun 15, 2026

Uh oh!

Abeeujah 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

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!

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!

Abeeujah left a comment • 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!

Abeeujah left a comment

Choose a reason for hiding this comment

Uh oh!

yancyribbens commented Jun 16, 2026

Uh oh!

Abeeujah commented Jun 16, 2026

Uh oh!

yancyribbens commented Jun 16, 2026

Uh oh!

Abeeujah commented Jun 17, 2026

Uh oh!

yancyribbens commented Jun 17, 2026

Uh oh!

apoelstra commented Jun 18, 2026

Uh oh!

yancyribbens commented Jun 18, 2026

Uh oh!

apoelstra commented Jun 20, 2026

Uh oh!

apoelstra commented Jun 20, 2026

Uh oh!

Abeeujah commented Jun 20, 2026

Uh oh!

apoelstra commented Jun 20, 2026

Uh oh!

apoelstra commented Jun 21, 2026

Uh oh!

Abeeujah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Abeeujah left a comment •

edited

Loading