Impl Clone and PartialEq for ChannelMonitor #2331
Conversation
benthecarman
force-pushed
the
clone-channel-mon
branch
from
3cfcd74 to
ee782a5
Compare
This gives people more freedom with the channel monitors. For Mutiny this would be nice for us to be able to create copies of them and pass aorund in memory without having to serialize until we actually want to.
benthecarman
force-pushed
the
clone-channel-mon
branch
from
ee782a5 to
e583826
Compare
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #2331 +/- ##
==========================================
- Coverage 90.34% 90.30% -0.05%
==========================================
Files 104 104
Lines 53392 53407 +15
Branches 53392 53407 +15
==========================================
- Hits 48237 48228 -9
- Misses 5155 5179 +24
☔ View full report in Codecov by Sentry. |
This should let you tell which is the more recent channel monitor easily. If they are for a different funding txo, then they will not be compared.
This allows you to get access to all the channel monitors without locking on each look-up.
benthecarman
force-pushed
the
clone-channel-mon
branch
from
e583826 to
7eb9aa8
Compare
benthecarman
changed the title
| /// Note that [`ChannelMonitor`]s are not removed when a channel is closed as they are always | ||
| /// monitoring for on-chain state resolutions. | ||
| pub fn get_monitors(&self) -> HashMap<OutPoint, ChannelMonitor<ChannelSigner>> { | ||
| self.monitors.read().unwrap().iter().map(|(outpoint, holder)| { |
There was a problem hiding this comment.
Hmm, what's the expected use-case here? I'm not convinced a full clone of each monitor in a HashMap is the right API - if a user wants to look up a monitor by the funding txo, they should use the existing get_monitor fn. If they're looking to serialize a copy of each monitor this will result in unnecessary clones and a hashmap just to iterate over it. Instead, we could either return an impl Iterator or a vec of the funding outpoints?
There was a problem hiding this comment.
there's already a list_monitors that gives a Vec<Outpoint> and get_mointor(outpoint) but these require doing a bunch of locks. this replaces so we can get them all in one lock. The idea for this is to be able to get all of our monitors so we can create a backup without having to iterate over each one ourself.
Doing an iterator might be better than the hashmap here
There was a problem hiding this comment.
Hmm, I think the repeated mutexes is probably faster than cloning the full set of monitors, at least if they're not contested :). Plus it avoids potential heap fragmentation. I'd definitely prefer an iterator.
There was a problem hiding this comment.
I don't think this can be an iterator because of the RwLock, maybe I'm doing something wrong but always getting returns a value referencing data owned by the current function error that seems to only be solved by doing an allocation
There was a problem hiding this comment.
Mmm, right, we can't do a straight iterator, we can get the list of keys and build an iterator over those, but we'd have to do map lookups for each key as a part of the next iteration (or in some iterator deref, ala LockedChannelMonitor, which could be adapted to hold a reference to the RwLockReadGuard). Honestly given monitors are pretty big that may well be more efficient, at least for your use-case of just iterating them all and serializing them.
| @@ -879,6 +886,18 @@ impl<Signer: WriteableEcdsaChannelSigner> PartialEq for ChannelMonitor<Signer> w | |||
| } | |||
| } | |||
|
|
|||
| impl<Signer: WriteableEcdsaChannelSigner> PartialOrd for ChannelMonitor<Signer> where Signer: PartialEq { | |||
| fn partial_cmp(&self, other: &Self) -> Option<cmp::Ordering> { | |||
There was a problem hiding this comment.
This is a super strange API to me - given Ord is generally used to insert into a set, I'd expect the Ord implementation to basically just be over the funding txos, not over the update_id and only defined for a specific monitor.
Instead, we could have a specific method that has a name that makes clear what the behavior here is.
There was a problem hiding this comment.
yeah I can see this being a bastardization of Ord, will change to a function instead
|
Any desire to pick this back up? |
|
Dont really need this one anymore, can work on it if it is still desired though |
|
Took the clone commit in #2448, the others will require some design thinking and if y'all don't need it we might as well just drop. |
Cloneshould give people more freedom with the channel monitors. For Mutiny this would be nice for us to be able to create copies of them and pass around in memory without having to serialize until we actually want to.PartialEqshould let you tell which is the more recent channel monitor easily. If they are for a different funding txo, then they will not be compared.Also added a function to ChainMonitor to let us get all the channel monitors without having to lock for each one of them