Add NodeAnnouncment Cache to the Gossiper

[ziggie1984]

While this PR started to enforce the tlv onion, we decided against the enforcement of assumed feature bits. During the implementation however I encountered some flaky tests where we would not add the node currently in our graph because we would receive the node announcement before the channel announcement which would lead us to drop the node announcement message. Now we precache them similar to channel update messages and replay them as soon as a new channel announcement is received. The cache is a LRU storage which makes sure we limit its capacity to prevent OOM attacks.

[ziggie1984]

/gemini review

[gemini-code-assist]

Code Review

This pull request primarily enforces that all hops in a route must support TLV-encoded onion payloads, which improves routing reliability by failing early for incompatible paths. The core logic is implemented in routing/pathfind.go. Additionally, there are significant improvements to the gossip handling mechanism in discovery/gossiper.go, which now caches and replays premature node announcements, enhancing graph sync robustness. The test suite has been updated accordingly to reflect these changes. My review includes a suggestion to fix a potential logic issue in blinded path feature handling and a minor style improvement.

[gemini-code-assist]

The logic for ensuring the TLVOnionPayloadOptional feature is set for blinded paths is flawed. If b.Features is non-nil and non-empty but lacks the TLVOnionPayloadOptional feature, this required bit won't be added. This can lead to creating invalid blinded routes.

The logic can be simplified to ensure the feature is always set:

	features := lnwire.EmptyFeatureVector()
	if b.Features != nil {
		features = b.Features.Clone()
	}
	features.Set(lnwire.TLVOnionPayloadOptional)

[gemini-code-assist]

According to the style guide, each case in a switch statement should be preceded by a brief comment explaining the condition. This case is missing that comment.¹

                // If the node is ignored because we don't have any of its
                // channels, we'll cache it for later processing.
		case graph.IsError(err, graph.ErrIgnored):

Style Guide References

Footnotes

1. The style guide recommends adding a brief comment to each case in a switch statement to explain the condition. (link) ↩

[ziggie1984]

Short summary:

• LND does only encode onion_packets using the tlv payload encoding for quite some time now, but still supports decoding also the legacy format.

• Now when enforcing this feature on the pathfinding side we need to make sure the the private nodes and also the blinded nodes for blinded payments always set this additional feature otherwise the pathfinding will fail.

• This enforcement revealed some flakes in our code where previously we wouldn't care if we did not have the nodes node_announcement now the pathfinding would fail. I had to introduce the caching of node_announcments which was especially a problem for the itests because when using private channels we send the channel_announcment and the node_announcment almost at the same time and there might be some race conditions that we do not receive them in order and would not persist the node_announcment which for private channels wouldn't be sent again. I think this fix has a positive side effect because I saw cases in the past where we did miss the node_announcment and this caching might be a solution, wdyt ?

Overall I tend to merge this PR because it favours more strict pathfinding and also graph consistency and moreover fix some flakes as well we saw previously not having the node_annoucment. However I just want to point out that this PR is not strictly needed for the payment series so happy to hear the thoughts of the reviews.

[ellemouton]

Im not sure this is the correct direction for deprecating a legacy feature bit. THe spec labels this bit as "Assumed" in bolt 9. So I think instead we can just stop checking for the bit completely and just assume that all nodes on any route support the new format?

Rather than now going and adding the bit everywhere where an empty feature vector would have sufficed (as this also makes testing trickier)

[ellemouton]

might make sense to have an lnwire.AssumedFeatures() which would include all the feature bits that the spec has marked as assumed? non-blocking

[ellemouton]

feels like this should be in its own PR? or at least have a commit message body? doesnt seem related to the TLV enforcement

[ellemouton]

Now that we need the node announcment to create a route because
we always verify if the node supports tlv payloads

i dont know if we should do this... i feel like we should lean more towards just assuming that all nodes support this format? Ie, i think when we deprecate a legacy encoding via a feature bit, we should in general be removing code & not adding extra checks etc?

I think for route hints, we should just assume that all nodes on that route support the TLV format

[ziggie1984]

Im not sure this is the correct direction for deprecating a legacy feature bit. THe spec labels this bit as "Assumed" in bolt 9. So I think instead we can just stop checking for the bit completely and just assume that all nodes on any route support the new format?

agree, it adds too much complexity without much benefit, I think we can still use the prematureNodeAnnouncment cache from this PR tho, which reveiled some flakes where we would not have a proper node_announcment set. So will drop all the commits except the one which introduced the cache.