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.
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
Support Koblitz-based and Keccak256-based custom witness verification #3209
Support Koblitz-based and Keccak256-based custom witness verification #3209
Changes from 6 commits
b77b5ed
7b730b3
46d6c51
96e3af0
52d8a83
48d3177
018968f
e7d9122
225f7f5
79b5626
cec836e
ebe2588
b7f86e9
02cbc50
c8bb5f5
a70b85c
47fe071
a7eaf7a
f565ea4
c2d9801
ff482ec
8d970eb
fc5f540
d108a03
a349af8
fe1b449
e2c88f6
bab3b21
64959f7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cache
DomainParameters
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do this, will update the PR in a few minutes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, take a look at the updated version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should the else branch be dropped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because we still have to process SHA256 with Koblitz curve (if not on mac).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but i think BouncyCastle is able to handle this case, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe the
IsOSX && pubkey.Curve == ECC.ECCurve.Secp256k1
is introduced here for performance optimization, @shargon do you have some comments here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Secp256k1
isn't natively supported onMacOS
; reason we need to useBouncyCastle
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cschuchardt88 why not just use BouncyCastle and drop the else branch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, to me it seems to be more preferable (and safe) to use native built-in implementation instead of some external libraries, and thus, we're stick to the native ECDsa's sign/verify implementation as much as possible. In future both k1 on macOS and Koblitz hasher may be added to the native library, then we may drop this
if
. But until then I think it's OK to keep BouncyCastle for these two cases.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure why. Maybe we want to avoid 3rd-party libraries or unsupported features for linux/windows support.
Maybe @shargon @Jim8y can help us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hold the opposite view
for blockchain, consistency is very important, and behavioral differences caused by architecture and platform cannot be tolerated.
the implementation of dotnet's Crypto library is to directly call the operating system API instead of implementing it by themselves. even their own developers are not clear enough about these behaviors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's then start a discussion issue, #3220.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think this should be marked as
[Obsolete]
and add creating of new method. We dont want to break wallets, dapps, etc.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The behaviour is compatible. It was SHA256 before this change, and it's now SHA256 by default. Users don't have to change anything in their code, and nothing is broken by this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hasher
is to generic leaves guessing; not being able to identify it as aEnum
. Currently the name says to me that it does something.Maybe
HashingAlgorithmNames
,SigningHashNames
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe two different enums is better than this pair, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need native CryptoLib’s werifyWithECDsa method (its API, in fact) to be compatible with the old one, since there are transactions that call this method. And thus, we use a pair here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Theoretically,
VerifyWithECDsa
could be changed to have two parameters in some HF. But practically, #3210.