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

Remove the BitcoinHash trait #385

Merged
merged 1 commit into from Jan 13, 2020

Conversation

@stevenroose
Copy link
Collaborator

stevenroose commented Jan 10, 2020

Replaced by a block_hash method on both Block and BlockHeader.

Fixes #383.

This is a breaking change and is not very urgent. So either we can just stall it until we want another breaking change, or when we merge it we'd need a 0.23.x branch for minor releases.

@stevenroose

This comment has been minimized.

Copy link
Collaborator Author

stevenroose commented Jan 10, 2020

I actually think this was an oversight in the hash newtypes PR because it was already removed for transactions.

Replaced by a `block_hash` method on both `Block` and `BlockHeader`.
Copy link
Member

apoelstra left a comment

concept ACK

let mut engine = BlockHash::engine();
self.consensus_encode(&mut engine).expect("engines don't error");
BlockHash::from_engine(engine)
}

This comment has been minimized.

Copy link
@dr-orlovsky

dr-orlovsky Jan 10, 2020

Contributor

I'm interested why not we use BlockHash::hash function instead and use longer version with HashEngine

This comment has been minimized.

Copy link
@stevenroose

stevenroose Jan 13, 2020

Author Collaborator

Because to use BlockHash::hash you first need to allocate the serialized block into a Vec<u8>. It's more efficient to serialize the data directly into the engine so you don't need to allocate a byte vector.

This comment has been minimized.

Copy link
@dr-orlovsky

dr-orlovsky Jan 14, 2020

Contributor

Ok, see it. Then better to re-factor this repeated code into a different function – I wrote this suggestion elsewhere: #391 (comment)

This comment has been minimized.

Copy link
@stevenroose

stevenroose Jan 14, 2020

Author Collaborator

I don't really think this 3 lines of code are a problem, though.

Copy link
Collaborator

elichai left a comment

utACK

Copy link
Collaborator

elichai left a comment

utACK

@stevenroose stevenroose merged commit e76803b into rust-bitcoin:master Jan 13, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.