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
ec_scalar: add non-const accessor and construct by value #1080
Conversation
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 test case fix is great, but the other aspects are problematic for the reasons given here. These were considered on the original PR.
@@ -32,6 +32,7 @@ class BC_API ec_scalar | |||
/// Constructors. | |||
ec_scalar(); | |||
ec_scalar(const ec_secret& secret); | |||
ec_scalar(uint64_t value); |
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 constructor was removed from the original pull request. There is no mapping from a 64 bit value to an ec_secret
nor should a 64 bit value ever be considered a secret. If the caller intends to implement such a mapping it should be implemented outside of this class and passed in as an ec_secret
.
@@ -128,5 +136,9 @@ const ec_secret& ec_scalar::secret() const | |||
{ | |||
return secret_; | |||
} | |||
ec_secret& ec_scalar::secret() |
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 allows the caller to modify ec_scalar internals, which should only be done via the assignment operator (which btw is equally efficient).
@@ -21,71 +21,19 @@ | |||
|
|||
using namespace bc; | |||
|
|||
BOOST_AUTO_TEST_SUITE(ec_scalar_tests) | |||
BOOST_AUTO_TEST_SUITE(ec_point_tests) |
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.
Nice catch.
{ | ||
secret_ = zero; | ||
auto serial = make_unsafe_serializer(secret_.end() - 4); | ||
serial.write_4_bytes_big_endian(value); |
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.
Writing 4 bytes casted from an 8 byte value, as big endian, into the high order bytes of a 32 byte value appears completely arbitrary and unrelated to the behavior of an ec scalar.
As the title says.
non-const accessor is needed to be able to modify underlying secret value without unecessary copying.
construct by value is very useful for doing calculations where we blind output amounts (for example).
Also the unit test filenames for ec_point and ec_scalar were reversed. I switched them over.