Use BLS keys for the DIP3 operator key #2352

codablock · 2018-10-15T23:54:14Z

This changes deterministic masternodes to use BLS for the operator key. It requires to generate a key pair with bls generate (which is also part of this PR), use it in protx register and then to configure the MN to use -masternodeblsprivkey with this secret key.

The ECDSA based keys had the advantage that we could manage the MN key in the wallet as well, which is unfortunately removed for the operator key. But I believe that's ok as MNOs are already used to this.

This PR also changes CBitcoinAddress and CBitcoinSecret to support BLS addresses and secrets. I'm not sure if my choice of prefixes was a good one and I'm open for suggestions.

UdjinM6

A couple of quick notes, see inline comments

UdjinM6 · 2018-10-16T02:39:39Z

src/base58.cpp

It's a bit weird to see that you can recover a pubkey from an address (which is normally a hash of the pubkey)... Should really think of implementing it in another way. If you want to use Base58Check encoding for bls pubkeys for some reason (why not just using hex btw?), you should probably consider using CBase58Data in CBLSPublicKey instead of injecting it here.

UdjinM6 · 2018-10-16T02:46:05Z

src/chainparams.cpp

UdjinM6 · 2018-10-16T02:46:33Z

src/chainparams.cpp

should not use mainnet values, could instead reuse the ones for testnet here

UdjinM6 · 2018-10-16T02:46:43Z

src/chainparams.cpp

UdjinM6 · 2018-10-16T03:10:13Z

src/rpc/rpcevo.cpp

3...? shouldn't this be b... according to chainparams?

UdjinM6 · 2018-10-16T03:16:42Z

src/txmempool.h

The name doesn't quite match imo. Also, why using uint256 and hashing all the way instead of using CBLSPublicKey as a map key?

Not sure I understand. Why does the name not match?

The reason I chose to use the hash as key is historical. I implemented this before I kept track of hashes inside CBLSPublicKey (and friends). Before this, using BLS primitives as keys would be horribly slow. I'm also not sure if I really should add an operator< to the primitives which doesn't really compare the primitives but the hashes of these. I'll however at least add a commit that uses .GetHash() instead of doing fresh serialization.

Ok, I see. But, I mean, still, it says mapProTx_BlsPubKeys_ and yet bls pubkeys are nowhere to be found in this map, only their hashes. And then, isn't CBLSId basically it, the hash of the bls pubkey? So this could be:

std::map<CBLSId, uint256> mapProTxBlsPubKeyIDs;

just like mapProTxPubKeyIDs for ecdsa pubkeys. And yes, would have to implement operator< in this case... BUT this led me to another thought - if all CBLSId does is storing hash then why implement it via CBLSWrapper at all? See UdjinM6@1446e93. And then it's trivial to apply changes I described above UdjinM6@05af5bc.

Ah now I see what you mean.

Regarding CBLSId: This came from the initial herumi based implementation, which had blsId as it's own type. This is NOT the ID of any key, it's actually just an arbitrary 256 bit number. This ID is used in the polynomial functions only and identifies a share. It's the X value in the polynomial evaluation and interpolation.

Ah and what I forgot to add. If Chia BLS ever officially starts to support Shamir's Secret Sharing, they might end up implementing their own class for IDs. That's why I left CBLSId as it is now and also let it be a derived class from CBLSWrapper

Hmmm... so that's just some random ID? I guess this was smth I was missing. Anyway... Let's just change the name e.g. std::map<uint256, uint256> mapProTxBlsPubKeyHashes; and keep other stuff untouched then?

Yepp, done.

UdjinM6 · 2018-10-16T03:45:17Z

src/chainparams.h

This is confusing, because it's the actual pubkey, not the address you are using this for.

codablock · 2018-10-16T10:13:02Z

Regarding using the full pubkey instead of a hash: With BLS there is no other way to have an address, because it's not possible to recover the public key from a signature. So later when a signature is verified, you need the full public key and a hash is not enough for verification.

But you made me thinking. I should probably really not use base58 for now. These keys are not going to be used for transactions for now, so we don't need that atm. If we implement base58 addresses for these keys now, we might end up making live harder in the future.

UdjinM6 · 2018-10-16T11:20:31Z

With BLS there is no other way to have an address, because it's not possible to recover the public key from a signature.

But this doesn't make pubkey an address suddenly. Base58Check is important part of it but hashing is also (and probably even more) important. The point of using addresses (hashes of pubkeys instead of pubkeys themselves) is that if there is a vulnerability found in pubkey/privkey relations or some quantum computing thing happens, attacker still won't be able to steal the funds because there is yet another type of the problem to solve for him which is a one way hashing (compression) function.

Also, why you can't use Base58Check of a hash of a CBLSPublicKey (e.g. CBLSId?) as a BLS address in payments which would preserve the property I described above (you'll have to provide pubkey together with the signature when you try to spend funds, but that's just like we do in "normal" p2pkh txes)?

codablock · 2018-10-16T12:08:32Z

We can't provide the full pubkey with the signature because it would be too huge and also would destroy all the benefits of aggregation. A pubkey is 48 bytes and the sig is 96 bytes. The sig is quite huge, but we wouldn't care that much because signature aggregation would reduce it to one sig per TX (or even per block). If we however add pubkeys into the inputs as well, we loose the space savings because we wouldn't be able to aggregate the pubkeys (we wouldn't be able to match the sig pubkey with the hashes from the outputs).

But as said, I should probably just remove the base58/address thing completely and only let us work with hex keys for the operator keys and then later (whenever needed) figure out addresses.

UdjinM6 · 2018-10-16T12:12:41Z

I see. Well, yes, better just drop it for now then to avoid confusion, we'll figure it out later when/if we'll actually use it for payments.

