Complete Resource limits #168
Conversation
sanket1729
force-pushed
the
resource_limits_new
branch
3 times, most recently
from
df9eba0 to
73a2a4c
Compare
sanket1729
force-pushed
the
resource_limits_new
branch
3 times, most recently
from
bdeb626 to
add9523
Compare
src/miniscript/analyzable.rs
Outdated
| #[derive(Debug)] | ||
| pub enum AnalysisError { | ||
| /// Top level is not safe. | ||
| TopLevelNonSafe, |
There was a problem hiding this comment.
BranchWithNoSignature is ok actually, it's not crazy long
src/miniscript/analyzable.rs
Outdated
| /// Use this function to check whether the guarantees of library hold. | ||
| /// Most functions of the library like would still | ||
| /// work, but results cannot be relied upon | ||
| pub fn check_safety(&self) -> Result<(), AnalysisError> { |
There was a problem hiding this comment.
Maybe sanity_check
And then we could have parse_insane and from_str_insane :P
src/miniscript/analyzable.rs
Outdated
| /// Repeated Pubkeys | ||
| RepeatedPubkeys, | ||
| /// Miniscript contains atleast one path that exceeds resource limits | ||
| ResourceLimitsExceeded, |
There was a problem hiding this comment.
also should rename to indicate that it's maybe only one branch
There was a problem hiding this comment.
BranchExceedsResourceLimits lol this is getting a bit long..
sanket1729
force-pushed
the
resource_limits_new
branch
from
add9523 to
a4d71a5
Compare
sanket1729
force-pushed
the
resource_limits_new
branch
from
a4d71a5 to
45a2ae0
Compare
src/descriptor/mod.rs
Outdated
| // extra allocation using clone allows us to reuse | ||
| // check safety function from Miniscript fragment instead | ||
| // of implemneting more code for safety checks | ||
| svm.clone().multi_ms().sanity_check()? |
There was a problem hiding this comment.
I think you should pull this clone-and-sanity-check logic into its own method on SortedMultiVec. Even though it's internal I think it's confusing to have a method multi_ms which does not sort the keys (and which therefore does not match the actual conversion of the SVM to a miniscript)
There was a problem hiding this comment.
I tried doing this. The odd part about that I don't have access to Ctx to create a new miniscript which I do not have inside the SortedMultiVec.
That is we don't know whether we have to create a new Miniscript<_, Legacy> or Segwitv0
There was a problem hiding this comment.
You can parameterize the sanity-check function over Ctx, which will be a bit awkward to call but should work
src/miniscript/analyzable.rs
Outdated
| .map(|pk_pkh| match pk_pkh { | ||
| PkPkh::PlainPubkey(pk) => pk.to_pubkeyhash(), | ||
| PkPkh::HashedPubkey(h) => h, | ||
| }) |
There was a problem hiding this comment.
you can drop the .map here entirely
There was a problem hiding this comment.
And substitute it with?
|
utack, except nits, and also there is an IRC discussion about naming for the validity constraints |
Updated Bare descriptor to use Bare Ctx Updated Bare descriptor to user Bare context for standardness checks instead of previously incorrect Legacy context Combine top level checks
Add Russel's doc about multiparty policy entailment to the repository.
Some of the compiler guanrantees are lost if we allow repeated keys. The compiler should error on these keys
sanket1729
force-pushed
the
resource_limits_new
branch
from
45a2ae0 to
4a8554b
Compare
Completes the resource limit detection of miniscript. Marking as draft, because we should merge this after #150 .
Fixes #134