Introduce Threshold type and use it in SortedMulti as a preview #660

apoelstra · 2024-03-19T19:48:21Z

I have a big branch which uses this type throughout the codebase. I split it up into pieces (separate commit for its use in each policy type, for its use in multi/multi_a, for its use in Terminal::thresh, etc) but it's still a pretty big diff.

Despite the size of the diff I think this is definitely worth doing -- in my current branch we have eliminated a ton of error paths, including a ton of badly-typed ones (grepping the codebase for errstr shows I've reduced it from 30 instances to 18).

This PR simply introduces the new Threshold type and uses it in SortedMultiVec to show how the API works. Later PRs will do the more invasive stuff.

We don't need to provide explicit targets for doclinks if the name matches the type being pointed to.

apoelstra · 2024-03-19T20:16:17Z

Will need to iterate a bit to get CI to pass. Rust's rules for redundant/unused imports are idiotic and incompatible with nostd.

Eliminates several redundant error checks, calls to errstr, and makes the logic more uniform. Also demonstrates the `Expression::to_null_threshold` and `Threshold::translate_by_index` methods, which may not have been clearly motivated in previous commits.

tcharding

Looks mad! Just commenting not acking because you force pushed while I was reviewing.

tcharding · 2024-03-19T20:02:06Z

src/primitives/threshold.rs

+//! Thresholds
+//!
+//! Miniscript


Suggested change

//! Thresholds

//!

//! Miniscript

//! A generic (k,n)-threshold type.

Literally the only thing that was better from #605 :)

Lol, oh, right :) I should finish this doccomment.

tcharding · 2024-03-19T20:05:02Z

src/primitives/threshold.rs

