Uh oh!
There was an error while loading. Please reload this page.
Use interior mutability in ChannelMonitor - #813
Conversation
jkczyz
commented
Mar 1, 2021
@TheBlueMatt Not sure if this is what you had in mind with moving |
Codecov Report
@@ Coverage Diff @@## main #813 +/- ##
==========================================
- Coverage 91.06% 91.01% -0.06%
==========================================
Files 48 48 Lines 25495 25562 +67 ==========================================
+ Hits 23218 23266 +48 - Misses 2277 2296 +19
Continue to review full report at Codecov.
|
TheBlueMatt
commented
Mar 1, 2021
Concept ACK. |
valentinewallace
commented
Mar 1, 2021
Concept ACK :) |
2d42d4d to
7aa17e6Compare| /// | ||
| /// panics if the given update is not the next update by update_id. | ||
| pub fn update_monitor<B: Deref, F: Deref, L: Deref>( | ||
| &self, |
There was a problem hiding this comment.
nit: I find rustfmt-style unwrapping like this rather hard to read, its something that my head thinks shouldn't be vertical but is (and I have to check it against the use which is horizontal), and suddenly I can't see as much context.
There was a problem hiding this comment.
I was resistant to this for awhile until dealing with extremely long signatures in lightning-block-sync. I think using this format for long signatures is a good convention because it makes the parameters easy to scan and provides a consistent rule for formatting.
Otherwise, parameters can be easily missed if they are bin packed. It may seem a little odd at first, but it doesn't take long to adjust to reading this format. And it has the advantage of clearly indicating a parameter's type. This otherwise becomes especially difficult for more elaborate types like tuples or those with more than one type parameter, as the additional commas make the parameter list harder to scan.
That said, I do prefer to bin pack arguments at the call sites as I've done throughout in cases where the line exceeds the 100-character limit. Here, there are no types so any advantage of breaking each argument across lines is lost. I think this is a good middle ground as the additional vertical spacing is limited to signatures.
There was a problem hiding this comment.
It is somewhere in between, indeed, and I see the value of it, but I'd argue that more things are missed in review due to missing context than scan errors. Sometimes just a few lines is needed, but sometimes even the next function up is important (or being able to fit the full parameter list while looking at a function). Sadly, different people have different terminal heights so there's not an easy fix.
| } | ||
| } | ||
| #[cfg(test)] |
There was a problem hiding this comment.
Hm, why are these functions test-only now?
Edit: oh, I guess that does make sense. In that case, maybe we could make the inner's corresponding methods private, instead of pub(crate)?
valentinewallace
left a comment
There was a problem hiding this comment.
This makes the public API so much nicer
ChainMonitor accesses a set of ChannelMonitors behind a single Mutex. As a result, update_channel operations cannot be parallelized. It also requires using a RefCell around a ChannelMonitor when implementing chain::Listen. Moving the Mutex into ChannelMonitor avoids these problems and aligns it better with other interfaces. Note, however, that get_funding_txo and get_outputs_to_watch now clone the underlying data rather than returning references.
Now that ChannelMonitor uses an internal Mutex to support interior mutability, ChainMonitor can use a RwLock to manage its ChannelMonitor map. This allows parallelization of update_channel operations since an exclusive lock only needs to be held when adding to the map in watch_channel.
The implementation of chain::Listen for ChannelMonitor required using a RefCell since its block_connected method required a mutable borrow. This is no longer the case since ChannelMonitor now uses interior mutability via a Mutex. So the RefCell is no longer needed.
7aa17e6 to
e8ea0d9Compare
ChainMonitorholds a map ofChannelMonitors guarded by a singleMutex. This prevents parallelizingupdate_channeloperations. This PR moves theMutexinsideChannelMonitorsuch that it uses interior mutability, making its interface in terms of&selfrather than&mut self. Thus,ChainMonitorcan use aRwLockfor its map instead of aMutex, allowing for parallel operations other than when updating the map inwatch_channel.Additionally, since
ChannelMonitornow uses interior mutability, it can implementchain::Listenwithout using aRefCell.Resolves#805