Skip to content

ChannelManager+Router++ Logger Arc --> Deref #622

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

Conversation

valentinewallace
Copy link
Contributor

This caused a bunch of cascading changes, including
passing loggers down to Channels in function calls
rather than having each Channel have a pointer to the
ChannelManager's Logger (which was a circular reference).
Other structs that the Channel had passed its Logger to also
had their loggers removed. Other newly unused Loggers were
also removed, especially when keeping them would've caused
a bunch of extra test changes to be necessary, e.g. with
the ChainWatchInterfaceUtil's Logger.

Sorry, as always these PRs are massive and hard to break up. But, the changes are almost all really mechanical.

@valentinewallace
Copy link
Contributor Author

Oop.. I'll rebase tmrw

@valentinewallace valentinewallace force-pushed the chanmgr-logger-arc-to-deref branch from 920f09b to e7eb342 Compare May 13, 2020 16:43
@valentinewallace
Copy link
Contributor Author

Rebased!

@codecov
Copy link

codecov bot commented May 13, 2020

Codecov Report

Merging #622 into master will decrease coverage by 0.04%.
The diff coverage is 92.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #622      +/-   ##
==========================================
- Coverage   91.26%   91.22%   -0.05%     
==========================================
  Files          35       35              
  Lines       20787    20675     -112     
==========================================
- Hits        18972    18861     -111     
+ Misses       1815     1814       -1     
Impacted Files Coverage Δ
lightning/src/util/macro_logger.rs 89.28% <ø> (ø)
lightning/src/ln/peer_handler.rs 58.25% <12.90%> (ø)
lightning/src/ln/channelmanager.rs 85.49% <91.22%> (-0.03%) ⬇️
lightning/src/ln/channel.rs 86.62% <93.81%> (+0.01%) ⬆️
lightning/src/chain/chaininterface.rs 91.89% <100.00%> (-0.05%) ⬇️
lightning/src/chain/keysinterface.rs 96.96% <100.00%> (-0.02%) ⬇️
lightning/src/ln/chanmon_update_fail_tests.rs 97.45% <100.00%> (ø)
lightning/src/ln/channelmonitor.rs 95.51% <100.00%> (-0.01%) ⬇️
lightning/src/ln/functional_test_utils.rs 94.67% <100.00%> (-0.16%) ⬇️
lightning/src/ln/functional_tests.rs 97.29% <100.00%> (+0.08%) ⬆️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 59fe300...2f2ef85. Read the comment docs.

@@ -1144,7 +1137,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
///
/// Note that it is still possible to hit these assertions in case we find a preimage on-chain
/// but then have a reorg which settles on an HTLC-failure on chain.
fn get_update_fulfill_htlc(&mut self, htlc_id_arg: u64, payment_preimage_arg: PaymentPreimage) -> Result<(Option<msgs::UpdateFulfillHTLC>, Option<ChannelMonitorUpdate>), ChannelError> {
fn get_update_fulfill_htlc<L: Deref>(&mut self, htlc_id_arg: u64, payment_preimage_arg: PaymentPreimage, logger: &L) -> Result<(Option<msgs::UpdateFulfillHTLC>, Option<ChannelMonitorUpdate>), ChannelError> where L::Target: Logger {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This (and a thousand other places) probably doesn't need to be a Deref - we pass in a reference to the thing, so might as well just dereference it at the callsite and pass in L: Logger.

Copy link
Contributor Author

@valentinewallace valentinewallace May 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, it looks like that causes this error: https://i.imgur.com/2wdqfaX.png And requiring Sized would seemingly be an issue for e.g. the fuzz crate's TestLogger, since it contains a pointer to a trait object?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, alright. Funny that rust can't figure out how to do it without the "&*", but, totally fine to leave it as-is if it doesnt work.

@TheBlueMatt
Copy link
Collaborator

Oh, can you do the same for PeerManager's Arc as well?

@valentinewallace valentinewallace force-pushed the chanmgr-logger-arc-to-deref branch 3 times, most recently from 4eca9ba to fc897de Compare May 17, 2020 16:27
This caused a bunch of cascading changes, including
passing loggers down to Channels in function calls
rather than having each Channel have a pointer to the
ChannelManager's Logger (which was a circular reference).
Other structs that the Channel had passed its Logger to also
had their loggers removed. Other newly unused Loggers were
also removed, especially when keeping them would've caused
a bunch of extra test changes to be necessary, e.g. with
the ChainWatchInterfaceUtil's Logger.
@valentinewallace valentinewallace force-pushed the chanmgr-logger-arc-to-deref branch from fc897de to 2f2ef85 Compare May 17, 2020 16:33
@valentinewallace valentinewallace force-pushed the chanmgr-logger-arc-to-deref branch from 2f2ef85 to 222f0cb Compare May 17, 2020 17:02
@valentinewallace
Copy link
Contributor Author

Oh, can you do the same for PeerManager's Arc as well?

Done! (build failing because the code coverage upload website seems to be down)

@TheBlueMatt TheBlueMatt merged commit d4ad57b into lightningdevkit:master May 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants