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

        // 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

…use it in SortedMulti as a preview 50bf40485654f9a32dc310e94b9e2c63a30e27c5 policy: use new Threshold type in semantic policy (Andrew Poelstra) 5964ec3de4d04934370c52cc18b2c5824fcf5f2e sortedmulti: use new Threshold type internally (Andrew Poelstra) 36ea5cc000866babcc9037c767926de881032fb5 expression: add method to validate and construct a threshold (Andrew Poelstra) 452615b5dd1d5e5fc2b3fc0b27b771296c274d96 rename expression.rs to expression/mod.rs (Andrew Poelstra) dbf7b32d32ee2e84d5ce7e8337920b12cf46e622 primitives: introduce Threshold type (Andrew Poelstra) 30a3b595f7dc6b16cbdbee549040d80a078f7f8d psbt: fix "redundant target" doc warning (Andrew Poelstra) Pull request description: 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. ACKs for top commit: tcharding: ACK 50bf40485654f9a32dc310e94b9e2c63a30e27c5 sanket1729: ACK 50bf40485654f9a32dc310e94b9e2c63a30e27c5. Tree-SHA512: 084034f43a66cb6e73446549aad1e1a01f7f0067e7ab3b401f8d819f7375f7a0f6b695c013e3917f550d07b579dcd8ca21adf6dd854bb82c824911e8d1ead152

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

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

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

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

Uh oh!

Conversation

apoelstra commented Mar 19, 2024

Uh oh!

apoelstra commented Mar 19, 2024

Uh oh!

tcharding 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!

apoelstra Mar 19, 2024 • 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!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

sanket1729 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

apoelstra Mar 19, 2024 •

edited

Loading