Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix two small topology issues #5286

Merged
merged 2 commits into from
May 25, 2022

Conversation

cdecker
Copy link
Member

@cdecker cdecker commented May 24, 2022

I found these while working on zeroconf and took me a while to track down:

  • We were considering the maximum number of HTLCs on the wrong direction, i.e. we were trying to go from A->B but were considering B->A. This is usually not an issue as long as we have channel_updates from both endpoints, but if that's not the case we'd be unable to use either direction (if A->B is missing/disabled we'd discard the channel right away, if B->A is missing we'd consider the channel, but then read that the maximum number of HTLCs is 0, thus indirectly discarding it too)
  • We were requiring both directions to be enabled, likely as a shorthand for a node dying and only the peer disabling their direction. This is unnecessary as a dying node would get isolated by all of its peers, thus we couldn't enter that node, and we don't require a way to leave from that node (which is the direction that the failing node would have failed to disable). This is actually quite a common occurrence in zeroconf, since the channels have different aliases depending on who assigned it, and so we end up with two half channels, instead of one full channel.

Changelog-Fixed: topology: Under some circumstances we were considering the limits on the wrong direction for a channel
This likely lead to a number of false errors when attempting to
route. We deemed a channel to be unusable as soon as either direction
isn't usable. This is bad since it excludes not only zeroconf
channels (which have different scids for the two directions), but it
also excludes any channel that we haven't seen an update from
yet. This was likely introduced when attemting to exclude nodes that
haven't sent a disable, but their peer has, but this is not necessary
as the unresponsive node would be marked as isolated by all its peers,
so we don't need to artificially mark a channel direction as disabled
when really we can't even enter the node to traverse the channel in
that direction.

Changelog-Fixed: routing: Fixed an issue where we would exclude the entire channel if either direction was disabled, or we hadn't seen an update yet.
@cdecker cdecker added this to the v0.12 milestone May 24, 2022
Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

ACK 85b3efa

I think that !dir and dir make the magic here :) good catch

@rustyrussell rustyrussell merged commit 74ddc15 into ElementsProject:master May 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants