-
Notifications
You must be signed in to change notification settings - Fork 385
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Hide internals of ChainMonitor behind getter #1112
Hide internals of ChainMonitor behind getter #1112
Conversation
36d0dc7
to
3d95770
Compare
Heh, the test changes here turned up a bug in ChainMonitor, which is now fixed in a new commit. |
3d95770
to
81ccf92
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will continue to review but wanted to get out some comments about the code moves.
/// transaction and losing money. This is a risk because previous channel states | ||
/// are toxic, so it's important that whatever channel state is persisted is | ||
/// kept up-to-date. | ||
pub trait Persist<ChannelSigner: Sign> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we expect a watchtower implementation to persist ChannelMonitor
s independently of a ChainMonitor
and reuse a Persist
implementation to do so? That could be an argument of keeping this outside of chainmonitor.rs
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chatted about it offline, but I don't think so - I'd think they'd use chain::Watch
directly and never think about Persister
, especially after #1108.
/// An error enum representing a failure to persist a channel monitor update. | ||
#[derive(Clone, Copy, Debug, PartialEq)] | ||
pub enum ChannelMonitorUpdateErr { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... it's a little strange this error is related to persisting ChannelMonitor
updates but lives in a different location than both Persist
and ChannelMonitor
. Not sure if there is a compelling case for moving this and Persist
. At very least I'd think this should live next to Persist
, wherever that would be. chain::Watch
is just forwarding these from a Persist
implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, its more subtle - chain::Watch
is what users will use if they're not using a ChainMonitor
, I think. It passing through a Persist
is only for those who use ChainMonitor
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. We may want to rename ChannelMonitorUpdateErr
later given it overloads the "update" terminology. That is, this is return by watch_channel
which doesn't involve applying a ChannelMonitorUpdate
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, that'd make sense, I think.
81ccf92
to
8fae31a
Compare
Codecov Report
@@ Coverage Diff @@
## main #1112 +/- ##
=======================================
Coverage 90.60% 90.60%
=======================================
Files 66 66
Lines 34474 34474
=======================================
Hits 31235 31235
Misses 3239 3239 Continue to review full report at Codecov.
|
3c5aafd
to
8bb8758
Compare
struct MonitorHolder<ChannelSigner: Sign> { | ||
monitor: ChannelMonitor<ChannelSigner>, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is MonitorHolder
needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, its used in #1108 to add additional data per monitor in the same hashmap.
impl<ChannelSigner: Sign> Deref for LockedChannelMonitor<'_, ChannelSigner> { | ||
type Target = ChannelMonitor<ChannelSigner>; | ||
fn deref(&self) -> &ChannelMonitor<ChannelSigner> { | ||
&self.lock.get(&self.funding_txo).expect("Checked at construction").monitor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to hold a reference to the ChannelMonitor
instead of a doing a look-up each time a LockedChannelMonitor
is accessed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd love to, but I can't seem to find a way to do it without making the borrow checker sad - "just" including a reference doesn't work because it'd be a self-referential reference (which makes rust very very sad), and doing it with a cache of a reference doesn't seem to either without dropping Deref
and using a custom function with a different lifetime declaration (if that).
/// An error enum representing a failure to persist a channel monitor update. | ||
#[derive(Clone, Copy, Debug, PartialEq)] | ||
pub enum ChannelMonitorUpdateErr { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. We may want to rename ChannelMonitorUpdateErr
later given it overloads the "update" terminology. That is, this is return by watch_channel
which doesn't involve applying a ChannelMonitorUpdate
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to be 95% mechanical cascading changes/moves/test changes from my PoV. Let me know if there's anything specific to review for, but this is looking pretty good!
cbc5e05
to
c0d7887
Compare
Note the check_commits failure here should resolve once squashed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, seems like a strict improvement even without #1108! I'm ACK mod these comments
lightning/src/chain/chainmonitor.rs
Outdated
/// Gets the [`LockedChannelMonitor`] for a given funding outpoint, returning an `Err` if no | ||
/// such [`ChannelMonitor`] is currently being monitored for. | ||
/// | ||
/// Note that the result holds a mutex our monitor state, and should not be held indefinitely. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting -- is there ways in the bindings or in the native languages to mem::drop
these objects? Might it be a limitation for future language bindings? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! Even better, the Java mapping is "clever" enough to map things that start with "Lock" to something that acts exactly like a native lock in Java!
c0d7887
to
1a368a2
Compare
lightning/src/chain/chainmonitor.rs
Outdated
|
||
/// Gets the current list of [`ChannelMonitor`]s being monitored for, by their funding | ||
/// outpoint. | ||
pub fn list_current_monitors(&self) -> Vec<OutPoint> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this intended to be used? If monitors are added or removed after it is called, will that be a problem for the user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just kinda figured users may care about the list of channels being monitored to fully replace the exposed HashMap
we had before - I suppose users may want to walk their monitor list and re-persist them or something? Indeed, its race-y, but hopefully not too race-y, at least users have a function called when a new channel is added (and we currently don't remove monitors, but we'd also call a persist function when we do). Would you prefer if we return a LockedIterator
of some form?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong preference, though the LockedIterator
would indeed give a more consistent API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, yea, slightly, though its a bit awkward to use, we could deref as an iterator, but then its all rust-only, or we could deref as a vec, but then its kinda hard to make sure the user doesn't drop the lock before using the vec, putting us back where we started. On the flip side, I think users may be fine with it unlocked. eg if you're going to re-persist the world, you first set a "ok, also sending to host X" bit, then when a new monitor comes in, it goes to host X, even if that happens while you're walking the list of old monitors, so there wouldn't be a race.
/// Gets the [`LockedChannelMonitor`] for a given funding outpoint, returning an `Err` if no | ||
/// such [`ChannelMonitor`] is currently being monitored for. | ||
/// | ||
/// Note that the result holds a mutex over our monitor set, and should not be held |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would this be worth reflecting in a more overt manner? Maybe a method name like get_locked_monitor() or something indicative of the mutex?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, the struct is called LockedChannelMonitor
, though I suppose the method name could be too. Note that its not actually super duper critical - its only a read lock and we can do everything around updating monitors even while the read lock is held, only not add new monitors.
/// restore the channel to an operational state. | ||
/// | ||
/// Note that a given ChannelManager will *never* re-generate a given ChannelMonitorUpdate. If | ||
/// you return a TemporaryFailure you must ensure that it is written to disk safely before |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I return a failure, I must ensure that the update is written to disk, correct? The phrasing is a tad ambiguous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If its alright with you I'll leave this for #1106 which rewrites a ton of the text here.
9a57918
to
40d4387
Compare
Squashed without diff:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM after squash :)
ddea65c
to
4b66bea
Compare
Squashed without diff, will land after CI. cc @arik-so since you gave this one pass review.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still looks good post-squash.
Exposing a `RwLock<HashMap<>>` directly was always a bit strange, and in upcoming changes we'd like to change the internal datastructure in `ChainMonitor`. Further, the use of `RwLock` and `HashMap` meant we weren't able to expose the ChannelMonitors themselves to users in bindings, leaving a bindings/rust API gap. Thus, we take this opportunity go expose ChannelMonitors directly via a wrapper, hiding the internals of `ChainMonitor` behind getters. We also update tests to use the new API.
test_simple_monitor_permanent_update_fail and test_simple_monitor_temporary_update_fail both have a mode where they use either chain::Watch or persister to return errors. As we won't be doing any returns directly from the chain::Watch wrapper in a coming commit, the chain::Watch-return form of the test will no longer make sense.
Previously, if a Persister returned a TemporaryFailure error when we tried to persist a new channel, the ChainMonitor wouldn't track the new ChannelMonitor at all, generating a PermanentFailure later when the updating is restored. This fixes that by correctly storing the ChannelMonitor on TemporaryFailures, allowing later update restoration to happen normally. This is (indirectly) tested in the next commit where we use Persister to return all monitor-update errors.
As ChainMonitor will need to see those errors in a coming PR, we need to return errors via Persister so that our ChainMonitor chain::Watch implementation sees them.
4b66bea
to
1464671
Compare
Rebased on main, range diff is pretty trivial:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-ACKing post-squash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 1464671
These are a few of the cleanups/pre-refactors from #1108 split out to make #1108 a bit smaller. They mostly move a few things around and change the API of ChainMonitor to no longer expose its internal datastructures to the world.