-
Notifications
You must be signed in to change notification settings - Fork 406
Move events into ChannelMonitor from ManyChannelMonitor #520
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
Move events into ChannelMonitor from ManyChannelMonitor #520
Conversation
0df209b
to
9ca5f23
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.
ACK 9ca5f23, comments are just nits while skimming over the code
&Event::PendingHTLCsForwardable { time_forwardable: _ } => { | ||
5u8.write(writer)?; | ||
// We don't write the time_fordwardable out at all, as we presume when the user | ||
// deserializes us at least that much time has elapsed. |
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.
About the assumption than some time has elapsed, I' m not sure, some people where interested by some HA scheme where you synchronize in real-time state between different ChannelManager. Dunno if it makes sense right now with current locking of ChannelHolder but still something to have in mind..
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.
Right, certainly isn't a thing you can do now, but also no big deal - if the user did some kind of crazy serialize-to-disk thing and then ends up relaying pending HTLCs a few seconds early, not really any harm done.
@@ -940,6 +932,7 @@ impl<ChanSigner: ChannelKeys> PartialEq for ChannelMonitor<ChanSigner> { | |||
self.current_local_signed_commitment_tx != other.current_local_signed_commitment_tx || | |||
self.payment_preimages != other.payment_preimages || | |||
self.pending_htlcs_updated != other.pending_htlcs_updated || | |||
self.pending_events.len() != other.pending_events.len() || // We trust events to round-trip properly |
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.
Should be documented in ChannelMonitor struct than its Events are invariant between round-trip (contrary to ChannelManager ones and previous commit)?
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.
We should just fix it in ChannelManager and serialize them out, instead of adding a comment. I did add a comment to ChannelMonitor.
As noted in the docs, Events don't round-trip fully, but round-trip in a way that is useful for ChannelManagers, specifically some events don't make sense anymore after a restart.
This is the next step after "Move pending-HTLC-updated ChannelMonitor from ManyChannelMonitor", moving our events into ChannelMonitor as well and leaving only new-outputs-to-watch in the return value for ChannelMonitor::block_connected (which is fine as those are duplicatively tracked in the ChannelMonitor directly, so losing/replaying them is acceptable).
9ca5f23
to
9ff6f29
Compare
ACK 9ff6f29 |
Continuing the work started by #474, this moves Events into ChannelMonitors from ManyChannelMonitors as well (serializing them as well), making ChannelMonitors much much easier to work with, and much harder to screw up by losing events that were pending when you saved.