-
Notifications
You must be signed in to change notification settings - Fork 152
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
Introduce BLS digital signature #2223
Introduce BLS digital signature #2223
Conversation
3827167
to
2c925b5
Compare
f70bdb9
to
3f73480
Compare
ccc25ed
to
1a9fb7c
Compare
Libplanet/Crypto/BlsSignature.cs
Outdated
public BlsSignature(IReadOnlyList<byte> signature) | ||
{ | ||
if (signature is ImmutableArray<byte> i ? i.IsDefaultOrEmpty : !signature.Any()) | ||
{ | ||
throw new ArgumentNullException( | ||
nameof(signature), "Signature is empty."); | ||
} | ||
|
||
if (signature.Count != KeyByteSize) | ||
{ | ||
throw new ArgumentOutOfRangeException( | ||
nameof(signature), | ||
$"The key must be {KeyByteSize} bytes." | ||
); | ||
} | ||
|
||
_signature = signature; | ||
_ = CryptoConfig.ConsensusCryptoBackend.ValidateGetNativeSignature(ToByteArray()); | ||
} |
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.
There is some difference between IReadOnly
and ImmutableArray
/ ImmutableList
. Either this should accept immutable type or copy signature
passed in as argument. Also I think _signature
should also be an immutable type.
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've changed the type of _signature
and other _privatekey
and _publickey
to ImmutableArray<byte>
, and also IPublic/PrivateKey.KeyBytes
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.
Have set type IReadOnlyList
for taking value as byte[]
and let given value read-only in the constructor. The ImmutableList/Array
can be also taken with IReadOnlyList
.
The ImmutableList
can be casted to ImmutableArray
, and if the ImmutableList
of null (i.e., default(ImmutableList)
) comes in, then ArgumentNullException
is thrown by ImmutableArray
, which is matching with exception description.
public static BlsSignature AggregateAll(this BlsSignature[] signatures) => | ||
signatures.Aggregate((x, y) => x.AggregateSignature(y)); |
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.
This would depend on its usage, but there may be a better suited enumerable type instead of []
.
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.
libplanet/Libplanet/Crypto/BlsSignatureExtensions.cs
Lines 10 to 11 in bcf03ac
public static BlsSignature AggregateAll(this IEnumerable<BlsSignature> signatures) => | |
signatures.Aggregate((x, y) => x.AggregateSignature(y)); |
How about this?
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.
Depends on usage. Usually enforcing an immutable type is safer but with worse performance. I don't know enough about the exact context in which this will be used, but from the looks of it, this might get called on a "pool" of signatures that can change often. The IEnumerable
signature signals that this method should be used synchronously or with some sort of locking mechanism from the caller.
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 think this can be used in asynchronous and synchronous in both situation. BlsSignature
is immutable. The AggregateSignature
internally creates new two native signatures from BLS library, and aggregates signatures, which are separate objects from given BlsSignature
s, and the results is packed into new BlsSignature
and returns it.
Does IEnumerable
is signaling the method should be used synchronously mean that LINQ does not support asynchronous operation?
41953cc
to
883c431
Compare
Thanks for catching this out. I have added zero value check in Footnotes |
Also, I'm not if it is exactly relevant, but there was this a while back. 🙄 |
7080503
to
75589d0
Compare
a8e0e8a
to
0223675
Compare
ef4bbc5
to
c3dda59
Compare
c3dda59
to
62eb854
Compare
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
This PR introduces a new digital signature, BLS, which has the aggregable private key, public key, and signature. Due to this property, it has an advantage in verifying multiple signatures with multiple public keys by reducing signatures with aggregation. The implementation uses Minimal-pubkey-size1 (G1 as Public key and G2 as Signature).
How fast it is?
FastAggregateVerify
2 is for the case where the same message but different signatures, andAggregateVerify
3 is for the case where different messages and different signatures.Classes
Use cases
This BLS digital signature will be used in during the PBFT consensus for verifying multiple votes from nodes. There will be another key to store and manage.
The validation of the block will be the aggregation of
LastCommit
. The same procedure can be done with the above (the verification of votes).In a bonding process of the validator, the public key should be validated with Proof of Possession4 created from its private key, for preventing Rouge Key Attack, which is simply saying, publishing the aggregated public key and not using the true public key of one's private key.
Further discussion
There is an ongoing discussion of forbidding zero value private key and public key (See Infinity pubkey and signature supranational/blst#11) due to the soundness and producing the infinite public key and signature. For now, This is allowed for the sake of standardization (IETF standard has no mention of blocking neither zero-value private key nor public key.)
The behavior of the Infinite Public key and Signature should be further investigated. For more details, See also Security of BLS batch verification.
The Proof of Possession is for validating whether the public key is not aggregated and proving there is a pair of private key. It should be enough checking once and that moment is when the non-validator peer is trying to bonding as a validator.
The byte size of
Vote
andLastCommit
will be increased after the replacement of the Public Key and Signature to BLS.This PR does not cover
ProtectedPrivateKey
,Web3KeyStore
,Address
andAddressExtensions
modification.The following discussion should be concerned and the thread is made at #2247.
Footnotes
https://www.ietf.org/archive/id/draft-irtf-cfrg-bls-signature-05.html#section-2.1-2.2.1 ↩
https://www.ietf.org/archive/id/draft-irtf-cfrg-bls-signature-05.html#name-fastaggregateverify ↩
https://www.ietf.org/archive/id/draft-irtf-cfrg-bls-signature-05.html#name-aggregateverify ↩
https://www.ietf.org/archive/id/draft-irtf-cfrg-bls-signature-05.html#name-proof-of-possession ↩