Uh oh!
There was an error while loading. Please reload this page.
Fix two circular Arc references - #736
Conversation
I've assigned @tnull as a reviewer! |
58ab500 to
05d7a7dCompareTheBlueMatt
commented
Dec 31, 2025
Note that this is (obviously) blocked on landing the upstream LDK PR. |
tnull
commented
Jan 8, 2026
This needs a rebase now that lightningdevkit/rust-lightning#4294 and #743 landed. When you bump the LDK dependency to |
`LiquiditySource` takes a reference to our `PeerManager` but the `PeerManager` holds an indirect reference to the `LiquiditySource`. As a result, after our `Node` instance is `stop`ped and the `Node` `drop`ped, much of the node's memory will stick around, including the `NetworkGraph`. Here we fix this issue by using `Weak` pointers, though note that there is another issue caused by LDK's gossip validation API.
In added logic to use the HRN resolver from `bitcoin-payment-instructions`, we created a circular `Arc` reference - the `LDKOnionMessageDNSSECHrnResolver` is used as a handler for the `OnionMessenger` but we also set a post-queue-action which holds a reference to the `PeerManager`. As a result, after our `Node` instance is `stop`ped and the `Node` `drop`ped, much of the node's memory will stick around, including the `NetworkGraph`. Here we fix this issue by using `Weak` pointers.
05d7a7d to
150b470CompareLDK's gossip validation API basically forced us to have a circular `Arc` reference, leading to memory leaks after `drop`ping an instance of `Node`. This is fixed upstream in LDK PR #4294 which we update to here.
150b470 to
93c38bdCompareTheBlueMatt
commented
Jan 8, 2026
Cleaned up and rebased. Note that a second arc circular ref slipped in in the interim. IMO the first commit should be backported as it by itself fixes |
4e1df76 to
97a750fComparetnull
commented
Jan 8, 2026
Thanks, will take a look. CI failure is unrelated (see #748). |
tnull
left a comment
There was a problem hiding this comment.
Mostly looks good, two questions regarding the cycle tests commit.
Uh oh!
There was an error while loading. Please reload this page.
| }) | ||
| } | ||
| #[cfg(cycle_tests)] |
There was a problem hiding this comment.
Rather than leaking internals and adding all that boilerplate which only makes sense in the cycle_tests context, could we just add a LeakChecker object (probably living in src/leak_checker.rs) taking a number of weak references and holding it as the last field in Node. This LeakChecker could then check all counts are 0 in its drop implementation? Seems like that would a) encapsulate the checks more cleanly b) could also be easily used to check that more (most?) of the main object Arcs aren't leaking on Node::drop?
There was a problem hiding this comment.
Sure, yea, that's cleaner. I didn't bother putting it in a separate file cause its literally 10 lines of code without comments and that seemed kinda absurd. let me know if you prefer a file.
There was a problem hiding this comment.
Yeah, keeping it in the same file is fine for if you so prefer, but I'm confused why we need the TestNode still then? See #736 (comment)
b3462c8 to
56832fbCompare| Err(e) if e == NodeError::NotRunning => {}, | ||
| Err(e) => panic!("{e:?}"), | ||
| } | ||
| leak_checks.assert_no_leaks(); |
There was a problem hiding this comment.
I'm confused: why can we not do this in Node::drop and avoid all the TestNode-related changes now? If we add a _leak_checker field as the last field in the struct Node, Rust's drop order should ensure it's always dropped last, i.e., when all other references should be gone, no?
There was a problem hiding this comment.
Mmm, I hadn't considered that, nifty idea.
There was a problem hiding this comment.
Made this change, though sadly note that we still need/want TestNode cause we have to make sure that we stop the node before dropping it, which we don't always do in tests.
There was a problem hiding this comment.
cause we have to make sure that we stop the node before dropping it
Not sure I'm following. I guess explicitly stopping in tests might be preferable, but why do we have "to make sure we stop before drop?
In particular, why did you remove the Drop implementation from Node? I don't think we want to do that, as we need to try to do everything to cleanly shutdown, even if users don't explicitly call stop.
There was a problem hiding this comment.
Ooops, uhhhh, cause I'm blind :). I reverted the pile of TestNode changes to just leave the checker.
There was a problem hiding this comment.
Ooops, uhhhh, cause I'm blind :). I reverted the pile of
TestNodechanges to just leave the checker.
Cool, seems you might still need to push these changes though :)
tnull
left a comment
There was a problem hiding this comment.
LGTM, feel free to squash the fixups!
Due to two circular `Arc` references, after `stop`ping and `drop`ping the `Node` instance the bulk of ldk-node's memory (in the form of the `NetworkGraph`) would hang around. Here we add a test for this in our integration tests, checking if the `NetworkGraph` (as a proxy for other objects held in reference by the `PeerManager`) hangs around after `Node`s are `drop`ped.
340f7de to
6a32f36CompareTheBlueMatt
commented
Jan 12, 2026
Cool, squashed without further changes. |
Uh oh!
There was an error while loading. Please reload this page.
Based on lightningdevkit/rust-lightning#4294, basically, but the first fix should probably get backported to 0.7 as it fixes the issue for RGS ldk-node instances even though we'll have to wait for an LDK update to fix the issue for P2P gossip instances.