WIP: Upgrade rust-miniscript / rust-bitcoin to latest version #1228
Conversation
| _ => None, | ||
| } | ||
| .ok_or(LianaPolicyError::IncompatibleDesc)?; | ||
| SemanticPolicy::Thresh(thresh) if thresh.is_or() && thresh.n() > 1 => { |
There was a problem hiding this comment.
shouldn't we check thresh.k() == 1?
| SemanticPolicy::Thresh(thresh) if thresh.is_or() && thresh.n() > 1 => { | |
| SemanticPolicy::Thresh(thresh) if thresh.is_or() && thresh.n() > 1 && threshk() == 1 => { |
There was a problem hiding this comment.
I think this is already checked by thresh.is_or().
There was a problem hiding this comment.
I think it makes sense to keep this thresh.n() > 1 condition here, but perhaps this is not strictly necessary as the previous version didn't have subs.len() > 1 and we already ensure further below that the primary path and recovery paths are not empty.
|
Yes. In general i don't think the code here is correct. I need to go through the descriptor core logic one more time. |
|
also not from these PR, but here if assert fail we will panic shouldn'nt we error instead? |
| @@ -185,11 +187,14 @@ impl PathInfo { | |||
| ) -> Result<PathInfo, LianaPolicyError> { | |||
| match policy { | |||
| SemanticPolicy::Key(key) => Ok(PathInfo::Single(key)), | |||
| SemanticPolicy::Threshold(k, subs) => { | |||
| let keys: Result<_, LianaPolicyError> = subs | |||
| SemanticPolicy::Thresh(thresh) if thresh.k() > 0 && thresh.n() >= thresh.k() => { | |||
There was a problem hiding this comment.
Is the if condition required here if these checks are already performed in the Threshold constructor?
| SemanticPolicy::Threshold(k, subs) => { | ||
| let keys: Result<_, LianaPolicyError> = subs | ||
| SemanticPolicy::Thresh(thresh) if thresh.k() > 0 && thresh.n() >= thresh.k() => { | ||
| // FIXME |
There was a problem hiding this comment.
Is this FIXME still required? The code seems OK 🤔
I need to go over the changes again, and this introduces a massive performance hit. From quick profiling i think it's from upstream changes in the miniscript policy compiler.
Running
cargo test descriptorsbefore:After:
Fixes #1224.