-
Notifications
You must be signed in to change notification settings - Fork 16
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
Remove PBC based cryptography #71
Conversation
d5042c5
to
76db063
Compare
76db063
to
d558860
Compare
Nice work and good riddance tpke/pbc (they will not be missed!) It would be good to run this branch with miner and do a local |
There's still dangling code that remains. Now that |
Thanks! Will do so. |
Yes we can probably remove the curve parameter now. |
Removed 5ab4d22 |
src/hbbft.erl
Outdated
|
||
-record(hbbft_data, { | ||
batch_size :: pos_integer(), | ||
curve :: curve(), | ||
key_share :: key_share(), | ||
key_share :: undefined | tc_key_share:tc_key_share(), |
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.
When can the key share be undefined?
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.
@Vagabond Great question. I simply replaced:
-type key_share() :: undefined | tc_key_share:tc_key_share() | tpke_privkey:privkey().
...
key_share :: key_share(),
with
key_share :: undefined | tc_key_share:tc_key_share(),
without checking if undefined
is still needed at all.
Will check.
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.
@Vagabond Indeed it doesn't look possible, since init
requires it and it never appears to be removed.
Do we (still) need have_key/1
then?
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.
It appears to be used in miner as some extra sanity assertion:
https://github.com/helium/miner/blob/master/src/miner_consensus_mgr.erl#L396
https://github.com/helium/miner/blob/master/src/miner_consensus_mgr.erl#L924
https://github.com/helium/miner/blob/master/src/miner_consensus_mgr.erl#L1041
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.
Looks good to me. All tests pass locally for me and have also ran run.sh
in miner with this branch with some basic start/stop testing.
fixes #70