Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert internal BIP-143 hashes to sha256d::Hash #391

Conversation

@stevenroose
Copy link
Collaborator

stevenroose commented Jan 13, 2020

They are not really SigHashes, just internal hashes.
Sadly this is a breaking change since the fields of this struct are
exposed. (I think they should be exposed, though.)

It also meant that the order of serialization is reverted.

Two things unrelated to this PR:

  1. The from_hex macro for testing is actually quite useful. I think it can be used in a lot more tests. I can do a follow-up PR if there is interest.
  2. I'm confused why the SigHash values had to be reversed. The hash_newtype macro preserves display order of the underlying hash. I think because they were not using the "display order" but the raw byte hex order for parsing.
They are not really SigHashes, just internal hashes.
Sadly this is a breaking change since the fields of this struct are
exposed. (I think they should be exposed, though.)

It also meant that the order of serialization is reverted.
@instagibbs

This comment has been minimized.

Copy link
Contributor

instagibbs commented Jan 13, 2020

concept ACK on the type change

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 13, 2020

Codecov Report

Merging #391 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #391      +/-   ##
=========================================
- Coverage   84.91%   84.9%   -0.01%     
=========================================
  Files          40      40              
  Lines        7280    7282       +2     
=========================================
+ Hits         6182    6183       +1     
- Misses       1098    1099       +1
Impacted Files Coverage Δ
src/internal_macros.rs 59.47% <ø> (ø) ⬆️
src/util/bip143.rs 100% <100%> (ø) ⬆️
src/hash_types.rs 87.5% <0%> (-12.5%) ⬇️

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 e76803b...2960582. Read the comment docs.

@stevenroose

This comment has been minimized.

Copy link
Collaborator Author

stevenroose commented Jan 13, 2020

@instagibbs If you refactor #390 to use a new components/cache structure, I can just leave this unchanged and mark is deprecated.

@@ -227,7 +227,7 @@ macro_rules! display_from_debug {
macro_rules! hex_script (($s:expr) => (::blockdata::script::Script::from(::hex::decode($s).unwrap())));

#[cfg(test)]
macro_rules! hex_hash (($h:ident, $s:expr) => ($h::from_slice(&::hex::decode($s).unwrap()).unwrap()));
macro_rules! from_hex (($s:expr) => (::hashes::hex::FromHex::from_hex($s).unwrap()));

This comment has been minimized.

Copy link
@dr-orlovsky

dr-orlovsky Jan 14, 2020

Contributor

It might be worth naming this function hash_from_hex or somehow similarly to indicate that it returns Hash structure, not just converts random hex string into Vec or something

This comment has been minimized.

Copy link
@stevenroose

stevenroose Jan 14, 2020

Author Collaborator

It can do Vec<u8> as well. Anything that has FromHex implemented can be parsed with this.

You can do let bytes: Vec<u8> = from_hex!("aabb");.

for txout in &tx.output {
txout.consensus_encode(&mut enc).unwrap();
}
SigHash::from_engine(enc)
sha256d::Hash::from_engine(enc)

This comment has been minimized.

Copy link
@dr-orlovsky

dr-orlovsky Jan 14, 2020

Contributor

I got your argument why engines are better, however it really seems that we have a plenty of repeated code with the current approach. Moving all patterns of enc=engine....;consensus_encode(engine);...from_engine(enc) into a function in Hash trait seems to be reasonable. Probably this should be done in separate PR on which I can work today

This comment has been minimized.

Copy link
@elichai

elichai Jan 14, 2020

Collaborator

That's usually the case in things that use Read/Write APIs

let mut f: File;
let mut buf = Vec::new();
f.read(&mut but)?;

This comment has been minimized.

Copy link
@dr-orlovsky

dr-orlovsky Jan 14, 2020

Contributor

I will probably still suggest generic programming best practices (avoid repeated code) over Rust std lib API strange parts, which may be just still undersigned at this stage...

This comment has been minimized.

Copy link
@stevenroose

stevenroose Jan 14, 2020

Author Collaborator

See my comment here: #388 (comment)

@stevenroose

This comment has been minimized.

Copy link
Collaborator Author

stevenroose commented Jan 14, 2020

Obsoleted by #390 (review)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.