The code before the fix only checked the length of R value of the last signature in the loop, and only for equality (but the length can be less than 32)
The fixed code checks that length of the R value is less than or equal to 32 on each iteration of the loop
ACK 0d96e2ff6fd68324dedf9b0963c775cc01c29998
180 | sig.clear(); 181 | std::string msg = "A message to be signed" + ToString(i); 182 | msg_hash = Hash(msg); 183 | BOOST_CHECK(key.Sign(msg_hash, sig)); 184 | - found = sig[3] == 0x20; 185 | + BOOST_CHECK(sig[3] <= 0x20);
BOOST_CHECK is fairly slow, this is likely why it is not used in the inner loop directly.
Do you think it would be better to do
if( sig[3] > 0x20 ) {
found_big = true;
break;
}
and then BOOST_CHECK(!found_big) after the loop ?
Also there is BOOST_CHECK for key.Sign() inside the loop, do you think it should be taken outside the loop, too ?
Yes, I think it's better to keep it in a similar way as it is now, assign to a boolean then assert that after the loop.
Also there is BOOST_CHECK for key.Sign() inside the loop, do you think it should be taken outside the loop, too ?
Probably! I mean, a loop of 256 isn't that bad, I guess, but there's no reason a tl east to make things worse.
The code before the fix only checked the length of R value of the last
signature in the loop, and only for equality (but the length can be
less than 32)
The fixed code checks that length of the R value is less than or equal
to 32 on each iteration of the loop
The BOOST_CHECK(sig.size() <= 70) is merged with sig[3] <= 32 check,
and BOOST_CHECKs are moved outside the loop, for efficiency
rebased
merged checks of sig.size() <= 70 and sig[3] <= 32 (with added comment)
moved BOOST_CHECKs out of the loop
fixed a comment ("less than or equal to 70", not "less than 70")
ACK 89895773b72275a620951730aef0b52e9437bc13