I'm not sure why this was delayed to later in the check, but it seems obvious that you could check this much earlier in the computation.
This probably serves no (or at least a very limited) performance implication, however, it does read more lexicographically in terms of the format, therefore I felt it was a worthwhile change.
This shouldn't change the semantics of the code at all, but it does provide a more consistent, lexicographical ordering in relation to the format it self.
63 | 64 | - // Make sure the length of the S element is still inside the signature. 65 | + // Zero-length integers are not allowed for R. 66 | + if (lenR == 0) return false; 67 | + 68 | + // Make sure the length of the R element is still inside the signature.
We are checking that 0x30 [total-length] 0x02 [R-length] [R] 0x02 is still inside the signature, 5 + lenR.
I therefore changed this to Make sure the length of the R element is still inside the signature
Is this really clearer? This makes the BIP text (slightly) further from the used implementation.
This is true, though I saw the Status: draft and thought it would be ok at least bring it up.
As you said, it is only slightly, however it changes the order from:
# STRICT BITCOIN FORMAT
// Minimum and maximum size constraints.
# DER FORMAT/LENGTH
# 0x02 INTEGERS ASSUMED
// A signature is of type 0x30 (compound).
// Make sure the length covers the entire signature.
// Extract the length of the R element.
// Make sure the length of the R element is still inside the signature.
// Extract the length of the S element.
// Verify that the length of the signature matches the sum of the length of the elements.
# R DER FORMAT
// Check whether the R element is an integer.
// Zero-length integers are not allowed for R.
# R STRICT BITCOIN FORMAT
// Negative numbers are not allowed for R.
// Null bytes at the start of R are not allowed, unless R would otherwise be interpreted as a negative number.
# S DER FORMAT
// Check whether the S element is an integer.
// Zero-length integers are not allowed for S.
# S STRICT BITCOIN FORMAT
// Negative numbers are not allowed for S.
// Null bytes at the start of S are not allowed, unless S would otherwise be interpreted as a negative number.
To
# STRICT BITCOIN FORMAT
// Minimum and maximum size constraints.
# DER FORMAT
// A signature is of type 0x30 (compound).
// Make sure the length covers the entire signature.
# R DER FORMAT
// Check whether the R element is an integer.
// Extract the length of the R element.
// Zero-length integers are not allowed for R.
// Make sure the length of the R element is still inside the signature.
# S DER FORMAT
// Check whether the S element is an integer.
// Extract the length of the S element.
// Zero-length integers are not allowed for S.
// Verify that the length of the signature matches the sum of the length of the elements.
# STRICT BITCOIN FORMAT
// Negative numbers are not allowed for R.
// Null bytes at the start of R are not allowed, unless R would otherwise be interpreted as a negative number.
// Negative numbers are not allowed for S.
// Null bytes at the start of S are not allowed, unless S would otherwise be interpreted as a negative number.
Namely, it removes an assumption that there is 0x02 DER integers by enforcing it earlier, and it does most of the DER checking first, with the stricter Bitcoin rules encapsulated to the bottom (and very top, respectively).
Though, it might be debatable that the zero length checks are part of STRICT BITCOIN FORMAT section too, but I 100% wasn't sure.
This doesn't actually change anything semantically, because we only accept this format. However, it is more representative of how it could be parsed (strictly), even if you used a ASN1 parsing library.
It's too late to change this, now that BIP66 already went into effect. Must be careful not to introduce differences between the document and the consensus code in actual use.
Good point on the status, it should be changed to Final.
Ok, closing then.