-
Notifications
You must be signed in to change notification settings - Fork 209
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
Feature/ecdsa #686
Feature/ecdsa #686
Conversation
Co-authored-by: vird <virdvip@gmail.com>
0953c65
to
c790054
Compare
94338e4
to
9676ac3
Compare
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.
LGTM, one small readability suggestion:
potentially would be good to replace pub_hash_key(Pub) use with to_address(Pub) so that doesn't create confusion that they are different notions. Unless you are planning to remove to_address in future
_ -> | ||
{?RSA_KEY_TYPE, Pub} | ||
end | ||
end. |
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.
unnecessary height check as the length pub is telling. (33) cannot be RSA key length.
If the intent is to not allow ECDSA to be used for rewards before 2.9 height, maybe
if KeyType == ECDSA & height < 2.9 then fail
would be more descriptive and direct.
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.
which you already have assert_key_type
, suggest use of that after determining the KeyType with get_reward_key_type(Pub).
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.
assert_key_type ensures we have the proper mining configuration so it's ok to crash. We cannot do it on validation.
@@ -859,11 +870,18 @@ parse_block_post_2_6_fields(B, << HashPreimageSize:8, HashPreimage:HashPreimageS | |||
last_step_checkpoints = parse_checkpoints(LastCheckpoints, Height), | |||
steps = parse_checkpoints(Steps, Height) }, | |||
RecallByte2_2 = case RecallByte2Size of 0 -> undefined; _ -> RecallByte2 end, | |||
SigType = | |||
case {RewardKeySize, Height >= ar_fork:height_2_9()} of | |||
{32, true} -> |
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.
33, same as comment up there. suggest assert_key_type if intent is to disallow ECDSA keys prior to 2.9
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.
Good catch!
* ECDSA support * RFC 6979 signature + ecrecover --------- Co-authored-by: Lev Berman <ldmberman@proton.me> Co-authored-by: Saam Tehrani <samanrtehrani@gmail.com> Co-authored-by: vird <virdvip@gmail.com>
No description provided.