Skip to content

Don't hash what you're not going to sign#4694

Closed
jtimon wants to merge 2 commits intobitcoin:masterfrom
jtimon:nohash
Closed

Don't hash what you're not going to sign#4694
jtimon wants to merge 2 commits intobitcoin:masterfrom
jtimon:nohash

Conversation

@jtimon
Copy link
Contributor

@jtimon jtimon commented Aug 13, 2014

A tiny optimization in place that is not performance critical, but in my opinion the end result is also more readable.

@jtimon jtimon mentioned this pull request Aug 13, 2014
Closed
@jgarzik
Copy link
Contributor

jgarzik commented Aug 13, 2014

Having two Solvers() that do different things is ugly. Thanks for cleaning that up.

This probably conflicts with #4692 as will anything that touches script.

@jtimon
Copy link
Contributor Author

jtimon commented Aug 13, 2014

Thanks, I hadn't seen that PR and it definitely interests me since I'm almost exclusively working on refactor script lately. I don't mind to rebase all my work on top of that since the hardest part was funding out the changes I wanted to make.

@jtimon jtimon mentioned this pull request Aug 13, 2014
Closed
@jtimon
Copy link
Contributor Author

jtimon commented Aug 14, 2014

Dependent on #4692

@jtimon
Copy link
Contributor Author

jtimon commented Aug 27, 2014

Rebased on top of #4754

@jtimon
Copy link
Contributor Author

jtimon commented Aug 31, 2014

Rebased on top of #4755

@jtimon jtimon force-pushed the nohash branch 2 times, most recently from d24bba7 to 28f3f4d Compare September 2, 2014 01:14
@jtimon
Copy link
Contributor Author

jtimon commented Sep 2, 2014

Closing until #4754 is merged.

@jtimon jtimon closed this Sep 2, 2014
@jtimon jtimon reopened this Sep 8, 2014
@jtimon jtimon force-pushed the nohash branch 2 times, most recently from 3744560 to 1b5938e Compare September 13, 2014 21:02
@sipa
Copy link
Member

sipa commented Sep 16, 2014

I haven't reviewed fully, but this seems like it could be simplified on top #4890.

@jtimon
Copy link
Contributor Author

jtimon commented Sep 16, 2014

I have already a version rebased on top of that, but I don't see the simplification...

@sipa
Copy link
Member

sipa commented Sep 16, 2014

You don't need to pass txTo and nIn around - just pass the SIgnatureHasher around instead.

@sipa
Copy link
Member

sipa commented Sep 16, 2014

Oh no, sorry - you still need it as the function updates the transaction itself, not just produce the signature. Nevermind.

@BitcoinPullTester
Copy link

BitcoinPullTester commented Sep 17, 2014

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4694_87784ec8d0720affac6d133bf550a0a6471b530b/ for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

@laanwj laanwj mentioned this pull request Oct 2, 2014
Merged
@jtimon
Copy link
Contributor Author

jtimon commented Oct 6, 2014

Rebased.

@jtimon
Copy link
Contributor Author

jtimon commented Oct 29, 2014

Closing until 0.10

@jtimon jtimon closed this Oct 29, 2014
@sipa sipa mentioned this pull request Nov 4, 2014
Merged
jtimon referenced this pull request in sipa/bitcoin Nov 5, 2014
@sipa
Abstract out Ctransaction-specific signing into SignatureCreator
f4b779b
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jtimon @jgarzik @sipa @BitcoinPullTester @laanwj