Fix two circular Arc references
#736
Conversation
|
I've assigned @tnull as a reviewer! |
TheBlueMatt
force-pushed
the
2025-12-circular-arc-refs
branch
from
58ab500 to
05d7a7d
Compare
|
Note that this is (obviously) blocked on landing the upstream LDK PR. |
|
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.
TheBlueMatt
force-pushed
the
2025-12-circular-arc-refs
branch
from
05d7a7d to
150b470
Compare
LDK'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.
TheBlueMatt
force-pushed
the
2025-12-circular-arc-refs
branch
from
150b470 to
93c38bd
Compare
|
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 |
TheBlueMatt
force-pushed
the
2025-12-circular-arc-refs
branch
3 times, most recently
from
1d9d3ba to
4e1df76
Compare
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.
TheBlueMatt
force-pushed
the
2025-12-circular-arc-refs
branch
from
4e1df76 to
97a750f
Compare
Thanks, will take a look. CI failure is unrelated (see #748). |
3 participants
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.