remove circular references in channelmanager and channelmonitor #389
Conversation
src/chain/chaininterface.rs
Outdated
| /// watching for. | ||
| fn get_watched_txs(&self, block: &Block) -> (Vec<Transaction>, Vec<u32>); | ||
|
|
||
| /// Provides a way for users of the ChainWatchInterface to determine whether its watched |
There was a problem hiding this comment.
This doc doesn't say much of anything about the semantics of it, nor how to use it.
There was a problem hiding this comment.
Lmk if it's improved
valentinewallace
force-pushed
the
split-chain-watch-interface
branch
from
af84fae to
40d10ba
Compare
valentinewallace
changed the title
valentinewallace
force-pushed
the
split-chain-watch-interface
branch
2 times, most recently
from
ba4ad3f to
f363fdb
Compare
valentinewallace
force-pushed
the
split-chain-watch-interface
branch
8 times, most recently
from
b5c49d0 to
cfb0ffc
Compare
|
This also takes some big steps towards resolving #80, by, instead of forcing the user to use an interface that calls multiple block_connected impl's and then also exposing those block_connected impls, we just expose them and tell the user to call them. That said, I think this PR should be better at telling the user to call them :). Specifically, I think ChannelManager new and deserialize constructors and ManyChannelMonitor trait should have a paragraph indicating that these are the things you need to call block_connected/disconnected on. As a next PR, it would be kinda cool to replace the BlockNofiier listeners list with just two - a ChannelManager ref and a Option<M: ManyChannelMonitor + ChainListener> one. If you want this to be done with, I can merge it as-is, but some good next-steps :) |
This includes the purpose of this PR, which is to remove the circular reference created by ChainListeners self-adding themselves to their ChainWatchInterface's `listeners` field.
Adding this struct will allow us to remove the circular reference between ChainListeners and the ChainWatchInterface, because it separates out the responsibility of notifying listeners about new blocks from the responsibility of storing and retrieving watched transactions.
valentinewallace
force-pushed
the
split-chain-watch-interface
branch
from
cfb0ffc to
f715183
Compare
…erface Because filter_block takes a and returns a list of s , we must add a lifetime to the ChainWatchInterface, which bubbles up in a lot of places. These places include adding a lifetime to the Node struct, which causes a lot of rearranging tests so that variables don't go out of scope before the Node that owns them does.
Let me know if the paragraphs added need work.
The deserialization seems to have some instruction already. Is there something in particular missing as is? 🤔
That sounds good to me! |
Nope, I just forgot how that part worked :p. Glad they work the same now, though :) |
The goal of this PR is to remove these circular references, which should better enable a fix for #237. Reason being that references with nonstatic lifetimes and circular references don't play nicely with each other.
The way I accomplish removing the circular references is by splitting off part of the ChainWatchInterface into a BlockNotifier struct, which takes over the responsibility for notifying listeners when a new block comes in.
This is a pretty nontrivial refactor so I'm seeking feedback on the overall approach/idea. As is, it still needs some code + commit history cleanup 😅