Implemented segwit address types. Moved PrivKey into its own module.

[tamasblummer]

PrivKey was shadowing SecretKey only to implement a WIF, removed.
Implemented pay-to-pk address
unified parameter order as: key, compressed, network

[apoelstra]

PrivKey isn't really "shadowing SecretKey". A private key represents the secret data associated with a Bitcoin address, which includes a secp secret key, a network ID a compression flag. You can't do anything Bitcoin-related without all three of those.

[tamasblummer]

@apoelstra I viewed network dependant serialization as purely UI concept and assumed that compressed/uncompressed is deprecated, hence I saw no reason for PrivKey. I go with your choice however and restored PrivKey. Changed however some signatures to unify their argument order and to support legacy address (so it has more reasons to exist).

[apoelstra]

It's not "network dependent serialization", private keys on one network are really genuinely distinct from private keys on other networks and if it's possible to confuse them then I expect Bad Things will happen. If you want just an ECDSA secret key, the secp SecretKey already has you covered.

I could maybe be convinced that compressed keys are "deprecated". But this is a consensus system and for people who still have coins backed by uncompressed keys, their only way to upgrade is to move them, which requires using those keys. And support for uncompressed keys is not exactly a lot of code, it's just a couple bool args.

I would support moving the uncompressed key support into separate _uncompressed functions, rather than a boolean argument, if you just want to use the API without being bothered by the distinction.

[tamasblummer]

As for the supported networks (Testnet and Bitcoin), the only difference of private keys is what was introduced through their serialization, certainly to avoid that people mix them up and bad things happen. PrivKey makes sense if one assumes additional networks in future or that e.g. testnet uses a signature algo not yet available on mainnet, I go with that. Similarly understand that e.g. Satoshi has coins on uncompressed keys, but wonder if he needs rust-bitcoin's help. This library does not have to promise the backward compatibility and consensus of Bitcoin Core (the second it can't), but could be a tool to create new software. It is your baby, therefore I respect your preference what it should aim for. As for the compressed flag, I would prefer having distinct API to emphasize that they are there is only for legacy reasons and also because bool flags are ugly. Should I rewrite the API like that or just drop this PR?

[apoelstra]

Sure, I'd be happy with a distinct API for uncompressed keys.

And testnet/mainnet have diverged in the past, consider how long Segwit signatures were valid on testnet but not on mainnet.

[tamasblummer]

@apoelstra added support also for segwit adresses

[apoelstra]

A couple comments:

1. How do you expect the WitnessUse enum to be used? The Address type currently does not encode the redeem script for p2sh addresses, which is necessary and sufficient to distinguish between Segwit and non-Segwit p2sh addresses, so this flag represents an unobservable property.
2. Ditto for p2pk vs p2pkh addresses for that matter. I have no idea why Core does this mapping, if you take the address from your unit test and run validateaddress on it, the resulting scriptpubkey is wrong, which is surprising and not something I want to replicate in rust-bitcoin unless there's a good reason.
3. Can you squash the "remove PrivKey" and "re-add PrivKey" commits together? You can do this easily with git rebase -i master wif.
4. Similarly can you squash the "replace unit test" commit into that one?

[tamasblummer]

1. WittnessUse helps to differentiate if a script should be legacy P2SH or e.g. P2WSH where the same script is wrapped into a V0 witness program.
2. p2pk is the very old pay to public key and p2pkh is the most popular (non-segwit) address type nowdays. You must mean something else. The unit tests I added use keys extracted from transactions accepted on the main Bitcoin blockchain and you see the same addresses if looking at them with ablock explorer. If they do not validate, then the problem is in the validation code. I can not reproduce what you mean, please be more specific.
   3, 4. squashed commits.

[apoelstra]

What does "help to differentiate" mean? The Address type does not have enough information to redeem coins sent to a given address. If you add the necessary information, you will know the spend conditions and therefore whether a segwit transaction is required for redemption.

Run bitcoin-cli validateaddress 1HLoD9E4SDFFPDiYfNYnkBLQ85Y51J3Zb1 which is the address in your unit test. It will give you a scriptpubkey which does not match the scriptpubkey in the transaction that you extracted the address from. This behaviour is incoherent and if you tried to send coins to a p2pk user using that address they would be unable to receive them.

[tamasblummer]

Run bitcoin-cli getrawtransaction 9b0fc92260312ce44e74ef369f5c66bbb85848f2eddd5a7a1cde251e54ccfdd5 1 instead to see that the correct public key in the transaction is what is in the unit test. The correct scriptpubkey is 047211a824f55b505228e4c3d5194c1fcfaa15a456abdf37f9b9d97a4040afc073dee6c89064984f03385237d92167c13e236446b417ab79a0fcae412ae3316b77 OP_CHECKSIG instead bitcoin-cli validateaddress 1HLoD9E4SDFFPDiYfNYnkBLQ85Y51J3Zb1 assumes that the address is a p2pkh which it is not.

The reason to differentiate addresses (also with WithnessUse) that one is able to construct the correct redeem script to access the coins.

[apoelstra]

Why doesn't Core differentiate addresses like this to avoid the incorrect validateaddress behaviour?

Can you provide example code for rust-bitcoin constructing a redeem script from an Address?

[tamasblummer]

that redeems script must have placeholders for signatures. Do you have a preference how this should be represented?

[tamasblummer]

the validateadress output is correct in the sense that it only uses the address as input and its output is a possible script for that address, which is however not what Satoshi used. The problem is that conversion to human readable address string is not reversible in some cases as the serialization drops this information.

[tamasblummer]

Actually it is a nice and necessary project (within rust-bitcoin) on its own to create a framework to compile and sign redeem scripts given an address and private keys. If you allow I would do that in an other PR. But I am sure WittnessUse will be needed for that.

[apoelstra]

The problem is that conversion to human readable address string is not reversible in some cases as the serialization drops this information.

Thank you, this is what I trying to get at. You cannot represent this data in the Address type because the data cannot be determined from an address. Trying to do so will result in coins going to the wrong destination, like if you took one of Satoshi's addresses and tried to send to him using Core.

Actually it is a nice and necessary project (within rust-bitcoin) on its own to create a framework to compile and sign redeem scripts given an address and private keys. If you allow I would do that in an other PR.

Sure, I'd appreciate that.

But I am sure WittnessUse will be needed for that.

I highly doubt that.

[tamasblummer]

No problem, I remove it and re-introduce if you are wrong.

[tamasblummer]

removed WittnessUse and squashed everything into 1 commit.

[apoelstra]

Thanks for your patience. Looks good now.