+#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
+pub struct Threshold<T, const MAX: usize = 0> {
+    k: usize,
+    inner: Vec<T>,


Slight preference for v (or something else) because this is not a wrapper type so inner seems slightly off.

tcharding · 2024-03-19T20:32:14Z

src/descriptor/sortedmulti.rs

+            inner: tree
+                .to_null_threshold()
+                .map_err(Error::ParseThreshold)?
+                .translate_by_index(|i| expression::terminal(&tree.args[i + 1], Pk::from_str))?,


I wasn't able to convince myself that the index addition is good. Would be good to have a simple test to check for potential off-by-one error.

I use this method all over the place in later PRs. It's definitely correct.

Actually, I'm confused by what you mean by "a simple test". This method is called literally every time that a SortedMulti is parsed (and whenever any other type is parsed, after later PRs which use this method in the same way for the other types). So the existing tests cover it.

The addition has a +1 because the original expression starts with a k value that we need to skip.

lolz, still some ways to go improving as a review huh. I just flicked through looking for a unit test, not looking for other call sites. Marking as resolved.

tcharding

ACK 50bf404

sanket1729

ACK 50bf404.

Sorry for the delay in reviewing this. For some reason, I thought this would be more complicated than it turned out to be.

sanket1729 · 2024-03-26T22:15:55Z

src/primitives/threshold.rs

+/// If the constant parameter `MAX` is nonzero, it represents a cap on the
+/// `n` value; if `n` exceeds `MAX` then an error is returned on construction.
+#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
+pub struct Threshold<T, const MAX: usize> {


Why was this MAX stuff necessary? Maybe it is useful, waiting for further commits :)

I am comparing the extra type safety vs binary bloat/compile time.

@sanket1729 it is used to distinguish between thresholds used for thresholds, and those used for multi and multi_a. Without the max in the type system it would be possible to construct thresholds with the wrong maximum which would make it much harder to enforce that it was actually correct.

I think the bloat shouldn't be too bad because there are only 3 values that are ever used (0, 20, and MAX_WEIGHT/50 or whatever the taproot max is) and because (I think) the complier is smart enough not to double-compile methods that don't even refer to the MAX parameter. Which is most of them.

sanket1729 · 2024-03-26T22:21:13Z

src/descriptor/sortedmulti.rs

        // Check the limits before creating a new SortedMultiVec
        // For example, under p2sh context the scriptlen can only be
        // upto 520 bytes.
-        let term: Terminal<Pk, Ctx> = Terminal::Multi(k, pks.clone());
+        let term: Terminal<Pk, Ctx> = Terminal::Multi(self.inner.k(), self.inner.data().to_owned());


Note that the constuctor_check now does an allocation and is expensive. Earlier we could return directly from here

Lemme check my followup PR and make sure that this allocation goes away. I'm pretty sure it does.

Yep, once Terminal::Multi also takes a Threshold this alllocation can go away.

…multi/multi_a 79d3f7e sortedmulti: eliminate allocation in constructor_check (Andrew Poelstra) f62322d miniscript: use new Threshold type for multi/multi_a (Andrew Poelstra) 1ced03b policy: use Threshold in concrete policy (Andrew Poelstra) Pull request description: Some more large but mostly-mechanical diffs. This is able to eliminate a bunch of error paths, though the actual error variants can't be removed until we also convert Terminal::thresh in the next PR(s). At that point we will start to see the real benefits of this type because fallible functions will become fallible and massive amounts of compiler error-checking can just go away entirely. Also removes the allocation in `SortedMulti::constructor_check` that was pointed out in #660. ACKs for top commit: sanket1729: ACK 79d3f7e tcharding: ACK 79d3f7e Tree-SHA512: 98828910c6fea91608b9f7dc15f68999ac4555e2e8eac28c04bd3e70f7c5e74dfeb7902805e606a3ad5a7ba36f77d4dc0c47f189b7ff591a8c4fd7abcccbefc7

tcharding mentioned this pull request Mar 19, 2024

Add a Threshold type #605

Closed

psbt: fix "redundant target" doc warning

30a3b59

We don't need to provide explicit targets for doclinks if the name matches the type being pointed to.

apoelstra force-pushed the 2024-03--threshold-1 branch from 5b40fc6 to 23ed735 Compare March 19, 2024 20:09

apoelstra force-pushed the 2024-03--threshold-1 branch from 23ed735 to 0fb3789 Compare March 19, 2024 20:19

apoelstra added 5 commits March 19, 2024 20:21

primitives: introduce Threshold type

dbf7b32

rename expression.rs to expression/mod.rs

452615b

expression: add method to validate and construct a threshold

36ea5cc

policy: use new Threshold type in semantic policy

50bf404

apoelstra force-pushed the 2024-03--threshold-1 branch from 0fb3789 to 50bf404 Compare March 19, 2024 20:28

tcharding reviewed Mar 19, 2024

View reviewed changes

tcharding approved these changes Mar 19, 2024

View reviewed changes

sanket1729 approved these changes Apr 2, 2024

View reviewed changes

apoelstra merged commit f83eb64 into rust-bitcoin:master Apr 2, 2024
16 checks passed

apoelstra deleted the 2024-03--threshold-1 branch April 2, 2024 14:06

apoelstra mentioned this pull request Apr 4, 2024

Use Threshold type in concrete policy and in Terminal::multi/multi_a #674

Merged

apoelstra mentioned this pull request May 20, 2024

Release tracking PR: v12.0.0 #688

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Threshold type and use it in SortedMulti as a preview #660

Introduce Threshold type and use it in SortedMulti as a preview #660

apoelstra commented Mar 19, 2024

apoelstra commented Mar 19, 2024

tcharding left a comment

tcharding Mar 19, 2024

apoelstra Mar 19, 2024

tcharding Mar 19, 2024

tcharding Mar 19, 2024

apoelstra Mar 19, 2024

apoelstra Mar 19, 2024 •

edited

Loading

tcharding Mar 19, 2024

tcharding left a comment

sanket1729 left a comment

sanket1729 Mar 26, 2024

sanket1729 Apr 2, 2024

apoelstra Apr 2, 2024

sanket1729 Mar 26, 2024

apoelstra Apr 2, 2024

apoelstra Apr 2, 2024

Introduce Threshold type and use it in SortedMulti as a preview #660

Introduce Threshold type and use it in SortedMulti as a preview #660

Conversation

apoelstra commented Mar 19, 2024

apoelstra commented Mar 19, 2024

tcharding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding left a comment

Choose a reason for hiding this comment

sanket1729 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra Mar 19, 2024 •

edited

Loading