Use Threshold type in concrete policy and in Terminal::multi/multi_a
#674
Conversation
src/descriptor/sortedmulti.rs
Outdated
| let ms = Miniscript::from_ast(term)?; | ||
| // This would check all the consensus rules for p2sh/p2wsh and | ||
| // even tapscript in future | ||
| Ctx::check_local_validity(&ms).map_err(From::from) | ||
| Ctx::check_local_validity(&ms).map_err(Error::from)?; |
There was a problem hiding this comment.
| Ctx::check_local_validity(&ms).map_err(Error::from)?; | |
| Ctx::check_local_validity(&ms)?; |
There was a problem hiding this comment.
Yeah, I could drop it here. In future though I want to move away from using from and explicitly write variants (e.g. map_err(Error::ScriptContext) or whatever).
But agreed that .map_err(Error::from)? is the worst of both worlds and I should drop it.
| keys.len() - 1, | ||
| keys.iter().map(|pubkey| Arc::new(Concrete::Key(*pubkey))), | ||
| ) | ||
| .unwrap(); |
There was a problem hiding this comment.
I notice there's a few unwraps() in this PR. Just curious if there's a reason to do this instead of returning an Error type.
There was a problem hiding this comment.
These are tests. unwraps are common in test and that is how we test most things.
There was a problem hiding this comment.
Oops, I had meant to place this comment on the generate_combination() changes. Heh yeah, I get using unwrap on the tests though.
There was a problem hiding this comment.
Yeah, that was one occurrence which was a safe unwrap because of miniscript invariants around Threshold. We could add a expect with a message with the reasoning, but there was a short comment the commit message explaining that.
There was a problem hiding this comment.
I added an expect. There was also a debug_assert which would have triggered before the unwrap did, but better to be explicit.
| keys.len() - 1, | ||
| keys.iter().map(|pubkey| Arc::new(Concrete::Key(*pubkey))), | ||
| ) | ||
| .unwrap(); |
There was a problem hiding this comment.
These are tests. unwraps are common in test and that is how we test most things.
This one is a pretty big diff but hopefully the unit tests provide some assurance. One maybe-subtle change is that the debug_assert in `generate_combinations` was incorrect -- it checked that k <= n, but the logic depended on k < n. I fixed the debug_assert and nothing broke.
apoelstra
force-pushed
the
2024-04--threshold-2
branch
from
1345fb2 to
79d3f7e
Compare
|
Rebased to fix the |
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_checkthat was pointed out in #660.