Various tests and recoverPubKey change #211
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
@sidazhang https://travis-ci.org/bitcoinjs/bitcoinjs-lib/jobs/27565629, looks like the faucet is still having locking issues. |
Merged
Contributor
There was a problem hiding this comment.
You changed the constructor here, just wanted to make mention of it incase it was an accident (PR comment didn't mention a TX constructor change).
Contributor
Author
There was a problem hiding this comment.
Sorry, this was added after the PR was written, as I continued removing untested functionality.
I will amend the PR.
Contributor
|
+1 |
weilu
added a commit
that referenced
this pull request
Jun 16, 2014
Various tests and recoverPubKey change
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request adds more tests for
ecdsa,messageandscripts.The only functional change is in
recoverPubKey, which as discussed with @jprichardson, has had a superfluous verification step removed in favour of forcing the user to verify the pubKey themselves.This is actually already inherent previously, but there were often only 2 candidate keys, not 4.
This does not affect any of our code, as we only use the
recoverPubKeyoperation inMessage.verifyandecdsa.calcPubKeyRecoveryParam, and in both cases we have independent verification occurring of the returned public key.This change can be seen in https://github.com/dcousens/bitcoinjs-lib/commit/69e086275b6924dd93c8ddcaf86cf5867f7be3d3.
edit: The TransactionIn constructor was also changed in that it had untested functionality removed. This shouldn't be a problem, but was pointed out by @kyledrake. Change made in https://github.com/dcousens/bitcoinjs-lib/commit/e49e1796d53898f35bfd733242699430115bd16b.