Skip to content

Only run aggressive test_node_counter_consistency in tests #3687

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
Apr 1, 2025
Merged

Only run aggressive test_node_counter_consistency in tests #3687

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
2 changes: 1 addition & 1 deletion lightning/src/routing/gossip.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1786,7 +1786,7 @@ where
}

fn test_node_counter_consistency(&self) {
#[cfg(debug_assertions)]
#[cfg(test)]
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if you could still do some of this by only verifying consistency on writes for non-test runs by gating the call in read_only() with cfg test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not quite sure I understand what you're suggesting? The specific test that was mega-slow was one that read the real network graph out of an RGS snapshot and thus does a ton of network graph writes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah right, so the write verification is the problem. And only calling test_node_counter_consistency on reads instead then?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could, I suppose, tho it seems a bit weird? I mean we're testing internal consistency of various maps, which seems like something to do when we update those maps, rather than when we read those maps? I guess we could have some that run in debug-assertions but disable some of the checks in tests. It all seems like splitting hairs, though, just running in tests seems like it should suffice, imo.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. Indeed, if the writes wouldn't have been the bottleneck, just asserting on writes would make more sense.

{
let channels = self.channels.read().unwrap();
let nodes = self.nodes.read().unwrap();
Expand Down
Loading