Skip to content

Commit

Permalink
Adding GroupElement validation test and removing unneeded test
Browse files Browse the repository at this point in the history
  • Loading branch information
levonpetrosyan93 committed Feb 24, 2022
1 parent 566da4f commit ed95c59
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 4 deletions.
2 changes: 1 addition & 1 deletion src/secp256k1/src/cpp/GroupElement.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -531,7 +531,7 @@ const unsigned char* GroupElement::deserialize(const unsigned char* buffer) {
secp256k1_gej_set_ge(reinterpret_cast<secp256k1_gej *>(g_), &result);

if (!secp256k1_ge_is_valid_var(&result)) {
throw "GroupElement: deserialize failed";
throw std::runtime_error("GroupElement: deserialize failed");
}
return buffer + memoryRequired();
}
Expand Down
23 changes: 20 additions & 3 deletions src/sigma/test/serialize_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,33 @@ BOOST_AUTO_TEST_CASE(group_element_serialize)
BOOST_CHECK(initial == resulted);
}

BOOST_AUTO_TEST_CASE(group_element_serialize_infinity)
BOOST_AUTO_TEST_CASE(group_element_invalid)
{
secp_primitives::GroupElement initial;
initial.randomize();
unsigned char buffer [initial.memoryRequired()];
initial.serialize(buffer);
//Shift all elements to right by 2, to make it invalid
for (int i = initial.memoryRequired() - 1; i > 1; i--)
buffer[i] = buffer[i - 2];
buffer[0] = std::rand();
buffer[1] = std::rand();

secp_primitives::GroupElement resulted;
resulted.deserialize(buffer);
BOOST_CHECK(initial == resulted);
BOOST_CHECK_THROW(resulted.deserialize(buffer), std::exception);
}

//This test is not valid already, as infinity point is invalid in check secp256k1_ge_is_valid_var, GroupElement.cpp:533
//BOOST_AUTO_TEST_CASE(group_element_serialize_infinity)
//{
// secp_primitives::GroupElement initial;
// unsigned char buffer [initial.memoryRequired()];
// initial.serialize(buffer);
// secp_primitives::GroupElement resulted;
// resulted.deserialize(buffer);
// BOOST_CHECK(initial == resulted);
//}

This comment has been minimized.

Copy link
@AaronFeickert

AaronFeickert Feb 24, 2022

Contributor

Rejecting the identity group element seems risky, as it's not inherently invalid. I recommend that checks for the identity be done at a higher level for algorithms that require this be done. Existing code already does this, and the new Spark code will check for it as needed.

BOOST_AUTO_TEST_CASE(scalar_serialize)
{
secp_primitives::Scalar initial;
Expand Down

0 comments on commit ed95c59

Please sign in to comment.