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

Unwind #1144 txid malleability #1316

Merged
merged 8 commits into from Sep 7, 2016

Conversation

Projects
None yet
5 participants
@bitcartel
Copy link
Contributor

bitcartel commented Aug 27, 2016

This PR unwinds #1144, retaining GetTxid() as an alias for GetHash() to avoid having to rename across many files and any code in progress.

The txid gtest has been updated to verify that GetTxid() returns the same result as GetHash().

Both zcash-gtest and test_bitcoin pass.

Reviewers: It's easiest to review the 5 commits in chronological order. Each commit is quite small. Test files that have been rolled back (bloom_tests, script_invalid, script_valid) appear to be a big change, so it's easier to diff against the commit mentioned in the log.

@bitcartel bitcartel added this to the Beta 1 - complete RPC, audit mitigations, other linux support, user bugfixes milestone Aug 27, 2016

bitcartel added a commit to bitcartel/zcash that referenced this pull request Aug 28, 2016

Closes zcash#1316. RPC getblocksubsidy height parameter is now option…
…al and

a test has been added to verify parameter input and results.
@nathan-at-least

This comment has been minimized.

Copy link
Contributor

nathan-at-least commented Aug 29, 2016

ACK on diff review; I didn't do test coverage review.

Also, I haven't decided if we do want to roll this back; I'm still learning about the security findings.

@nathan-at-least

This comment has been minimized.

Copy link
Contributor

nathan-at-least commented Aug 29, 2016

We've decided to merge this for Beta 1 and roll back this feature.

@ebfull

This comment has been minimized.

Copy link
Contributor

ebfull commented Aug 30, 2016

I want us to get rid of the GetTxid/GetHash dichotomy and do a complete revert of #1144. It's not too tricky to see that the revert was done properly, just a git diff is needed.

@bitcartel

This comment has been minimized.

Copy link
Contributor Author

bitcartel commented Aug 30, 2016

@ebfull I can add a commit to refactor GetTxid() calls as GetHash() and then remove that method completely. Is that ok?

@ebfull

This comment has been minimized.

Copy link
Contributor

ebfull commented Aug 30, 2016

@bitcartel That would be fine with me! :)

@bitcartel

This comment has been minimized.

Copy link
Contributor Author

bitcartel commented Aug 30, 2016

Note that the source tree will contain three references to a method getTxID() used by the QT gui. The capitalization is different from the method we removed.

@ebfull

This comment has been minimized.

Copy link
Contributor

ebfull commented Aug 30, 2016

We're gonna wait until the last second to merge this that way our work on latest for the RPC can play nicely with the testnet before beta.

@daira

This comment has been minimized.

Copy link
Contributor

daira commented Sep 7, 2016

utACK+cov.

@ebfull

This comment has been minimized.

Copy link
Contributor

ebfull commented Sep 7, 2016

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 7, 2016

📌 Commit 131f020 has been approved by ebfull

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 7, 2016

⌛️ Testing commit 131f020 with merge c6eec79...

zkbot pushed a commit that referenced this pull request Sep 7, 2016

zkbot
Auto merge of #1316 - bitcartel:zc.v0.11.2.z9_unwind_1144, r=ebfull
Unwind #1144 txid malleability

This PR unwinds #1144, retaining GetTxid() as an alias for GetHash() to avoid having to rename across many files and any code in progress.

The txid gtest has been updated to verify that GetTxid() returns the same result as GetHash().

Both zcash-gtest and test_bitcoin pass.

Reviewers: It's easiest to review the 5 commits in chronological order.  Each commit is quite small.  Test files that have been rolled back (bloom_tests, script_invalid, script_valid) appear to be a big change, so it's easier to diff against the commit mentioned in the log.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 7, 2016

💔 Test failed - zcash

@bitcartel bitcartel force-pushed the bitcartel:zc.v0.11.2.z9_unwind_1144 branch from 131f020 to fa511e1 Sep 7, 2016

@ebfull

This comment has been minimized.

Copy link
Contributor

ebfull commented Sep 7, 2016

Thanks!

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 7, 2016

📌 Commit fa511e1 has been approved by ebfull

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 7, 2016

⌛️ Testing commit fa511e1 with merge fa511e1...

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 7, 2016

☀️ Test successful - zcash

@zkbot zkbot merged commit fa511e1 into zcash:zc.v0.11.2.latest Sep 7, 2016

1 check passed

homu Test successful
Details

@solardiz solardiz referenced this pull request Nov 15, 2018

Merged

New txid calculation #16

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment