bolt11: clarify signature normalization requirements

[erickcestari]

Explicitly document that signature validation requirements differ based on the presence of the n field:

• With n field: signature MUST be normalized lower-S form
• Without n field: pubkey recovery accepts both high-S and low-S

This clarifies existing implementation behavior where secp256k1_ecdsa_verify (used with n field) requires normalized signatures, while secp256k1_ecdsa_recover (used without n field) accepts both normalized
and non-normalized signatures.

cc @real-or-random

[real-or-random]

More context:

• Bolt11: Signature verification fails after public key recovery due to s-value malleability ACINQ/lightning-kmp#802
• Clarify API doc of ecdsa_recover return value bitcoin-core/secp256k1#1718

[t-bast]

Explicitly document that signature validation requirements differ based on the presence of the n field:

Why don't we just require lower-S form in all cases? That would be simpler, wouldn't it?

[erickcestari]

Explicitly document that signature validation requirements differ based on the presence of the n field:

Why don't we just require lower-S form in all cases? That would be simpler, wouldn't it?

It’s an option, but in practice it would add complexity because of how secp256k1_ecdsa_recover works. Most implementations, when recovering the pubkey, pass the signature directly to secp256k1_ecdsa_recover without also calling secp256k1_ecdsa_verify, since verification isn’t required (secp256k1_ecdsa_recover guarantees a correct signature). The current approach matches what the majority of implementations already do, so only a few would need changes. If we mandated low-S everywhere, implementations would have to add extra checks in the recovery path to validate the S value before or after pubkey recovery, which makes things more complicated/redundant.

lnd:

	// If the destination pubkey was provided as a tagged field, use that
	// to verify the signature, if not do public key recovery.
	if decodedInvoice.Destination != nil {
		signature, err := sig.ToSignature()
		if err != nil {
			return nil, fmt.Errorf("unable to deserialize "+
				"signature: %v", err)
		}
		if !signature.Verify(hash, decodedInvoice.Destination) {
			return nil, fmt.Errorf("invalid invoice signature")
		}
	} else {
		headerByte := recoveryID + 27 + 4
		compactSign := append([]byte{headerByte}, sig.RawBytes()...)
		pubkey, _, err := ecdsa.RecoverCompact(compactSign, hash)
		if err != nil {
			return nil, err
		}
		decodedInvoice.Destination = pubkey
	}

clightining:

	if (!have_n) {
		struct pubkey k;
		if (!secp256k1_ecdsa_recover(secp256k1_ctx,
					     &k.pubkey,
					     &sig,
					     (const u8 *)&hash))
			return decode_fail(b11, fail,
					   "signature recovery failed");
		node_id_from_pubkey(&b11->receiver_id, &k);
	} else {
		struct pubkey k;
		/* n parsing checked this! */
		if (!pubkey_from_node_id(&k, &b11->receiver_id))
			abort();
		if (!secp256k1_ecdsa_verify(secp256k1_ctx, &b11->sig,
					    (const u8 *)&hash,
					    &k.pubkey))
			return decode_fail(b11, fail, "invalid signature");
	}

[t-bast]

Fair enough!

[Roasbeef]

Makes sense.

Before merge, we should also go through the test vectors to see if any of them are high-s, regenerating if so.

[erickcestari]

There are no BOLT-11 test vectors that cover signature validation using the n field. We should probably add some.

[real-or-random]

implementations would have to add extra checks in the recovery path to validate the S value before or after pubkey recovery, which makes things more complicated/redundant.

We could totally provide a function in libsecp256k1 to make that easy, but I still think that not insisting on low-s is the simplest thing to do here.

Some nits:

• The operation is called "pubkey recovery", not "signature recovery".
• The entire BOLT doesn't even mention the word ECDSA. Maybe this should be added for clarity.

[t-bast]

Thanks for the clarification!

[erickcestari]

I’ll also add a new test vector to cover public key recovery from a high-S signature.

I noticed that rust-lightning currently enforces low-S even when recovering a public key from a signature similar to lightning-kmp.

[t-bast]

ACK aa85c9d

[Roasbeef]

LGTM 🍦

[rustyrussell]

Ack, thanks!