[1/3]: Preparatory work for Forwarding Blinded Routes

[carlaKC]

This PR is the first in a series to add the ability for LND to forward payments to blinded routes.

It contains a collection of prep work to get the codebase into shape for forwarding functionality:

• Add encoding/decoding and validation for encrypted_data that is provided for blinded forwards.
• Add blinding_point to update_add_htlc and persist across restarts.
• Small refactors to prepare for decoding onion payloads with blinding points.

Note: the ability to generate/receive payments via blinded routes is out of scope for these PRs.

[ellemouton]

Very nice & clean! First pass done. Basically LGTM . Main comment is re potentially using the new tlv.RecordT and tlv.OptionalRecordT types for the various new tlv fields.

[coderabbitai]

Important

Auto Review Skipped

Auto reviews are limited to the following labels: llm-review. Please add one of these labels to enable auto reviews.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository.

To trigger a single review, invoke the @coderabbitai review command.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

• X
• Mastodon
• Reddit
• LinkedIn

---

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>.
  • Generate unit-tests for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit tests for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai generate interesting stats about this repository and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @coderabbitai read src/utils.ts and generate unit tests.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• The JSON schema for the configuration file is available here.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

[carlaKC]

Just pushed rebase + small fixups, looking into moving TLVs over to generics!

[lightninglabs-deploy]

@bitromortac: review reminder
@carlaKC, remember to re-request review from reviewers when ready

[ellemouton]

Just following up on my previous comment:

I personally think it's fine for us to move ahead with the current structure of the new TLV fields. Better to stick to the current "standard" way and then leave the big re-hall of all TLV fields to another PR. Else we will need to reason about 2 different ways of doing the same thing.

Let me know if you agree with this @carlaKC - then i'll re-request myself here for final review which I will prio asap 🚀

[carlaKC]

I personally think it's fine for us to move ahead with the current structure of the new TLV fields. Better to stick to the current "standard" way and then leave the big re-hall of all TLV fields to another PR. Else we will need to reason about 2 different ways of doing the same thing.

Started working on switching over to generics here, and I agree that It may not be worth trying to make this jump now. From what I can tell generics work isn't totally there, so it's a bit patchwork adding it in this PR IMO.

Let me know if you agree with this @carlaKC - then i'll re-request myself here for final review which I will prio asap 🚀

I'll rebase then re-request you, thanks!!

[Roasbeef]

seems like we have so far only used the generics in extension TLVs (not native TLV structs like) so there aren't regular tlv.Stream utilities? Everything needs to be converted to ExtraOpaqueData and then packed. Not a lot of code, but is this how we want to handle things?

Nope, you can just serialize into a normal blob. Look at this commit where I encoded things into a normal blob: 784d236. It's just like before, you make a stream form the records, but now the attributes themselves are records.

while we have RecordT for primitive types, there don't seem to be the same for non-primitive TLVs? Hit this with feature vectors in this PR: we can't implement Record because features are encoded with different tlv types in different contexts, and there isn't a simple way to use generics without having implemented RecordProducer, so we still need boilerplate.

You can make it from anything that implements record producer:

lnd/tlv/record_type.go

Lines 34 to 44 in b0552da

	// NewRecordT creates a new RecordT type from a given RecordProducer type. This
	// is useful to wrap a given record in this utility type, which also serves as
	// an extra type annotation. The underlying type of the record is retained.
	func NewRecordT[T TlvType, K any, V RecordProducerT[K]](
	record K,
	) RecordT[T, K] {
	return RecordT[T, K]{
	Val: record,
	}
	}

. So for this, we'd have a single struct that wraps the feature vector to implement the Record() method, then that single struct can be re-used across any other attributes.

Adapting Option: handling optional records is quite clumsy? In rust, you can just if let Some(A) to access the inner value when it is present and remain in the scope of your current function. With the existing Unwrap utilities we're always using closures to do simple things like validateSome(A) if it's non-nil.

It's what we have right now, but doesn't at all appear to be a fundamental barrier. You can either revert back to using plain pointers, then be extra diligent about nil checks, use the existing utilities, or make your own along the way to mske things more egonomic. You can use WhenSome for when you want to run a chunk of logic only if the thing is there, the Map functions for when you want to have a return value, and the other hard checks if you want to force an error if something is non-nil UnwrapOrErr. The latter is pretty much the same amount of lines as a ptr != nil check.