PastaPastaPasta

some nits

PastaPastaPasta · 2018-10-17T23:54:59Z

src/base58.cpp

make it one line

PastaPastaPasta · 2018-10-17T23:55:09Z

src/base58.cpp

add brackets

PastaPastaPasta · 2018-10-17T23:55:27Z

src/base58.cpp

one line (or brackets)

PastaPastaPasta · 2018-10-17T23:58:12Z

src/evo/providertx.cpp

brackets pls

PastaPastaPasta · 2018-10-17T23:58:33Z

src/evo/providertx.cpp

brackets/same line

PastaPastaPasta · 2018-10-18T00:01:44Z

src/governance-object.cpp

this could be combined with &

that would require to include !deterministicMNManager->IsDeterministicMNsSporkActive() && ... into the else if case, which I don't like. I find this style much easier to read and understand as it's immediately clear that it begins with 2 possible cases of which also have 2 possible cases.

PastaPastaPasta · 2018-10-18T00:01:47Z

src/governance-object.cpp

Why is this two lines... I'd like it as one.

because it's actually 4 cases we handle here and I like things to be as explicit as possible.

PastaPastaPasta · 2018-10-18T00:02:10Z

src/governance-vote.cpp

combine to one line

In this case, I actually left in some unfinished garbage by mistake. Added a commit on-top.

PastaPastaPasta · 2018-10-18T00:04:20Z

src/instantx.cpp

does clang want this on second line? I don't...

PastaPastaPasta · 2018-10-18T00:05:32Z

src/instantx.cpp

this should be indented further, might be out of scope for this PR tho

Agree, intending was wrong here in the beginning. Fixed it.

codablock · 2018-10-19T13:53:58Z

Removed CBitcoinAddress support and changed to use hex strings instead.

Also added commits of #2357 on-top to fix a build issue. I'll remove these later when #2357 got merged.

@PastaPastaPasta Did a few review fixes but left out the ones that are related to files where code-style generally differs. I'll do a cleanup/formatting PR for this later.

UdjinM6 · 2018-10-20T14:25:15Z

#2357 merged, pls remove corresponding commits

Needed because the Chia BLS library crashes when keys are created before the library is initialized, even if keys are not used. This is the case here as we have static instances here.

This seems to be some garbage I left in by mistake.

All keys that are used as examples are random. None of the secret keys belongs to any of the public keys.

…ool.cpp

codablock · 2018-10-20T19:57:57Z

@UdjinM6 done

PastaPastaPasta

utACK, assuming a clean up PR in the near ish future

UdjinM6

utACK

Related to dashpay/dash#2352

* P2P - Remove dsa and dsq input count Related to dashpay/dash#2075, dashpay/dash#2259, and dashpay/dash#2318 * P2P - Update dsq and dstx to include BLS signature Related to dashpay/dash#2352 * P2P - Update govobj and govobjvote to include BLS signature Related to dashpay/dash#2352

UdjinM6 added this to the 12.4 milestone Oct 16, 2018

UdjinM6 requested changes Oct 16, 2018

View reviewed changes

PastaPastaPasta requested changes Oct 18, 2018

View reviewed changes

codablock force-pushed the pr_blsforoperator branch from b99f36e to 4ba5c26 Compare October 19, 2018 10:31

codablock mentioned this pull request Oct 19, 2018

Update Chia BLS libs to latest master #2357

Merged

codablock added 8 commits October 20, 2018 21:56

Add "bls generate" RPC to generate BLS keys

7ae774a

Use BLS keys for operator keys

d7faf6b

Use unique_ptr to store blsKeyOperator and blsPubKeyOperator

282f596

Needed because the Chia BLS library crashes when keys are created before the library is initialized, even if keys are not used. This is the case here as we have static instances here.

Remove unnecessary CheckSignature calls

ee758f4

This seems to be some garbage I left in by mistake.

Fixed review comments

d8cf7f0

Fix rpc help for operator keys

12266a3

All keys that are used as examples are random. None of the secret keys belongs to any of the public keys.

Use .GetHash() instead of ::SerializeHash() for BLS pubkeys in txmemp…

d1a8570

…ool.cpp

Rename mapProTxBlsPubKeys to mapProTxBlsPubKeyHashes

b8d7755

codablock force-pushed the pr_blsforoperator branch from 2f97d79 to b8d7755 Compare October 20, 2018 19:56

PastaPastaPasta approved these changes Oct 21, 2018

View reviewed changes

UdjinM6 approved these changes Oct 21, 2018

View reviewed changes

UdjinM6 merged commit c9d2745 into dashpay:develop Oct 21, 2018

thephez added a commit to dash-docs/dash-docs that referenced this pull request Dec 26, 2018

P2P - Update dsq and dstx to include BLS signature

fe9a9dc

Related to dashpay/dash#2352

thephez added a commit to dash-docs/dash-docs that referenced this pull request Dec 26, 2018

P2P - Update govobj and govobjvote to include BLS signature

7024112

Related to dashpay/dash#2352

codablock deleted the pr_blsforoperator branch December 27, 2018 15:24

Use BLS keys for the DIP3 operator key #2352

Use BLS keys for the DIP3 operator key #2352

Uh oh!

Conversation

codablock commented Oct 15, 2018

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codablock commented Oct 16, 2018

Uh oh!

UdjinM6 commented Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codablock commented Oct 16, 2018

Uh oh!

UdjinM6 commented Oct 16, 2018

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codablock commented Oct 19, 2018

Uh oh!

UdjinM6 commented Oct 20, 2018

Uh oh!

codablock commented Oct 20, 2018

UdjinM6 commented Oct 16, 2018 •

edited

Loading