Hash new types as specified in #284

[dr-orlovsky]

Progress report:

• List of the new hash types was discussed and new types were defined: New hash data types (Txid etc) #284 (comment)
• bitcoin_hashes new_hashtype macro is fixed to work correctly and minimize amount of code changes required for adopting the new hash types: Fixing hash_newtype macro to work correctly bitcoin_hashes#63 (it depends on the other PR here: Adding docs to macros and fixeing rust lint warnings bitcoin_hashes#64)
• All new types are defined: e9c4d7d#diff-107236903cdf97a062c0be08467a2769
• Txid is adopted, the library is compiling and tests are running correctly: e9c4d7d
• All proposed hashes are implemented

[dr-orlovsky]

@apoelstra The PR is ready for a review; the build with rust 1.22 fails since rust compiler fails to recognise that FromHex use is actually used by the macro, and allow_unused does not work for this version (but works fine for the rest). I can't do anything with this false error...

[dpc]

Maybe make #![deny(unused_imports)] conditional on a feature , and turn that feature on travis builds of non-1.22.0 builds?

[apoelstra]

I think we should just put allow(unused_imports) on hash_types.rs

Alternately, we could remove the deny line entirely; these are just a random collection of rust-0.6-era style lints which are handled today by better error messages and better coding conventions. But I don't like the warnings, even if they're only on 1.22.

[dr-orlovsky]

Everything works for now with allow(unused_imports) moved on hash_types mod in lib.rs. Also merged recent changes from the master.

[codecov-io]

Codecov Report

Merging #349 into master will decrease coverage by 0.07%.
The diff coverage is 88.03%.

@@            Coverage Diff            @@
##           master    #349      +/-   ##
=========================================
- Coverage   84.78%   84.7%   -0.08%     
=========================================
  Files          39      40       +1     
  Lines        7221    7680     +459     
=========================================
+ Hits         6122    6505     +383     
- Misses       1099    1175      +76

Impacted Files	Coverage Δ
src/lib.rs	100% <ø> (ø)	⬆️
src/internal_macros.rs	59.47% <ø> (ø)	⬆️
src/network/message_filter.rs	100% <100%> (ø)	⬆️
src/util/contracthash.rs	78.5% <100%> (ø)	⬆️
src/util/address.rs	85.49% <100%> (-0.9%)	⬇️
src/network/message.rs	95.37% <100%> (+0.23%)	⬆️
src/util/bip143.rs	100% <100%> (ø)	⬆️
src/util/psbt/mod.rs	86.06% <100%> (ø)	⬆️
src/blockdata/block.rs	79.13% <100%> (+0.94%)	⬆️
src/util/bip32.rs	87.4% <100%> (+0.25%)	⬆️
... and 33 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e2caebc...63d0194. Read the comment docs.

[apoelstra]

Could you please clean up the history? Squash down fixup commits, remove merge commits, etc?

[dr-orlovsky]

I have tried to do my best in structuring my changes into logical structure of sequential commits plus the last two for rebasing master and resolving conflicts with the master

[apoelstra]

Can you rebase instead of merging?

[dr-orlovsky]

I can, but when I did it it said about conflicts (even after rebasing), so I had to merge. Now I see that there are new conflicts...

[apoelstra]

Right, if there are conflicts you need to fix the conflicts

[elichai]

If you prefer I don't mind helping with rebasing/fixing conflicts.
(all you'll need to do is tick the allow edits from maintainers)

[dr-orlovsky]

Sorry for replying late, being out for our first client-side validation event we were doing in Milan. @elichai, thank you for your proposal, I will address @stevenroose review today with a new commit and will allow edits for maintainers - I am completelly missed with all this rebase vs merge + conflicts...

[dr-orlovsky]

@apoelstra the command succeeds with all of the commits; so there is no change

[apoelstra]

:( on the first commit, 4b1fb9970f6ce32977945440db8668e60f0ecff7, I get

error: unused imports: `FromHex`, `ToHex`
  --> src/hash_types.rs:22:52
   |
22 | use hashes::{sha256, sha256d, hash160, Hash, hex::{ToHex, FromHex}};
   |                                                    ^^^^^  ^^^^^^^
   |
note: lint level defined here
  --> src/lib.rs:44:9
   |
44 | #![deny(unused_imports)]
   |         ^^^^^^^^^^^^^^

Then on 985bfb70c58d9b82662155b3db0711ce31fbf402 I get many errors like

error[E0599]: no function or associated item named `from_hex` found for type `hash_types::XpubIdentifier` in the current scope
  --> src/hash_types.rs:48:1
   |
48 | hash_newtype!(XpubIdentifier, hash160::Hash, 20, doc="XpubIdentifier as defined in BIP-32.");
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   | |
   | function or associated item `from_hex` not found for this
   | function or associated item not found in `hash_types::XpubIdentifier`
   |
   = help: items from traits can only be used if the trait is in scope
   = note: the following trait is implemented but not in scope, perhaps add a `use` for it:
           `use hashes::hex::FromHex;`
   = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

[dr-orlovsky]

Very strange, what I get is:

rust-bitcoin % git rebase -x '~/.cargo/bin/cargo test && ~/.cargo/bin/cargo +1.22.0 test && ~/.cargo/bin/cargo test --features "use-serde rand"'
Executing: ~/.cargo/bin/cargo test && ~/.cargo/bin/cargo +1.22.0 test && ~/.cargo/bin/cargo test --features "use-serde rand"
    Finished dev [unoptimized + debuginfo] target(s) in 0.12s
     Running target/debug/deps/bitcoin-bfc5fbd5ae8e345a

and after quite a lot of time I get

Successfully rebased and updated refs/heads/hashtypes.

rust-bitcoin % git status
On branch hashtypes
Your branch is up to date with 'origin/hashtypes'.

nothing to commit, working tree clean

What am I doing wrong?

PS. Sorry, figured that out myself, I have missed master suffix while I was copying the string. Now everything runs as in your output. Working on the fixes

[dr-orlovsky]

@apoelstra done. It is needed to reset Travis cache and re-run tests though.

[apoelstra]

Great, thanks! I'm unlikely to get it done today on account of Christmas :)

[dr-orlovsky]

Sure, I am too celebrating the Winter Solstice and Solar Eclipse :)

[apoelstra]

My rebase command still fails on the first three commits (at least) :(

One persistent error is

error: expected one of `,` or `as`, found `::`
  --> src/hash_types.rs:22:49
   |
22 | use hashes::{sha256, sha256d, hash160, Hash, hex::{ToHex, FromHex}};
   |                                                 ^^ expected one of `,` or `as` here

on 1.22.

[dr-orlovsky]

@apoelstra very strange, it could be that I had pushed a wrong version. Have re-done rebasing and double-checked, it seems to be fine now.

PS. Travis still fails due to its internal problem, needs to reset cache.

[apoelstra]

ACK. Thanks for the many revisions.

[elichai]

LGTM.
only logic change is the bitcoin_merkle_root, which looks ok