In short, nothing you've mentioned appear to be a significant barrier. Particularly given that many of PRs and newly introduce sub-systems have started to use the new TLV records. I've pointed out areas where we can delete entire files, and also make nil checks more explicit, to avoid a dreaded HTLC of death panic error, or other DOS issues. I really don't see any reason for switching over in this PR. In the amount of back and forth, you likely could've just made the routine API switch. Compared to everything else in this PR, it's rather minor, and works to make TLV handling more uniform, and also less error prone (forgot to do a nil check for optional TLV, causing a panic).

[carlaKC]

In the amount of back and forth, you likely could've just made the routine API switch.

While I absolutely could have updated the PR to use generics without full confidence that it was the right decision as the author, I don't really think that's how we want to be writing code. I appreciate the time that you and @ellemouton have taken to point out where I've been wrong about my concerns, and have throughout this discussion made a good faith attempt to maintain a version that uses generics that I feel this comment conveniently handwaves away.

I still have my reservations about directly porting rust primitives over to golang implementations, but I recognize that I should have reviewed the PRs in question if I wanted a vote. I have switched the series over to use generics - thanks in advance to reviewers for the extra round / apologies for making it necessary.

[ellemouton]

Update to generics LGTM! Just left one suggestion that would reduce the number of lines changed a bit 👍

[Roasbeef]

This can be simplified to c.BlindingPoint.UnwrapOr(nil).

[carlaKC]

cannot use nil as tlv.RecordT[*tlv.tlvType0, *secp256k1.PublicKey] value in argument to htlc.BlindingPoint.UnwrapOr?

iiuc we need two layers of unwrapping here to get the underlying type in the record rather than the type in the option?

[Roasbeef]

Ah sure, had just glanced at that. Can also use WhenSomeV instead here to avoid needing to deref Val, but non-blocking ofc.

Philosophical point, but generally I like to review the option as sort of lazy evaluation, so you pipe it down all the way until you actually need to obtain the true value. Otherwise, by forcing evaluation sooner than needed, you just end up burdening the caller once again with required nil checks.

[Roasbeef]

LGTM 🦋

Nice work!

Left some non-blocking comments re slightly different wayts to use some of the new APIs. The other thing that jumped out is that we can get away with just writing/reading ExtraData in a few more places to not have to worry about ensuring that all the new HTLC TLVs are properly written/read from disk. One example is when we add the endorsement bit directly or indirectly to lnd, the logic now also needs to make sure that's passed along vs just passing the TLV blob then leaving it to the caller at the last moment to unpack things. A similar comment applies to some of the option usage as well.

[Roasbeef]

Directly returning something here, so can be written as:

err := fn.MapOptionZ(blindedData.Contraints, func(c ...) error {
})
if err != nil {
}

Non-blocking. Similar instances below.

[Roasbeef]

Can be written as:

blindingPoint := fn.MapOptionz(msg.BlindingPoint, func(b ..) []byte {
    return b.Val.SerializeCompressed()
})

Non-blocking.

[Roasbeef]

Won't this information already be stored in ExtraData if you just take the opaque bytes from the update add HTLC message? Since we code, but then keep the blob as is.

[Roasbeef]

Re above (and also related to future changes like storing the endorsement bit in an opaque manner), if we copied the extra records from the pay desc into this HTLC, then it's a more generic way to handle storing any future TLV data.

[carlaKC]

if we copied the extra records from the pay desc into this HTLC

def, but iirc from the original PR we wanted to be more intentional about what we store (ie, only things we care about) rather than including the full ExtraBytes and wasting space if people send us random junk along with the HTLC.

[Roasbeef]

WhenSomeV is useful here again as it gives you just *btcec.PublicKey in this case.

[Roasbeef]

Similar point here re just copying over the raw bytes so you don't need to be concerned about the record mapping at this state.

[Roasbeef]

If the pay desc just stores the record directly, then we also don't need to handle this shuffling.

[Roasbeef]

WhenSomeV