Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 12 additions & 8 deletions aliasmgr/aliasmgr.go
Original file line number Diff line number Diff line change
Expand Up @@ -242,11 +242,12 @@ func (m *Manager) populateMaps() error {
// AddLocalAlias adds a database mapping from the passed alias to the passed
// base SCID. The gossip boolean marks whether or not to create a mapping
// that the gossiper will use. It is set to false for the upgrade path where
// the feature-bit is toggled on and there are existing channels. The linkUpdate
// flag is used to signal whether this function should also trigger an update
// on the htlcswitch scid alias maps.
// the feature-bit is toggled on and there are existing channels. The base
// lookup flag indicates whether we want store a mapping from the alias to its
// base scid. The linkUpdate flag is used to signal whether this function should
// also trigger an update on the htlcswitch scid alias maps.
func (m *Manager) AddLocalAlias(alias, baseScid lnwire.ShortChannelID,
gossip, linkUpdate bool) error {
gossip, baseLookup, linkUpdate bool) error {

// We need to lock the manager for the whole duration of this method,
// except for the very last part where we call the link updater. In
Expand Down Expand Up @@ -302,8 +303,9 @@ func (m *Manager) AddLocalAlias(alias, baseScid lnwire.ShortChannelID,
// Update the aliasToBase and baseToSet maps.
m.baseToSet[baseScid] = append(m.baseToSet[baseScid], alias)

// Only store the gossiper map if gossip is true.
if gossip {
// Only store the gossiper map if gossip is true, or if the caller
// explicitly asked to store this reverse mapping.
if gossip || baseLookup {
Comment on lines +307 to +308
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i dont think we can do this cause now it will be stored in the m.aliasToBase map if gossip is false but baseLookup is true.. meaning it will be available in the map when the gossiper calls FindBaseByAlias which we explicitly dont want, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, now baseLookup flag alone is sufficient to store this in the map.

meaning it will be available in the map when the gossiper calls FindBaseByAlias which we explicitly dont want, right?

The gossiper will only look up scids from received channel updates, so whether we have more values there stored for reverse look-up doesn't seem like an issue as it's unlikely that the gossiper will look-up an scid that we manually created.

W.r.t conflicts in the alias scid range:
we will currently fail if the alias that we're trying to add via RPC already exists for another base scid, so it's even more unlikely that the gossiper will get a bad value out of the lookup, as we explicitly care about the uniqueness of the alias (see related code)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

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
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

m.aliasToBase[alias] = baseScid
}

Expand Down Expand Up @@ -342,7 +344,9 @@ func (m *Manager) GetAliases(
}

// FindBaseSCID finds the base SCID for a given alias. This is used in the
// gossiper to find the correct SCID to lookup in the graph database.
// gossiper to find the correct SCID to lookup in the graph database. It can
// also be used to look up the base for manual aliases that were added over the
// RPC.
func (m *Manager) FindBaseSCID(
alias lnwire.ShortChannelID) (lnwire.ShortChannelID, error) {

Expand Down Expand Up @@ -446,7 +450,7 @@ func (m *Manager) DeleteLocalAlias(alias,
}

// Finally, we'll delete the aliasToBase mapping from the Manager's
// cache (but this is only set if we gossip the alias).
// cache.
delete(m.aliasToBase, alias)

// We definitely need to unlock the Manager before calling the link
Expand Down
13 changes: 10 additions & 3 deletions aliasmgr/aliasmgr_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ func TestAliasLifecycle(t *testing.T) {
aliasScid2 := lnwire.NewShortChanIDFromInt(alias + 1)

// Add the first alias.
err = aliasStore.AddLocalAlias(aliasScid, baseScid, false, true)
err = aliasStore.AddLocalAlias(aliasScid, baseScid, false, false, true)
require.NoError(t, err)

// The link updater should be called.
Expand All @@ -124,7 +124,7 @@ func TestAliasLifecycle(t *testing.T) {
require.Contains(t, aliasList, aliasScid)

// Add the second alias.
err = aliasStore.AddLocalAlias(aliasScid2, baseScid, false, true)
err = aliasStore.AddLocalAlias(aliasScid2, baseScid, false, false, true)
require.NoError(t, err)

// The link updater should be called.
Expand Down Expand Up @@ -179,10 +179,17 @@ func TestAliasLifecycle(t *testing.T) {
require.Equal(t, StartingAlias, firstRequested)

// We now manually add the next alias from the range as a custom alias.
// This time we also set the base lookup flag, in order to be able to
// go from alias back to the base scid.
secondAlias := getNextScid(firstRequested)
err = aliasStore.AddLocalAlias(secondAlias, baseScid, false, true)
err = aliasStore.AddLocalAlias(secondAlias, baseScid, false, true, true)
require.NoError(t, err)

baseLookup, err := aliasStore.FindBaseSCID(secondAlias)
require.NoError(t, err)

require.Equal(t, baseScid, baseLookup)

// When we now request another alias from the allocation list, we expect
// the third one (tx position 2) to be returned.
thirdRequested, err := aliasStore.RequestAlias()
Expand Down
6 changes: 6 additions & 0 deletions docs/release-notes/release-notes-0.20.0.md
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,12 @@ a certain amount of msats.
- Added support for [P2TR Fallback Addresses](
https://github.com/lightningnetwork/lnd/pull/9975) in BOLT-11 invoices.

- A new experimental RPC endpoint
[XFindBaseLocalChanAlias](https://github.com/lightningnetwork/lnd/pull/10133)
was added for looking up the base scid for an scid alias. Aliases that were
manually created via the `XAddLocalChanAliases` endpoint will get lost on
restart.
Comment on lines +67 to +69
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


## Functional Enhancements
* [Add](https://github.com/lightningnetwork/lnd/pull/9677)
`ConfirmationsUntilActive` and `ConfirmationHeight` field to the
Expand Down
2 changes: 1 addition & 1 deletion funding/interfaces.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ type aliasHandler interface {

// AddLocalAlias persists an alias to an underlying alias store.
AddLocalAlias(lnwire.ShortChannelID, lnwire.ShortChannelID, bool,
bool) error
bool, bool) error

// GetAliases returns the set of aliases given the main SCID of a
// channel. This SCID will be an alias for zero-conf channels and will
Expand Down
7 changes: 4 additions & 3 deletions funding/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -1290,7 +1290,7 @@ func (f *Manager) advancePendingChannelState(channel *channeldb.OpenChannel,
// Persist the alias to the alias database.
baseScid := channel.ShortChannelID
err := f.cfg.AliasManager.AddLocalAlias(
baseScid, baseScid, true, false,
baseScid, baseScid, true, false, false,
)
if err != nil {
return fmt.Errorf("error adding local alias to "+
Expand Down Expand Up @@ -3397,7 +3397,7 @@ func (f *Manager) handleFundingConfirmation(
}

err = f.cfg.AliasManager.AddLocalAlias(
aliasScid, confChannel.shortChanID, true, false,
aliasScid, confChannel.shortChanID, true, false, false,
)
if err != nil {
return fmt.Errorf("unable to request alias: %w", err)
Expand Down Expand Up @@ -3565,7 +3565,7 @@ func (f *Manager) sendChannelReady(completeChan *channeldb.OpenChannel,

err = f.cfg.AliasManager.AddLocalAlias(
alias, completeChan.ShortChannelID,
false, false,
false, false, false,
)
if err != nil {
return err
Expand Down Expand Up @@ -4144,6 +4144,7 @@ func (f *Manager) handleChannelReady(peer lnpeer.Peer, //nolint:funlen

err = f.cfg.AliasManager.AddLocalAlias(
alias, channel.ShortChannelID, false, false,
false,
)
if err != nil {
log.Errorf("unable to add local alias: %v",
Expand Down
2 changes: 1 addition & 1 deletion funding/manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ func (m *mockAliasMgr) GetPeerAlias(lnwire.ChannelID) (lnwire.ShortChannelID,
}

func (m *mockAliasMgr) AddLocalAlias(lnwire.ShortChannelID,
lnwire.ShortChannelID, bool, bool) error {
lnwire.ShortChannelID, bool, bool, bool) error {

return nil
}
Expand Down
Loading
Loading