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

Take circular GossipVerifier reference by Arc #3432

Merged

Conversation

tnull
Copy link
Contributor

@tnull tnull commented Nov 28, 2024

Closes #3369.

Our current architecture requires GossipVerifier's type signature to include a circular reference to itself. Previously, we directly gave that as Self, which however did not work when setting it dynamically under all circumstances. Here we take Self by Arc mitigate this issue.

This change was proposed by @TheBlueMatt in #3369 (comment), I have yet to confirm it works as expected myself.

Our current architecture requires `GossipVerifier`'s type signature to
include a circular reference to itself. Previously, we directly gave
that as `Self`, which however did not work when setting it dynamically
under all circumstances. Here we take `Self` by `Arc` mitigate this
issue.
@tnull tnull added this to the 0.1 milestone Nov 28, 2024
@tnull tnull requested a review from TheBlueMatt November 28, 2024 11:06
@TheBlueMatt
Copy link
Collaborator

After all the back-and-forth on that issue I'm kinda of the opinion that we should do #3369 (comment) instead

@tnull
Copy link
Contributor Author

tnull commented Dec 3, 2024

After all the back-and-forth on that issue I'm kinda of the opinion that we should do #3369 (comment) instead

Hmm, I still kind of dislike this from an architectural standpoint. FWIW, some users recently also discovered that in a remote-storage world, it makes a lot of sense to keep the network graph in a separate (potentially ephemeral) local cache, not it the main store. The message queue probably won't get persisted at all of course, but I fear that if we start walking down that line of putting more and more things into the NetworkGraph struct we might get to a point where the lines get blurry here and we find ourselves in the need to persist something that is not entirely re-syncable from the gossip network. IMO, that is just one of the reason why maintaining NetworkGraph as a pure data model for the graph would make sense.

But maybe I'm still misunderstanding what you're proposing?

@TheBlueMatt
Copy link
Collaborator

No, I mean I agree, its definitely not ideal to make NetworkGraph publicly look like just a data model but then secretly stuff a message queue in there. But, IMO, the alternative (that we have now) is much worse - a circular reference (the type being circular is the least of the issues, downstream users also have to manage actually handling a circular reference which is quite annoying to deal with) isn't really a great alternative.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Discussed it further offline. I'd like to do something different, but for 0.1 we can just land this as-is.

@TheBlueMatt TheBlueMatt merged commit 239d10a into lightningdevkit:main Dec 5, 2024
16 of 19 checks passed
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.

Drop reference to Self in GossipVerifier::new
2 participants