Message signing and altcoins #181
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
| Fixes the last (known) missing feature in relation to #84. |
Contributor
| LGTM +1 |
| +1 awesome! |
Contributor
| +1, will merge after I get one more ACK on #179. |
Contributor
There was a problem hiding this comment.
Probably not related to this particular PR, it occurs to me that ecdsa maybe not be the best place to host parseSigCompact because
- It is not used else where other than message sig verification. And I don't know how useful it is to expose it as a publicly accessible API.
- Sitting side by side with
parseSigit is just confusing.
Maybe consider moving it into message.js and make it a internal function?
Contributor Author
There was a problem hiding this comment.
Its a valid concern, but I'm not sure where to put it. I settled with the status-quo after trying to figure out where to put ecdsa.serializeSigCompact.
I don't mind, but it should be noted it is a custom format specifically for Bitcoin.
The use of fixtures allows for more behavioural driven tests and simpler addition of more test cases in future. However, as ECPubKey is just a wrapper around other strenuously tested modules, the test data is currently limited to testing a subset of the total wrapper. This should probably be done better by using mocked out modules instead.
An ECKey is a composition of a private key (D), a public key (Q) and its compression flag. These functions gave the impression of serialization of this composition, when really they only serialized `D`. They have therefore been removed in favour of always using a sane serialization format (WIF) that matches the needed behaviour. If a user needs the previous functionality, simply use `privKey.D.*` instead of `privKey.*`, as BigInteger supports `*Buffer/*Hex` functions as expected.
The signature verifications here are unnecessary, as this is not what is under test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 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.
Chained on #180, but not dependent.
This pull request centers mostly around adding support for alternative
strMessageMagicconstants which often vary between alt-coin implementations.The implemented list of constants can be seen here.
It should be noted that the length has been included at the front of these constants as inline hex constants
\x18, this is to save on adding the generic variable integer code that would be needed to do it.Also, for all signature data, I've opted towards base64, as that is easily verifiable against
bitcoin-qt.Comparison link: https://github.com/dcousens/bitcoinjs-lib/compare/eckey1...mesnet