aliasmgr: Allow persisting manually added alias scids #10411
Conversation
Previously we were able to manually add scid aliases for channels but that would slightly misbehave especially around persistence and restarts. By default when a channel gets confirmed it won't get any of its aliases reloaded in memory. We change the existing base-lookup flag to also signal that an alias may be forcefully persisted and reloaded to the in-memory maps when the alias manager starts up. We achieve this by appending an extra byte to all the alias storage entries. Previously we'd have 8 bytes, and now we extend the entry to have 9 bytes. For old entries, the x00 byte is assumed which signals non-persistency. Any new entries will always have 9 bytes stored with the last byte serving as the signal for persistence.
Summary of ChangesHello @GeorgeTsagk, I'm Gemini Code Assist1! 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! This pull request enhances the alias manager functionality by introducing the ability to persist manually added channel aliases. Previously, such aliases would be removed once a channel reached six confirmations, leading to unexpected behavior for users. By adding a persistence flag to the alias storage and modifying the alias management logic, this change ensures that aliases explicitly marked for persistence remain accessible, improving the reliability and usability of manual alias management, especially for RPC-driven operations. Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a persistence flag for SCID aliases, allowing manually added aliases to survive channel confirmations. The implementation is well-executed, ensuring backward compatibility with the old data format by extending the alias storage from 8 to 9 bytes to include the flag. The changes are accompanied by a comprehensive set of tests that cover the new persistence logic, backward compatibility, and deletion of persistent aliases. The code is clear and logically sound. I have one suggestion regarding a potential performance improvement in DeleteSixConfs to enhance scalability, which involves avoiding a full database scan.
| return aliasToBaseBucket.ForEach(func(k, v []byte) error { | ||
| // Check if this entry maps to our baseScid. | ||
| entryBase := lnwire.NewShortChanIDFromInt( | ||
| byteOrder.Uint64(v[:8]), | ||
| ) | ||
| if entryBase.ToUint64() != baseScid.ToUint64() { | ||
| return nil | ||
| } | ||
|
|
||
| // Check if persistent flag is set. | ||
| if len(v) > 8 && (v[8]&AliasFlagPersistent) != 0 { | ||
| aliasScid := lnwire.NewShortChanIDFromInt( | ||
| byteOrder.Uint64(k), | ||
| ) | ||
| persistentAliases[aliasScid] = struct{}{} | ||
| } | ||
|
|
||
| return nil | ||
| }) |
There was a problem hiding this comment.
The use of aliasToBaseBucket.ForEach here introduces a full scan of the aliasBucket. While this is consistent with the existing database design which optimizes for writes, it could lead to performance issues on nodes with a very large number of aliases, as DeleteSixConfs holds a write lock on the database and a mutex on the manager during this operation.
A potential improvement could be to introduce a new secondary index (a new kvdb bucket) that maps baseScid to its persistent aliases (baseScid -> []persistentAliases). This would make lookups in DeleteSixConfs much faster (O(log N) instead of O(N)), at the cost of slightly more complexity when adding or deleting persistent aliases (requiring a read-modify-write on the new bucket).
Given that DeleteSixConfs is called for every confirmed channel, while manual alias management is likely less frequent, this trade-off might be worthwhile for long-term scalability.
1 participant
Description
Previously, all aliases were removed from the aliasToBase map when a channel reached 6 confirmations via
DeleteSixConfs, making them unfindable viaFindBaseSCID. This was problematic for manually-added aliases (e.g., via RPC) that users expected to persist. This PR adds a 1-byte flag field to the alias storage format, extending entries from 8 bytes (base SCID) to 9 bytes (base SCID + flag). This flag simply marks the persistence intention for that alias entry.Now when the user manually adds a local channel alias they may signal the intention to persist the alias by re-using the optional argument
WithBaseLookup(the only call site at the moment is the RPC endpointXAddLocalChanAliases.Tests have been updated to cover the behavior of old & new entries under the new format with the persistence flag.