Address type system#668
Address type system#668dr-orlovsky wants to merge 3 commits intorust-bitcoin:masterfrom BP-WG:address/guarantees
Conversation
|
Did a big update completing this PR by providing guarantees for address-related type sanity |
There was a problem hiding this comment.
EDIT: updated after the following discussion from #648 and reading rest of the commits.
Left a few comments on the first commit. Did not review all of it
My personal opinion is that this PR has too much code and additional data structures for the things it tries to achieve. I think developers using this library should only care about the following from address API:
- Users should be able to send to future addresses.
- Users should receive on known supported addresses.
To that end, I think we can do the following:
enum AddressInfo {
// we should send and receive to these addresses
Supported(AddressType),
// We should support sending to these, but don't receive here
Future(version, len),
}This keeps the code diff small, users don't really need to know taprootv1 non-32 bytes, vs future version 2. If there is someone who is interested in implementing the differences of why something is valid vs non-valid, they can do so easily downstream. Also I don't see the point of SegwitInfo struct. There can be simple method to get Option<(verion, len)>
Curious what do other people feel about this?
|
@sanket1729 I agree that detailed analysis has limited usefulness but there is at least one use case: in privacy-oriented protocols/wallets (e.g. BIP78) one needs to at least compare equality of address types to get maximum privacy. I think the current proposal goes beyond that but maybe @dr-orlovsky has some other use case in mind? |
|
@sanket1729 agree on replacing |
|
@Kixunil my usecase is professional software (wallets, blockchain explorers - Bitcoin Pro, MyCitadel) that provides as much detailed information as possible. Also databases indexing blockchain data for research purposes. |
|
I share Sanket's concerns that this PR adds a lot of data structures with unclear usecases, which will need to be adjusted in breaking ways in the future. Wallets should be using rust-miniscript to keep track of their own addresses and related information. Block explorers etc. can use their own ad-hoc analysis of scriptPubKeys ... it is not at all obvious to me that they would want to categorize this information in the way proposed by this PR. And if they did, it is not obvious to me that this library is the place to add direct support. |
|
Yeah, I think having it in a separate crate with integrations would be better and closer to the idea of splitting up this crate. |
|
@sanket1729 @apoelstra @Kixunil I have simplified this PR by removing |
|
Sorry for bringing this up again. In my opinion, there should not be any At the very least, I think we should merge If you really want to analysis on script pubkeys, I would not be opposed to supporting it in |
sanket1729
left a comment
There was a problem hiding this comment.
I think we should check if the 32-byte in p2tr is actually a correct x-only key in p2tr.
|
|
||
| /// Returns whether support of the address will be possible only in a future through a softfork | ||
| #[inline] | ||
| pub fn requires_softfork(self) -> bool { |
There was a problem hiding this comment.
I don't know if this is useful but sure
tcharding
left a comment
There was a problem hiding this comment.
I'm not convinced this AddressInfo struct is helping folks out. I see the benefit of it for internals, having all the edge cases of the various variants there in code but for users coming to the library they are going to go Address -> AddressInfo -> WTF?
We already have a bunch of methods on Address that give useful hints about the address. Why not make AddressInfo private and just add methods as they are required?
| pub fn from_address_info(address_info: AddressInfo) -> Option<AddressType> { | ||
| address_info.address_type() | ||
| } |
There was a problem hiding this comment.
Do we really need this method? It seem that a call such as let t = address_info.address_type() is just as erganomic, if not more so, than let t = AddressType::from_address_info(address_info). Or am I missing some other way of using this?
| /// NB: This does include future addresses which can be spent by anybody without a proper | ||
| /// signature before some future soft fork. |
There was a problem hiding this comment.
Is this comment correct? Seems that AddressInfo::Future returns false would imply that this does _not` include future addresses?
| /// by satisfying certain (address-specific) public key and/or script requirements and this may | ||
| /// not be changed with any future soft-fork. |
There was a problem hiding this comment.
Is this comment correct? Future returns true but comment says may not be changed with an future soft-fork?
| } | ||
|
|
||
|
|
||
| /// Detects if the address can be used for security receiving funds, i.e. it can be spent only |
There was a problem hiding this comment.
| /// Detects if the address can be used for security receiving funds, i.e. it can be spent only | |
| /// Detects if the address can be used for securely receiving funds, i.e. it can be spent only |
| /// this will be a non-Taproot witness v1 output, spendable by anyone under the current | ||
| /// consensus rules before one of future softfork would change that. |
There was a problem hiding this comment.
This sentence is worded a bit funnily, one possible fix is.
| /// this will be a non-Taproot witness v1 output, spendable by anyone under the current | |
| /// consensus rules before one of future softfork would change that. | |
| /// this will be a non-Taproot witness v1 output, spendable by anyone under the current | |
| /// consensus rules (until some future softfork changes that). |
| Future(WitnessVersion, /** witness program length */ usize), | ||
| /// Witness version is 0 and is known to consensus, but witness program has non-standard length | ||
| /// which is not supported by consensus rules and can't be spent. | ||
| NotSpendableWitnessV0(/** witness program length */ usize), |
There was a problem hiding this comment.
As others have mentioned, I find this variant very confusing. There are lots of ways to construct provably unspendable outputs, and this one is particularly obscure. It's also not clear that you could actually produce a usable address for something like this, even if you bech32-encoded it, because parsers tend to have length checks.
Similarly NonTaprootWitnessV1 should be merged into Future.
|
Even with the reduced scope I agree with others that this PR has an unclear motivation and purpose. My original comment, that users who want to classify addresses should do so themselves and/or use descriptors (and that if they do, it seems pretty unlikely to me that they would want this particular ad-hoc classification scheme), still stands. |
|
Ok, closing it |
7634642 Use struct update syntax (Tobin C. Harding) 2bc083e Use iterator instead of manual loop (Tobin C. Harding) 6d663e1 Allow range loop in test code (Tobin C. Harding) a8c156e Use map instead of and_then (Tobin C. Harding) d5662da Remove redundant closure (Tobin C. Harding) 90f1585 Remove redundant clone (Tobin C. Harding) 409d4b0 Remove explicit reference (Tobin C. Harding) 9ec6b17 Remove redundant field names in struct initialization (Tobin C. Harding) Pull request description: After doing rust-bitcoin#668 I wanted to be able to use the `just sane` command before pushing PRs, so fix the lint issues. ACKs for top commit: apoelstra: ACK 7634642 thanks! we should re-enabled Clippy in CI in this repo...I believe we disabled it due to a clippy bug in July Tree-SHA512: 1e953f2fc1a6e649324293a84abf1be54feca6095111653af046aacf17a83e6ae15452fe8825599ccbfae0ea84fe7d1988c8b72d5dd902c9d0fb1d34e660ef8b
Major refactoring of witness program processing in address and scriptPubkeys. Makes address-related types to guarantee correctness of the witness programs.
Closes #648