[custom channels 2/5]: Extract PART2 from mega staging branch #9049
Conversation
|
Important Review skippedAuto reviews are limited to specific labels. Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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? TipsChatThere are 3 ways to chat with CodeRabbit:
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Pull reviewers statsStats of the last 30 days for lnd:
|
guggero
force-pushed
the
extract-part2-from-staging-branch
branch
from
70877f8
to
ac24f5b
Compare
guggero
force-pushed
the
extract-part2-from-staging-branch
branch
from
ac24f5b
to
63ed0ad
Compare
|
I addressed all TODOs, this is ready for full second round review. |
|
I did a first pass. I'll do another more detailed review next week. |
guggero
force-pushed
the
extract-part2-from-staging-branch
branch
from
63ed0ad
to
10cb424
Compare
lnrpc/routerrpc/router_server.go
Outdated
| // But we only add it, if it's a valid alias, as defined | ||
| // by the BOLT spec. | ||
| if !aliasmgr.IsAlias(aliasScid) { | ||
| return nil, fmt.Errorf("SCID alias %v is not "+ | ||
| "a valid alias", aliasScid) | ||
| } |
There was a problem hiding this comment.
Seems like this check should be done first to prevent needing to query the alias manager for the existence of aliases that are fundamentally impossible.
There was a problem hiding this comment.
Makes sense, yes. Even though we just query the alias manager once for all the aliases and cache the result, from the flow of things this should come first.
lnrpc/routerrpc/router.proto
Outdated
| to the channel peer via any message. Therefore, routing over such an alias | ||
| will only work if the peer also calls this same RPC on their end. If an |
There was a problem hiding this comment.
The comment may need an update concerning that it's only possible to route if both add the alias (only the forwarding node needs to). Is there a strong reason to prepend this with X? I think we could drop it as I can see this to be an important LSP functionality.
There was a problem hiding this comment.
You are correct, for just the routing part it's enough if only one side sets this. But for the custom channel functionality (especially the RFQ part where the custom data within an HTLC might reference such an SCID alias) the alias needs to be known to both parties to fully work.
We added the X because of the requirement of both nodes needing to call this individually and it not being negotiated over a custom p2p message between the peers. I think it's still the goal to eventually (not this PR) make it possible to communicate this over the wire, that's when we're going to remove the X.
| } | ||
|
|
||
| err = s.cfg.AliasMgr.AddLocalAlias( | ||
| aliasScid, baseScid, false, true, |
There was a problem hiding this comment.
Not sure I fully understand the gossip flag it seems like it makes it possible to retrieve data about the channel via rpc?
There was a problem hiding this comment.
That was pre-existing and I'm not 100% I understand it either.
The description reads:
The gossip boolean marks whether or not to create a mapping
that the gossiper will use.
So I assume it's only set to true if we'd ever expect gossip messages referencing such an alias? Not sure when that would be the case and when not.
With the new RPC calls that we are going to add in the next commits, it will be possible for users to add (local only, non-gossipped) SCID aliases for channels. Since those will be in the same range as the ones given out by RequestAlias, we need to make sure that when we generate a new one that it doesn't collide with an already existing one.
Because we restrict custom SCID aliases to be in a specific range, we export the range start and end values so a user of the RPCs we're going to add in the next commits can adjust their values to fit within the range.
Introduce `ResumeModified` action to resume standard behavior of a p2p message with optional modifications as specified by the client during interception.
This commit extends the forward HTLC intercept response with fields that can be used in conjunction with a `ResumeModified` action to modify the intercepted HTLC p2p message.
Implement an integration test where an HTLC is intercepted and the interception response modifies fields in the resultant p2p message.
With this commit we make sure the first hop custom records aren't lost on restart/resume of a payment, so we persist it as part of the PaymentCreationInfo struct.
guggero
force-pushed
the
extract-part2-from-staging-branch
branch
from
10cb424
to
74a559d
Compare
We might be trying to send an invoice amount that's greater than the size of the channel, but once you factor in the custom channel logic, an actual HTLC can be sent over the channel to pay that larger payment. As a result, we'll skip over this check if a have a custom HTLC.
guggero
force-pushed
the
extract-part2-from-staging-branch
branch
from
74a559d
to
427d41d
Compare
Extracts part 2 from #8960.