Add XFindBaseLocalChanAlias RPC

[GeorgeTsagk]

Description

This PR exposes a new RPC method XFindBaseLocalChanAlias which enhances the set of alias-related RPCs by allowing the user to look up the base scid for an alias that they added.

We also enhance the alias manager with an extra flag that explicitly indicates that we want the backwards mapping (baseScid -> aliasScid) to be stored. This flag is used when calling XAddLocalChanAlias

[gemini-code-assist]

Summary of Changes

Hello @GeorgeTsagk, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

I've introduced a new RPC endpoint, XFindBaseLocalChanAlias, to enhance the alias management capabilities. This RPC allows users to easily retrieve the base Short Channel ID (SCID) associated with a local channel alias they've added. To support this, I've updated the alias manager to explicitly store the reverse mapping from alias to base SCID when a new alias is added, ensuring that these lookups are possible.

Highlights

• New RPC Endpoint: Added a new experimental RPC, XFindBaseLocalChanAlias, enabling users to query the base SCID for a given local channel alias.
• Alias Manager Enhancement: Enhanced the AddLocalAlias function in the alias manager with a baseLookup flag to control the explicit storage of the alias-to-base SCID mapping.
• RPC Integration: Modified the XAddLocalChanAlias RPC to utilize this new baseLookup flag, ensuring that manually added aliases can be looked up using the new XFindBaseLocalChanAlias RPC.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

[lightninglabs-deploy]

@GeorgeTsagk, remember to re-request review from reviewers when ready

[GeorgeTsagk]

/gemini review

[GeorgeTsagk]

(previous run failed)

[gemini-code-assist]

Code Review

This pull request introduces a new RPC, XFindBaseLocalChanAlias, which allows users to look up the base SCID for a given alias. To support this, the alias manager's AddLocalAlias function is updated with a baseLookup flag to control the creation of the in-memory reverse mapping. The changes are logical and include corresponding tests. My feedback includes a few minor suggestions to improve comment clarity, fix a style issue, and correct a typo in the release notes.

[ellemouton]

is it the desired behaviour that this reverse lookup is not persisted? currently, all these mappings added will be lost on restart

[GeorgeTsagk]

When we add a local alias we also store a (kvdb) entry that maps the alias to its base

lnd/aliasmgr/aliasmgr.go

Line 297 in d821318

return aliasToBaseBucket.Put(aliasBytes[:], baseBytes[:])

Then on restart when we call the helper populateMaps() we read the entries from the map:

lnd/aliasmgr/aliasmgr.go

Line 178 in d821318

err = aliasToBaseBucket.ForEach(func(k, v []byte) error {

and populate aliasToBase accordingly for each entry:

lnd/aliasmgr/aliasmgr.go

Lines 225 to 234 in d821318

	for aliasSCID, baseSCID := range aliasMap {
	m.baseToSet[baseSCID] = append(m.baseToSet[baseSCID], aliasSCID)
	// Skip if baseSCID is in the baseConfMap.
	if _, ok := baseConfMap[baseSCID]; ok {
	continue
	}
	m.aliasToBase[aliasSCID] = baseSCID
	}

[GeorgeTsagk]

Ok now I realise that baseConfMap is causing issues here.

After looking over the baseConfMap its entries only affect whether a baseScid will get the aliasToBase values populated on restart:

• When we call DeleteSixConfs we mark the base scid in the conf bucket
• If an alias entry's base scid is in the conf bucket we skip adding it to aliasToBase on startup

If we make sure to also delete the alias entries from the kvdb aliasToBase bucket here

lnd/aliasmgr/aliasmgr.go

Lines 385 to 391 in d821318

	// Now that the database state has been updated, we'll delete all of
	// the aliasToBase mappings for this SCID.
	for alias, base := range m.aliasToBase {
	if base.ToUint64() == baseScid.ToUint64() {
	delete(m.aliasToBase, alias)
	}
	}

Then we can totally strip away the baseToConf bucket, as it won't be needed anymore:

• When a baseScid is marked as confirmed we'll purge all previous aliases both from memory and the bucket.
• This way on restart any conf'd baseScids will by default not populate any aliasToBase entries, as there won't be any left.
• We're now free to add any manual aliasToBase entries which will persist and be restored by default on startup.

Note: if an RPC user added custom aliases before six confs, then those would also be deleted by above approach. In this case:
a) Can make it super clear in the docs that custom aliases are persisted only post-confirmation.
b) Keep the conf bucket after all, only to prohibit RPC user from adding custom entries before the channel is confirmed.

I lean towards a) for the last note.

Also worth investigating how this affects interaction with other systems

[GeorgeTsagk]

I just pushed some commits that change the approach. We don't use the baseLookup flag, instead we simplify the aliasmgr to not need the baseLookup flag at all (also we get rid of the previously used gossip flag).

With the latest set of commits, we instead change the behavior of aliasmgr:

• All added aliases will populate aliasToBase map and also persist in the db
• On deletion we remove both the in-memory mappings and the db entries for aliases
• On restart we don't consult the base-conf bucket at all, we just load all values that are still in storage (because of previous bullet point we're now safe to do so)

One interesting call site was from brontide:
https://github.com/lightningnetwork/lnd/blob/master/peer/brontide.go#L1045-L1062

We explicitly used the gossip flag there to make sure we don't accept channel alias updates after the channel confirmation.

---

After reviewing this approach I realized it's very hard to reason about. Current unit & itest coverage seems happy but not sure if they capture all funding flow edge cases that can lead to buggy behavior -- will have to investigate more deeply.

Some alternative approaches, if this one is not deemed worthy:

• Keep the gossip flag to directly affect the in-memory population of aliasToBase and also skip persistence altogether
• Serve the persistence needs of XFindBaseLocalChanAlias (the original scope of this PR) with a workaround, like keeping direct track of them in a new kvdb bucket.

[GeorgeTsagk]

Reverted back to the non-persistent approach, turns out there's too many things to deal with to properly support persistence for manually added aliases. Added the note on the release doc as per your suggestion @ellemouton.

Will re-iterate in the future for persistence.

[ellemouton]

lgtm 👍

I think that warning in the release notes should also be mentioned in the code.
One other suggestion about turning the new option into a functional option since it is experimental & doesnt work across restarts.. that would also eliminate the need to update every call-site. Non-blocking though (but defs please add a warning comment at least)

[GeorgeTsagk]

lgtm 👍

I think that warning in the release notes should also be mentioned in the code. One other suggestion about turning the new option into a functional option since it is experimental & doesnt work across restarts.. that would also eliminate the need to update every call-site. Non-blocking though (but defs please add a warning comment at least)

Thanks for the extra recommendations. Applied all of them 👍

[Roasbeef]

LGTM 💥

[Roasbeef]

Needs a rebase!

Then should be good to land.