Skip to content

Add additional test coverage of NetworkGraph counters #3388

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 7, 2024
Merged

Add additional test coverage of NetworkGraph counters #3388

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 4 additions & 0 deletions lightning/src/routing/gossip.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2089,6 +2089,10 @@ where
};
}

core::mem::drop(nodes);
Copy link
Contributor

Choose a reason for hiding this comment

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

Mind adding a comment here that will explain our future-selves what this does?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What's not clear here? If you remove this many tests instantly fail with recursive lock errors :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, it's pretty clear in the context of this PR, but in a month for now it might not be as obvious why we manually drop here, etc. Would be great to just add a brief comment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why else would you mem::drop a lock? I'm not really a fan of adding comments if I don't see why someone would get confused...

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, doesn't matter too much, going ahead and landing this

core::mem::drop(channels);
self.test_node_counter_consistency();

Ok(())
}

Expand Down
Loading