DerivedDescriptor unfinished business #448
Conversation
Earlier we introduced DerivedDescriptorKey (an descriptor whose wildcards are known to have been replaced with derivation indexes) to eliminate wildcard derivation errors. This commit attempts to realise the full benefits of this idea. I tried to keep backwards compatibility. The naming of things no longer follows the logic of code so I'm going to fix that in a follow up.
The words "derive", "derived" and "derivable" were being used to describe things that aren't stricly derivation. Derivation is the process of taking cryptographic data and deterministically producing something else given some inputs. This is how the term is used in bip32, rust-bitcoin and more generally. In rust-miniscript the term is also used to mean "replacing a wildcard in a derivation path with a particular index". This can be quite confusing. For example, `is_derivable` returns true when there is a wildcard in the descriptor -- but it should return the opposite (given the usual meaning)! You can't do a derivation because you don't know the full derivation path yet (it's s got a wildcard in it). This commit replaces all instances where "derive" is used in this way.
LLFourn
force-pushed
the
derived_descriptor_unfinished_business
branch
from
875dbc1 to
2ad6555
Compare
|
This looks great! The renaming really helps make sense of these APIs. Is "definite" a standard term or did you make it up for this PR? TBH I'm not sure what the right way to say "non-wildcard" is. |
Ha! I got confused as hell over the 'derive' usage and also came to this same conclusion, so I did a definition search just now on wildcard to try come up with some terms, could we use one of these?
(We have concrete already in policy so probably not good.) |
| #[deprecated(note = "use at_derivation_index instead")] | ||
| /// Deprecated name of [`at_derivation_index`]. |
There was a problem hiding this comment.
I quite like this idea of removing the original comment and replacing with "Deprecated name of ..", I'm going to use that myself in future.
nit: Can you put the attribute below the comment please if you rebase for any other reason.
| @@ -441,40 +428,50 @@ impl DescriptorPublicKey { | |||
| } | |||
| } | |||
|
|
|||
| /// Whether or not the key has a wildcards | |||
| /// Whether or not the key has a wildcard | |||
| #[deprecated(note = "use has_wildcard instead")] | |||
There was a problem hiding this comment.
@apoelstra / @sanket1729 I wonder if we can come up with a way to remind ourselves to add the 'since' field to this (and to these attributes in general) when we do the release tracking issue that bumps the version. these 'deprecated' attributes are impossible to write correctly because the next release number is not known at time of writing.
There was a problem hiding this comment.
maybe we could file an issue with clippy to lint this :) "deprecated without since" default warn
There was a problem hiding this comment.
But we only want it right before doing release not all the time.
Not a standard term in anything surrounding Bitcoin. I borrowed it from linguistics which I think would describe a language feature like To keep to this line of thinking
"concrete" refers to a descriptor that has already derived keys in comments and docs already too. |
|
I can live with any of these (including Sorry to stir up a bikeshedding discussion, especially when I have no strong preference, but I think it could be worthwhile. |
|
I think you are right @LLFourn nothing else seems better than 'definite', FWIW +1 from me. |
This PR further develops the idea introduced in #345. It has two commits both with relatively detailed commit messages that can be reviewed separately. In summary:
DerivedDescriptorKey(a key that is guaranteed not to have wildcard in it) idea more by adding missing functionality and refining the API. In addition, I removed the error cases fromConversionErrorwhich seems to have been omitted from Define a new type for derivedDescriptorPublicKeys #345.DerivedDescriptorKey, the overlapping usage of the term "derive" has become quite confusing. In addition to the usual definition of "derive" we have also used it to mean "replace a wildcard with a particular derivation index". I deprecated and renamed everything that uses the latter definition.