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

Add bip143 sighash support for other flags #390

Merged
merged 2 commits into from Jan 21, 2020

Conversation

@instagibbs
Copy link
Contributor

instagibbs commented Jan 13, 2020

Resolves #384

Tests generated manually in Core.

I can add more tests and dedup the strings a bunch once approach is agreed upon.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 13, 2020

Codecov Report

Merging #390 into master will increase coverage by 0.49%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #390      +/-   ##
==========================================
+ Coverage   84.89%   85.39%   +0.49%     
==========================================
  Files          40       40              
  Lines        7278     7695     +417     
==========================================
+ Hits         6179     6571     +392     
- Misses       1099     1124      +25
Impacted Files Coverage Δ
src/blockdata/transaction.rs 94.66% <100%> (+0.73%) ⬆️
src/util/bip143.rs 100% <100%> (ø) ⬆️
src/hash_types.rs 55% <0%> (-45%) ⬇️
src/blockdata/constants.rs 82.81% <0%> (-2.59%) ⬇️
src/util/psbt/serialize.rs 83.63% <0%> (-0.98%) ⬇️
src/blockdata/opcodes.rs 97.28% <0%> (-0.48%) ⬇️
src/util/uint.rs 83.43% <0%> (-0.22%) ⬇️
src/blockdata/script.rs 78.45% <0%> (-0.15%) ⬇️
src/network/message_filter.rs 100% <0%> (ø) ⬆️
... and 11 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 2cba819...d1c5c7b. Read the comment docs.

@stevenroose

This comment has been minimized.

Copy link
Collaborator

stevenroose commented Jan 13, 2020

What is the reason you can't merge this into the SigHashComponents in bip143.rs?

I think it'd be more suitable there. Like this you're generating a new SigHashComponents for every time you're calculating a SigHash, which might be many times per transaction.

@stevenroose

This comment has been minimized.

Copy link
Collaborator

stevenroose commented Jan 13, 2020

(Side note: I noticed that the SigHashComponents::hash_* fields are of the type SigHash. I don't really agree with this, but I didn't catch them in my review of the newtypes PR. I think they are not sighashes yet and should just have stayed sha256(d)::Hash. I can PR that. It's a breaking change, though as they are public fields..)

@instagibbs

This comment has been minimized.

Copy link
Contributor Author

instagibbs commented Jan 13, 2020

@stevenroose I find the status-quo a bit odd myself: the sighash calculation being a function member of the SigHashComponents struct. I'm open to whatever, it just seemed more straight forward at the time to include it in Transaction just like the "legacy" sighash, wtxid calculation, etc.

One idea to reduce re-computation is for the caller to send in an Option<SigHashComponents>?

@stevenroose

This comment has been minimized.

Copy link
Collaborator

stevenroose commented Jan 13, 2020

I agree that the API for this is a bit unintuitive. But it makes sense for efficiency. The idea is that very few transactions require only a single sighash calculation. There's almost always multiple inputs and many inputs have multiple signatures. So it should be strongly encouraged to calculate those upfront.

Once you find the bip143 module, the API is very workable. So perhaps we should just refer to it in the Transaction docs or so.

@instagibbs

This comment has been minimized.

Copy link
Contributor Author

instagibbs commented Jan 13, 2020

@stevenroose did a Take 2 of this PR, moving it to bip143 util as discussed, looking to deprecate the old API.

I'll clean up history if I get concept ACKs, and adapt current tests

Copy link
Collaborator

stevenroose left a comment

I like the new approach. Indeed I'd vote for deprecation of SighashComponents in this PR then as well.

Anyone have strong feelings about providing a special-case sighash_all function for compatibility/convenience? I don't think it's that much needed. Also the &TxIn argument is no longer needed, so the method signature will change anyhow.

(This also makes #391 obsolete.)

src/util/bip143.rs Outdated Show resolved Hide resolved
src/util/bip143.rs Show resolved Hide resolved
src/util/bip143.rs Outdated Show resolved Hide resolved
@instagibbs instagibbs force-pushed the instagibbs:bip143_sighash_notall branch 2 times, most recently from 8b2bc8b to 52e9d61 Jan 14, 2020
@stevenroose

This comment has been minimized.

Copy link
Collaborator

stevenroose commented Jan 16, 2020

The deprecated warning prints are definitely super annoying.

I complained about them here: rust-lang/rust#47219

What seems to reduce them the most is if you would add a #[allow(deprecated)] tag to the impl SighashComponents block and the mod test block in the bip143 module. I'd suggest we do that. It keeps the deprecation overhead to this module and reduces warning messages that might make us miss out on future other warnings.

@instagibbs instagibbs force-pushed the instagibbs:bip143_sighash_notall branch from 66289ae to 89c8bcc Jan 16, 2020
@instagibbs

This comment has been minimized.

Copy link
Contributor Author

instagibbs commented Jan 16, 2020

made the deprecation slightly less annoying

Copy link
Collaborator

stevenroose left a comment

Cool! Very happy about the lazy calculation of the intermediate hashes!

src/util/bip143.rs Outdated Show resolved Hide resolved
src/util/bip143.rs Outdated Show resolved Hide resolved
src/blockdata/transaction.rs Outdated Show resolved Hide resolved
src/util/bip143.rs Outdated Show resolved Hide resolved
src/util/bip143.rs Outdated Show resolved Hide resolved
src/util/bip143.rs Outdated Show resolved Hide resolved
src/util/bip143.rs Outdated Show resolved Hide resolved
@instagibbs instagibbs force-pushed the instagibbs:bip143_sighash_notall branch from 89c8bcc to 660e850 Jan 21, 2020
@instagibbs instagibbs force-pushed the instagibbs:bip143_sighash_notall branch from 660e850 to d1c5c7b Jan 21, 2020
Copy link
Collaborator

elichai left a comment

ACK
d1c5c7b
also compared the logic to make sure it matches bip143

@stevenroose stevenroose merged commit 930a6ca into rust-bitcoin:master Jan 21, 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
5 participants
You can’t perform that action at this time.