-
Notifications
You must be signed in to change notification settings - Fork 419
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.
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
Conversation
`test_node_counter_consistency` can make gossip operations *really* slow. This makes it a pretty bad idea in a general node just running in debug mode. It also makes our `lightning-rapid-gossip-sync` real-world test painfully slow. Thus, here, we make `test_node_counter_consistency` only actually run in the `lightning`-crate tests, rather than always with `debug_assertions`.
I've assigned @joostjager as a reviewer! |
@@ -1786,7 +1786,7 @@ where | |||
} | |||
|
|||
fn test_node_counter_consistency(&self) { | |||
#[cfg(debug_assertions)] | |||
#[cfg(test)] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
Backported in #3932 |
v0.1.5 - Jul 16, 2025 - "Async Path Reduction" Performance Improvements ======================== * `NetworkGraph`'s expensive internal consistency checks have now been disabled in debug builds in addition to release builds (lightningdevkit#3687). Bug Fixes ========= * Pathfinding which results in a multi-path payment is now substantially smarter, using fewer paths and better optimizing fees and successes (lightningdevkit#3890). * A counterparty delaying claiming multiple HTLCs with different expiries can no longer cause our `ChannelMonitor` to continuously rebroadcast invalid transactions or RBF bump attempts (lightningdevkit#3923). * Reorgs can no longer cause us to fail to claim HTLCs after a counterparty delayed claiming multiple HTLCs with different expiries (lightningdevkit#3923). * Force-closing a channel while it is blocked on another channel's async `ChannelMonitorUpdate` can no longer lead to a panic (lightningdevkit#3858). * `ChannelMonitorUpdate`s can no longer be released to storage too early when doing async updates or on restart. This only impacts async `ChannelMonitorUpdate` persistence and can lead to loss of funds only in rare cases with `ChannelMonitorUpdate` persistence order inversions (lightningdevkit#3907). Security ======== 0.1.5 fixes a vulnerability which could allow a peer to overdraw their reserve value, potentially cutting into commitment transaction fees on channels with a low reserve. * Due to a bug in checking whether an HTLC is dust during acceptance, near-dust HTLCs were not counted towards the commitment transaction fee, but did eventually contribute to it when we built a commitment transaction. This can be used by a counterparty to overdraw their reserve value, or, for channels with a low reserve value, cut into the commitment transaction fee (lightningdevkit#3933).
test_node_counter_consistency
can make gossip operations really slow. This makes it a pretty bad idea in a general node just running in debug mode. It also makes ourlightning-rapid-gossip-sync
real-world test painfully slow.Thus, here, we make
test_node_counter_consistency
only actually run in thelightning
-crate tests, rather than always withdebug_assertions
.