Post-#2314 Cleanups #2765
Post-#2314 Cleanups #2765
Conversation
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2765 +/- ##
==========================================
- Coverage 88.64% 88.61% -0.03%
==========================================
Files 115 115
Lines 90257 90287 +30
Branches 90257 90287 +30
==========================================
+ Hits 80009 80011 +2
- Misses 7842 7872 +30
+ Partials 2406 2404 -2 ☔ View full report in Codecov by Sentry. |
| @@ -2399,13 +2413,11 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> { | |||
| Ok(()) | |||
| } | |||
|
|
|||
| pub(crate) fn provide_initial_counterparty_commitment_tx<L: Deref>( | |||
| fn provide_initial_counterparty_commitment_tx<L: Logger>( | |||
| &mut self, txid: Txid, htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Box<HTLCSource>>)>, | |||
| commitment_number: u64, their_per_commitment_point: PublicKey, feerate_per_kw: u32, | |||
| to_broadcaster_value: u64, to_countersignatory_value: u64, logger: &L | |||
There was a problem hiding this comment.
Should we enforce that these are WithChannelMonitor in the method signatures?
There was a problem hiding this comment.
Yea, fair, I guess I was thinking I'd do it later but that would be useless - we're already touching all the code here, might as well do it now.
| @@ -797,6 +796,7 @@ where C::Target: chain::Filter, | |||
| ChannelMonitorUpdateStatus::UnrecoverableError => { | |||
| // Take the monitors lock for writing so that we poison it and any future | |||
| // operations going forward fail immediately. | |||
| core::mem::drop(pending_monitor_updates); | |||
In order to ensure log lines generated by `ChannelMonitor` always have a counterparty and channel ID entry, this consistently wraps `Logger`s in a decorator in all `pub(X)` `ChannelMonitor` functions, removing `pub` markings on `ChannelMonitorImpl` methods that aren't actually publicly reachable anyway. This also lets us clean up the `Logger` types in various `ChannelMonitor` methods.
Now that `ChannelMonitor` is careful about wrapping `Logger`s at the edge, there's no need to use `WithChannelMonitor` in a few cases in `channel.rs` and one in `channelmanager.rs`.
df3ab2e added `WithContext` wrappers in most logs in `peer_handler.rs`, but due to long-term rebasing it missed a few, which we add here.
e21a500 cleaned up the error handling in `ChainMonitor::update_channel` a bit, but accidentally replaced the deliberate panic with a hang. This commit ensures we properly drop the monitors read lock before taking a write lock.
df3ab2e was rebased one too many times and ended up reverting some of the `log_bytes!()` removals around types which now implement `Display` in `ChannelManager`. This commit removes those, as well as one additoinal excess macro which slipped in somewhere else.
The `WithChannelMonitor` log decorator redundantly locks the `ChannelMonitor` inner mutex, which we fix here, as well as add a new constructor which avoids locking at all if an inner mutex lock is already readily available.
TheBlueMatt
force-pushed
the
2023-12-2314-cleanups-1
branch
from
23e93da to
4f0d5ed
Compare
|
Rewrote the first commit, so didn't really bother with a fixup commit. |
There was a problem hiding this comment.
Thank you so much for this great cleanup!
The PR looks fantastic, and the cleanups make a significant improvement. I had a smooth understanding of changes in commits 2 to 6, and your detailed commit message was immensely helpful.
I did face a bit of a puzzle with the first commit, particularly in understanding the reasoning behind the changes in chainmonitor.rs and onchaintx.rs. While I grasped the modifications in the ChannelManager file, a bit more context in the commit message or splitting these changes into a separate commit would make it even clearer.
Overall, great improvement, and I'm confident these small adjustments will make the PR even better!
|
Let's go ahead and merge. One reviewer should be fine as this PR largely addresses follow-ups from #2314. |
There may be one or two followup PRs, but this one is big enough by itself. One big commit at the top to make the monitor logging more robust and then a few small cleanups and one fix (which I'd like to land to avoid regressions).