[feature]: Make sure the BOLT11 invoice encoding follows the new spec changes

[ziggie1984]

Make sure LND is compliant with the new BOLT11 specification.

For more information see: lightning/bolts#1243

[MPins]

Hello @ziggie1984 and @saubyk,
I'd be happy to work on this issue! I’ve just started looking into #9591, and this one seems related, so I believe it makes sense to tackle them together.

[ziggie1984]

I would keep them separate. This issue was raised to make sure we are spec compliant, I am not sure whether something needs to be done codewise in the first place, can you check whether there are code changes necessary ?

[MPins]

Sure ... I`ll be checking if are code changes necessary and let you know.

[MPins]

Make sure LND is compliant with the new BOLT11 specification.

For more information see: lightning/bolts#1243

- MUST skip over `f` fields that use an unknown `version`.

✅ Somehow compliant. LND will skip any unknown SegWit version until it finds a valid fallback address. After that, it will skip any remaining fallback addresses without further validation.

lnd/zpay32/decode.go

Lines 367 to 376 in 2269859

	case fieldTypeF:
	if invoice.FallbackAddr != nil {
	// We skip the field if we have already seen a
	// supported one.
	continue
	}
	invoice.FallbackAddr, err = parseFallbackAddr(
	base32Data, net,
	)

lnd/zpay32/decode.go

Lines 520 to 577 in 2269859

	// parseFallbackAddr converts the data (encoded in base32) into a fallback
	// on-chain address.
	func parseFallbackAddr(data []byte, net *chaincfg.Params) (btcutil.Address, error) { // nolint:dupl
	// Checks if the data is empty or contains a version without an address.
	if len(data) < 2 {
	return nil, fmt.Errorf("empty fallback address field")
	}
	var addr btcutil.Address
	version := data[0]
	switch version {
	case 0:
	witness, err := bech32.ConvertBits(data[1:], 5, 8, false)
	if err != nil {
	return nil, err
	}
	switch len(witness) {
	case 20:
	addr, err = btcutil.NewAddressWitnessPubKeyHash(witness, net)
	case 32:
	addr, err = btcutil.NewAddressWitnessScriptHash(witness, net)
	default:
	return nil, fmt.Errorf("unknown witness program length %d",
	len(witness))
	}
	if err != nil {
	return nil, err
	}
	case 17:
	pubKeyHash, err := bech32.ConvertBits(data[1:], 5, 8, false)
	if err != nil {
	return nil, err
	}
	addr, err = btcutil.NewAddressPubKeyHash(pubKeyHash, net)
	if err != nil {
	return nil, err
	}
	case 18:
	scriptHash, err := bech32.ConvertBits(data[1:], 5, 8, false)
	if err != nil {
	return nil, err
	}
	addr, err = btcutil.NewAddressScriptHashFromHash(scriptHash, net)
	if err != nil {
	return nil, err
	}
	default:
	// Ignore unknown version.
	}
	return addr, nil
	}

☝️ 'f' tagged field

- MUST fail the payment if any mandatory field (`p`, `h`, `s`, `n`) does not have the correct length (52, 52, 52, 53).

❌ Not compliant: LND is not failing the payment.

Decode call parse before calling validate

lnd/zpay32/decode.go

Lines 143 to 147 in 2269859

	// Parse the timestamp and tagged fields, and fill the Invoice struct.
	if err := parseData(&decodedInvoice, invoiceData, net); err != nil {
	return nil, err
	}

lnd/zpay32/decode.go

Lines 201 to 206 in 2269859

	// Now that we have created the invoice, make sure it has the required
	// fields set.
	if err := validateInvoice(&decodedInvoice); err != nil {
	return nil, err
	}

And parse will skip p, h, s, n if they have not the correct length.

lnd/zpay32/decode.go

Lines 257 to 425 in 2269859

	// parseTaggedFields takes the base32 encoded tagged fields of the invoice, and
	// fills the Invoice struct accordingly.
	func parseTaggedFields(invoice *Invoice, fields []byte, net *chaincfg.Params) error {
	index := 0
	for len(fields)-index > 0 {
	// If there are less than 3 groups to read, there cannot be more
	// interesting information, as we need the type (1 group) and
	// length (2 groups).
	//
	// This means the last tagged field is broken.
	if len(fields)-index < 3 {
	return ErrBrokenTaggedField
	}
	typ := fields[index]
	dataLength, err := parseFieldDataLength(fields[index+1 : index+3])
	if err != nil {
	return err
	}
	// If we don't have enough field data left to read this length,
	// return error.
	if len(fields) < index+3+int(dataLength) {
	return ErrInvalidFieldLength
	}
	base32Data := fields[index+3 : index+3+int(dataLength)]
	// Advance the index in preparation for the next iteration.
	index += 3 + int(dataLength)
	switch typ {
	case fieldTypeP:
	if invoice.PaymentHash != nil {
	// We skip the field if we have already seen a
	// supported one.
	continue
	}
	invoice.PaymentHash, err = parse32Bytes(base32Data)
	case fieldTypeS:
	if invoice.PaymentAddr.IsSome() {
	// We skip the field if we have already seen a
	// supported one.
	continue
	}
	addr, err := parse32Bytes(base32Data)
	if err != nil {
	return err
	}
	if addr != nil {
	invoice.PaymentAddr = fn.Some(*addr)
	}
	case fieldTypeD:
	if invoice.Description != nil {
	// We skip the field if we have already seen a
	// supported one.
	continue
	}
	invoice.Description, err = parseDescription(base32Data)
	case fieldTypeM:
	if invoice.Metadata != nil {
	// We skip the field if we have already seen a
	// supported one.
	continue
	}
	invoice.Metadata, err = parseMetadata(base32Data)
	case fieldTypeN:
	if invoice.Destination != nil {
	// We skip the field if we have already seen a
	// supported one.
	continue
	}
	invoice.Destination, err = parseDestination(base32Data)
	case fieldTypeH:
	if invoice.DescriptionHash != nil {
	// We skip the field if we have already seen a
	// supported one.
	continue
	}
	invoice.DescriptionHash, err = parse32Bytes(base32Data)
	case fieldTypeX:
	if invoice.expiry != nil {
	// We skip the field if we have already seen a
	// supported one.
	continue
	}
	invoice.expiry, err = parseExpiry(base32Data)
	case fieldTypeC:
	if invoice.minFinalCLTVExpiry != nil {
	// We skip the field if we have already seen a
	// supported one.
	continue
	}
	invoice.minFinalCLTVExpiry, err =
	parseMinFinalCLTVExpiry(base32Data)
	case fieldTypeF:
	if invoice.FallbackAddr != nil {
	// We skip the field if we have already seen a
	// supported one.
	continue
	}
	invoice.FallbackAddr, err = parseFallbackAddr(
	base32Data, net,
	)
	case fieldTypeR:
	// An `r` field can be included in an invoice multiple
	// times, so we won't skip it if we have already seen
	// one.
	routeHint, err := parseRouteHint(base32Data)
	if err != nil {
	return err
	}
	invoice.RouteHints = append(
	invoice.RouteHints, routeHint,
	)
	case fieldType9:
	if invoice.Features != nil {
	// We skip the field if we have already seen a
	// supported one.
	continue
	}
	invoice.Features, err = parseFeatures(base32Data)
	case fieldTypeB:
	blindedPaymentPath, err := parseBlindedPaymentPath(
	base32Data,
	)
	if err != nil {
	return err
	}
	invoice.BlindedPaymentPaths = append(
	invoice.BlindedPaymentPaths, blindedPaymentPath,
	)
	default:
	// Ignore unknown type.
	}
	// Check if there was an error from parsing any of the tagged
	// fields and return it.
	if err != nil {
	return err
	}
	}
	return nil
	}

lnd/zpay32/decode.go

Lines 436 to 456 in 2269859

	// parse32Bytes converts a 256-bit value (encoded in base32) to *[32]byte. This
	// can be used for payment hashes, description hashes, payment addresses, etc.
	func parse32Bytes(data []byte) (*[32]byte, error) {
	var paymentHash [32]byte
	// As BOLT-11 states, a reader must skip over the 32-byte fields if
	// it does not have a length of 52, so avoid returning an error.
	if len(data) != hashBase32Len {
	return nil, nil
	}
	hash, err := bech32.ConvertBits(data, 5, 8, false)
	if err != nil {
	return nil, err
	}
	copy(paymentHash[:], hash)
	return &paymentHash, nil
	}

lnd/zpay32/decode.go

Lines 480 to 496 in 2269859

	// parseDestination converts the data (encoded in base32) into a 33-byte public
	// key of the payee node.
	func parseDestination(data []byte) (*btcec.PublicKey, error) {
	// As BOLT-11 states, a reader must skip over the destination field
	// if it does not have a length of 53, so avoid returning an error.
	if len(data) != pubKeyBase32Len {
	return nil, nil
	}
	base256Data, err := bech32.ConvertBits(data, 5, 8, false)
	if err != nil {
	return nil, err
	}
	return btcec.ParsePubKey(base256Data)
	}

Then validate would fail if 'p', 'h' and 'n' tagged field have not the correct length (but they were skipped).

lnd/zpay32/invoice.go

Lines 399 to 415 in 2269859

	// Check that we support the field lengths.
	if len(invoice.PaymentHash) != 32 {
	return fmt.Errorf("unsupported payment hash length: %d",
	len(invoice.PaymentHash))
	}
	if invoice.DescriptionHash != nil && len(invoice.DescriptionHash) != 32 {
	return fmt.Errorf("unsupported description hash length: %d",
	len(invoice.DescriptionHash))
	}
	if invoice.Destination != nil &&
	len(invoice.Destination.SerializeCompressed()) != 33 {
	return fmt.Errorf("unsupported pubkey length: %d",
	len(invoice.Destination.SerializeCompressed()))
	}

☝️Fail if 'p', 'h' and 'n' tagged field have not the correct length.

- MUST fail the payment if neither a 'd' field nor a 'h' field is present, or if both are present.

✅ Compliant

lnd/zpay32/invoice.go

Lines 391 to 398 in 2269859

	// Either Description or DescriptionHash must be set, not both.
	if invoice.Description != nil && invoice.DescriptionHash != nil {
	return fmt.Errorf("both description and description hash set")
	}
	if invoice.Description == nil && invoice.DescriptionHash == nil {
	return fmt.Errorf("neither description nor description hash set")
	}

[MPins]

@ziggie1984 and @saubyk IMO LND is not compliant as you can see above.

@saubyk I'll be happy to jump into this one.

This is also related to issue (#9700)

[ellemouton]

Thanks @MPins ! I've assigned it to you