BIP-146 using lower S at start

[Kirillvs]

Hello!

Problem

As you know, due to BIP-146 https://github.com/bitcoin/bips/blob/master/bip-0146.mediawiki#user-content-LOW_S, we should use only a lower S

And there is a check in the code (if lengthS == 33 {...}) that converts higher S to lower S, but there are a few problems with it

• first one, this check is indirect, because, if I understand correctly, 31, 32 and 33 bytes are correct lengths for S part of signature
• second one and the most important, sometimes it leads to the incorrect signature

About second problem: sometimes, with some circumstances, after this check and calculating lower S, encoded lower S has length 31 bytes (that is valid for S too), but encodeBigInt function in this case unconditionally adds a leading zero byte, to make length equal 32 bytes, and if before adding zero byte, the s signature had leading byte less than 0x7F byte (that means that we have zero leading bit and is being considered as a positive), it leads to -26: mandatory-script-verify-flag-failed (Non-canonical DER signature) in bitcoin

This check fails https://github.com/bitcoin/bitcoin/blob/80ad135a5e54e8a065fee5ef36e57034679111ab/src/script/interpreter.cpp#L159

    // Null bytes at the start of S are not allowed, unless S would otherwise be
    // interpreted as a negative number.
    if (lenS > 1 && (sig[lenR + 6] == 0x00) && !(sig[lenR + 7] & 0x80)) return false;

Solution

So the solution in this MR is to explicitly check if we have a high S at the start, and if so, we calculating the low S at this point, and after that we don't need to have check of if lengthS == 33 {...}

[mrtnetwork]

Hello!

Problem

As you know, due to BIP-146 https://github.com/bitcoin/bips/blob/master/bip-0146.mediawiki#user-content-LOW_S, we should use only a lower S

And there is a check in the code (if lengthS == 33 {...}) that converts higher S to lower S, but there are a few problems with it

• first one, this check is indirect, because, if I understand correctly, 31, 32 and 33 bytes are correct lengths for S part of signature
• second one and the most important, sometimes it leads to the incorrect signature

About second problem: sometimes, with some circumstances, after this check and calculating lower S, encoded lower S has length 31 bytes (that is valid for S too), but encodeBigInt function in this case unconditionally adds a leading zero byte, to make length equal 32 bytes, and if before adding zero byte, the s signature had leading byte less than 0x7F byte (that means that we have zero leading bit and is being considered as a positive), it leads to -26: mandatory-script-verify-flag-failed (Non-canonical DER signature) in bitcoin

This check fails https://github.com/bitcoin/bitcoin/blob/80ad135a5e54e8a065fee5ef36e57034679111ab/src/script/interpreter.cpp#L159

    // Null bytes at the start of S are not allowed, unless S would otherwise be
    // interpreted as a negative number.
    if (lenS > 1 && (sig[lenR + 6] == 0x00) && !(sig[lenR + 7] & 0x80)) return false;

Solution

So the solution in this MR is to explicitly check if we have a high S at the start, and if so, we calculating the low S at this point, and after that we don't need to have check of if lengthS == 33 {...}

Thank you for the detailed explanation! I'll take a closer look in the next few days.

[mrtnetwork]

Hello!

Problem

As you know, due to BIP-146 https://github.com/bitcoin/bips/blob/master/bip-0146.mediawiki#user-content-LOW_S, we should use only a lower S

And there is a check in the code (if lengthS == 33 {...}) that converts higher S to lower S, but there are a few problems with it

• first one, this check is indirect, because, if I understand correctly, 31, 32 and 33 bytes are correct lengths for S part of signature
• second one and the most important, sometimes it leads to the incorrect signature

About second problem: sometimes, with some circumstances, after this check and calculating lower S, encoded lower S has length 31 bytes (that is valid for S too), but encodeBigInt function in this case unconditionally adds a leading zero byte, to make length equal 32 bytes, and if before adding zero byte, the s signature had leading byte less than 0x7F byte (that means that we have zero leading bit and is being considered as a positive), it leads to -26: mandatory-script-verify-flag-failed (Non-canonical DER signature) in bitcoin

This check fails https://github.com/bitcoin/bitcoin/blob/80ad135a5e54e8a065fee5ef36e57034679111ab/src/script/interpreter.cpp#L159

    // Null bytes at the start of S are not allowed, unless S would otherwise be
    // interpreted as a negative number.
    if (lenS > 1 && (sig[lenR + 6] == 0x00) && !(sig[lenR + 7] & 0x80)) return false;

Solution

So the solution in this MR is to explicitly check if we have a high S at the start, and if so, we calculating the low S at this point, and after that we don't need to have check of if lengthS == 33 {...}

Thanks for that, I've reviewed everything, and it all looks correct.