Skip to content
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

Improve block connection logging and filtered txids #2637

Merged
merged 1 commit into from
Dec 11, 2023

Conversation

Sharmalm
Copy link
Contributor

@Sharmalm Sharmalm commented Oct 2, 2023

this pr solves #2348.

@codecov-commenter
Copy link

codecov-commenter commented Oct 2, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (fcf47a4) 88.57% compared to head (f89d42c) 89.68%.
Report is 50 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2637      +/-   ##
==========================================
+ Coverage   88.57%   89.68%   +1.10%     
==========================================
  Files         115      115              
  Lines       90687    98529    +7842     
  Branches    90687    98529    +7842     
==========================================
+ Hits        80327    88364    +8037     
+ Misses       7962     7842     -120     
+ Partials     2398     2323      -75     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Sharmalm Sharmalm marked this pull request as ready for review October 2, 2023 16:11
lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved
lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved
@Sharmalm
Copy link
Contributor Author

Sharmalm commented Oct 15, 2023

I have updated the changes, please review it :)

lightning/src/chain/chainmonitor.rs Outdated Show resolved Hide resolved
lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved
lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved
lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved
lightning/src/chain/channelmonitor.rs Show resolved Hide resolved
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Sorry, somehow missed your previous push. Two more comments and I think this is good.

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved
lightning/src/chain/chainmonitor.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

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

Some nits but nothing blocking!

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved
lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved
lightning/src/chain/chainmonitor.rs Outdated Show resolved Hide resolved
@Sharmalm
Copy link
Contributor Author

Sharmalm commented Nov 2, 2023

if every looks good in this PR, then i'll rebase this commit also.
Thank you everyone for the guidance and help during this PR :)

@Sharmalm
Copy link
Contributor Author

I have been busy with my End term examination of my college , so i wasn't able to update this PR. Now i am active.

Copy link
Member

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

Hi @Sharmalm! Thank you for the PR update, and I hope your End Term Examinations went well!

I have a suggestion to enhance consistency in the PR. Given that we have explicitly defined "Display" for the OutPoint struct, I propose using it instead of (".. {}:{} ..", txid, index) for better clarity.

Here's the proposed diff:

                let lock = self.inner.lock().unwrap();
                filter.register_tx(&lock.get_funding_txo().0.txid, &lock.get_funding_txo().1);
                log_trace!(logger, 
-                       "Registering funding outpoint {}:{}", &lock.get_funding_txo().0.txid, &lock.get_funding_txo().0.index);
+                       "Registering funding outpoint {}", &lock.get_funding_txo().0);
                for (txid, outputs) in lock.get_outputs_to_watch().iter() {
                        for (index, script_pubkey) in outputs.iter() {
                                assert!(*index <= u16::max_value() as u32);
+                               let outpoint = OutPoint { txid: *txid, index: *index as u16 };
                                filter.register_output(WatchedOutput {
                                        block_hash: None,
-                                       outpoint: OutPoint { txid: *txid, index: *index as u16 },
+                                       outpoint: outpoint,
                                        script_pubkey: script_pubkey.clone(),
                                });
-                               log_trace!(logger, "Adding monitoring for spends of outpoint {}:{} to the filter", txid, index);
+                               log_trace!(logger, "Adding monitoring for spends of outpoint {} to the filter", outpoint);
                        }
                }
        }

@Sharmalm
Copy link
Contributor Author

Okay

@shaavan
Copy link
Member

shaavan commented Nov 27, 2023

Let's squash the commits together before going with the final ACK.
If you need help with git squash or rebasing, this guide will prove helpful!

https://www.freecodecamp.org/news/git-squash-commits/

@Sharmalm
Copy link
Contributor Author

So everything looks good?

@shaavan
Copy link
Member

shaavan commented Nov 27, 2023

Yep!

Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

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

LGTM after squash.

@TheBlueMatt
Copy link
Collaborator

Can you please update the commit to have a description of what its doing, rather than a list of previous commit titles? See https://cbea.ms/git-commit/ for suggested formats.

@Sharmalm
Copy link
Contributor Author

Sharmalm commented Dec 6, 2023

I have update the the description.

Copy link
Member

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

Just a quick note:

This commit is logging confirmed TXIDs and also logging when we add things to the filter.

The commit message is improved, but it still feels like it's missing something. How about adding a mention of implementing the Display trait?

Here's my suggestion:

"Implement the Display trait for Outpoint and utilize it in the codebase for monitoring outpoints. Additionally, add log tracing for best_block_update and confirmed transactions."

Thanks!

Copy link
Member

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

ACK

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved
@valentinewallace valentinewallace dismissed their stale review December 7, 2023 22:40

needs to use new logging wrappers

lightning/src/chain/chainmonitor.rs Outdated Show resolved Hide resolved
lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved
lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved
lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved
lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved
lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved
lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved
Implement the Display trait for Outpoint and utilize it in the codebase for monitoring outpoints.
Additionally, add log tracing for best_block_update and confirmed transactions.
solves lightningdevkit#2348
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Good enough.

@TheBlueMatt TheBlueMatt merged commit 4b35697 into lightningdevkit:main Dec 11, 2023
14 of 15 checks passed
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.

6 participants