several improvements and bugfixes to uncompressed key handling and sortedmulti #849
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The lifetime stuff in particular has been blocking rust-bitcoin#805 every week for several months now.
This rule is weird and wrong in a number of ways. It was introduced in PR rust-bitcoin#97 c93bdef which was the massive PR that introduced the Ctx parameter. This particular rule wasn't commented on, but we can infer some things: * From the docs on the error and my vague memory, the problem was that we couldn't estimate satisfaction for a pubkeyhash fragment because we didn't know if the key was 33 or 65 bytes ahead of time. * Therefore, the code bans this in a legacy context, but not in bare contexts (in bare contexts there is a comment saying that bare fragments "can't contain miniscript because of standardness rules" so maybe we thought about this ... but that comment has never been true, so we thought wrong..). We should've banned the fragment in bare contexts as well. * Also, while the error is called MalleablePkH, none of this has anything to do with malleability! Later, in rust-bitcoin#431, we introduced the RawPkH fragment and put a pubkey into the PkH fragment. At that point, for PkH we always knew the pubkey and could just directly compute its size using the `is_uncompressed` accessor. The RawPkH fragment was a second-class citizen which didn't even have a string serialization. At this point we should have dropped the MalleablePkH rule. Still later, in rust-bitcoin#461, we introduced `ExtParams` and the ability to parse "insane" scripts in a more flexible way, including enabling the experimental `expr_raw_raw_pkh` fragment. We then gained an error `Analyzable::ContainsRawPkh` which conceptually overlaps with `ScriptContextError::MalleablePkH` but which isn't tied to any context, just whether the user has enabled this second-class feature.
In our existing code we assume that all "ECDSA keys" are compressed, i.e. 34 bytes long. This is not a valid assumption. We originally did this because under some circumstances we legitimately did not know the key and didn't want to penalize users by assuming uncompressed keys when this was unlikely the case. However, now the only way to have a pk or pkh where we don't know the key is if the user is using a raw pkh. And if they do this in Taproot or Segwit we still know that there are no uncompressed keys (though with the current Ctx trait we can't distinguish Segwit from pre-Segwit so the new logic assumes uncompressed keys even for Segwit.) This logic will be cleaned up and improved with ValidationParams; but currently it is wrong, and it is causing me trouble with later commits. So fix the situation here as best we can. The next commit will make a similar change to the multi() combinator (multi_a is fine because the keys and sigs are fixed size); the commit after that will introduce a unit test to SortedMulti which exercises this logic.
As in the previous commit, we were previously assuming that all pubkeys were compressed. This isn't so. We have the keys, we can query them for whether they are uncompressed or not.
Update the constructor to avoid calling Miniscript::from_ast, which can
return many kinds of errors, none of which are reachable. Instead call
Miniscript::multi and then do a validation check once this is
constructed.
There is a comment in the constructor explaining why the validation is
needed: it may be that an otherwise-valid checkmultisig script could
exceed the 520-byte limit for p2sh outputs.
HOWEVER, we have no test for this behavior, and when trying to test it,
I found several issues. One, that our accounting for uncompressed keys
is wrong, I fixed in the previous commits.
The others are:
1. In the existing unit test we try to create a sortedmulti with 21 keys,
violating the constructor for Threshold. This is a fine test...
2. ...except that we set k == 0 which makes the constructor fail no matter
what n is, so we're not actually testing "too many keys".
3. Furthermore, we have no test at all that tries to exceed the P2SH
limits, and when I added one I was able to exceed these limits
because of the type system bugs fixed in the last two commits.
We currently take a k value and a vector as input to the constructor to the SortedMultiVec type, and propagate this out to all the sortedmulti constructors. This means that we have to construct the threshold inside the method and then return errors outward. It's more flexible, though potentially a bit more annoying, to make the user construct the Threshold. Then they can deal with the error. This means that the descriptor constructors *only* return a validation error, which will let us tighten the Result type in a later commit.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Removes the weird and incorrect
MalleablePkHscript context rule; fixes several bugs related to key size estimation in pkh and multisig fragments; improves tests of sortedmulti; uses theThreshtype in sortedmulti constructors.These bugs are obscure and only visible when using uncompressed pubkeys and I don't think they're worth backporting.
This PR is starting to make "real" changes toward unifying our validation parameters. In particular we separate out validation errors in the
sortedmulticonstructors from threshold-construction errors (which the user is forced to deal with before calling asortedmulticonstructor). The PR is bigger than you might think because when testing this separation I found some bugs.This PR also includes a clippy fix which should unstick #805 (update nightly version) which has been stalled for months because the weekly retries don't trigger Github notifications and I didn't notice it.