Use interior mutability in ChannelMonitor #813
Conversation
|
@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.
|
|
Concept ACK. |
|
Concept ACK :) |
jkczyz
force-pushed
the
2021-02-channel-monitor-mutex
branch
from
2d42d4d to
7aa17e6
Compare
| /// | ||
| /// 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)?
There was a problem hiding this comment.
Ah, good point. Done!
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.
jkczyz
force-pushed
the
2021-02-channel-monitor-mutex
branch
from
7aa17e6 to
e8ea0d9
Compare
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