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

Fix/reduce downloader verbosity #5905

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

fdefelici
Copy link

Description

This fix reduce tenure downloader trace verbosity by removing info! log statement.

basically with this change info log lines like the following example will no longer be written:

INFO [1741330250.786256] [stackslib/src/net/download/nakamoto/tenure_downloader_set.rs:674] [ThreadId(5)] Downloader for tenure dd4aaf9c10bae038e324b74674db3d2710889b29 is finished

Applicable issues

Additional info (benefits, drawbacks, caveats)

Checklist

  • Test coverage for new or modified code paths
  • Changelog is updated
  • Required documentation changes (e.g., docs/rpc/openapi.yaml and rpc-endpoints.md for v2 endpoints, event-dispatcher.md for new events)
  • New clarity functions have corresponding PR in clarity-benchmarking repo
  • New integration test(s) added to bitcoin-tests.yml

@fdefelici fdefelici requested review from a team as code owners March 7, 2025 07:12
@obycode obycode self-requested a review March 7, 2025 15:32
@obycode obycode added this to the 3.1.0.0.8 milestone Mar 7, 2025
Copy link
Member

@jcnelson jcnelson left a comment

Choose a reason for hiding this comment

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

I think we want to instead find the underlying issue that's causing this to be verbose

@kantai kantai linked an issue Mar 7, 2025 that may be closed by this pull request
@kantai
Copy link
Contributor

kantai commented Mar 7, 2025

I think we want to instead find the underlying issue that's causing this to be verbose

See this issue (#5871)

Specifically, my understanding is that there's multiple reasons for the verbosity here: there may be downloader instances for the same tenure on multiple peers, and as the tenure grows with new blocks, a completed downloader will be reinstantiated. From a casual look at stacks-node logs, for the same consensus hash, this log line appears 34 times, does that convey any information for a node operator? Is this implying that the downloader logic is buggy?

@jcnelson
Copy link
Member

Per the call, we expect to see a particular tenure downloaded often if it's not yet confirmed by the Bitcoin chain (this is a known issue, and the downloader logic here is too naive to know not to do this). So, we'd only want to switch logging of unconfirmed tenure downloads completing to debug!. Confirmed downloaders should remain info!().

To check this, you'll need to add a field to NakamotoTenureDownloader to indicate whether or not the tenure it's fetching is unconfirmed. This flag would only be set in the call to NakamotoUnconfirmedTenureDownloader::make_highest_complete_tenure_downloader(), since this is what creates a NakamotoTenureDownloader for an unconfirmed tenure. The NakamotoTenureDownloaderSet would check this flag in order to determine whether or not to log as info!() or debug!().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Status: In Review
Development

Successfully merging this pull request may close these issues.

Downgrade log level for "Downloader for tenure {} is finished"
4 participants