Skip to content

Refactors logging for startup accounts verification#7327

Merged
brooksprumo merged 2 commits intoanza-xyz:masterfrom
brooksprumo:verify-accounts/logs
Aug 6, 2025
Merged

Refactors logging for startup accounts verification#7327
brooksprumo merged 2 commits intoanza-xyz:masterfrom
brooksprumo:verify-accounts/logs

Conversation

@brooksprumo
Copy link
Copy Markdown

@brooksprumo brooksprumo commented Aug 5, 2025

Problem

Startup accounts verification does logging in multiple places, and is also incomplete and inconsistent.

  • In verify_snapshot_bank(), we log that verification is happening in the background, even if it actually runs in the foreground.
  • If we do end up doing the verification in the foreground, there's no additional logging indicating as such
  • The logging for the background verification is inconsistent with other startup logging, esp w.r.t. the start and stop text plus the duration.

Summary of Changes

Unifies all the startup verification logging into verify_accounts_hash().

@brooksprumo brooksprumo self-assigned this Aug 5, 2025
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Aug 5, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.0%. Comparing base (3c32519) to head (69f5bb0).
⚠️ Report is 2669 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master    #7327     +/-   ##
=========================================
- Coverage    83.0%    83.0%   -0.1%     
=========================================
  Files         800      800             
  Lines      362384   362384             
=========================================
- Hits       300861   300820     -41     
- Misses      61523    61564     +41     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@brooksprumo brooksprumo force-pushed the verify-accounts/logs branch from 2d6a119 to 2641992 Compare August 5, 2025 22:34
@brooksprumo brooksprumo marked this pull request as ready for review August 5, 2025 22:36
@brooksprumo brooksprumo requested a review from roryharr August 5, 2025 22:36
Comment thread runtime/src/bank.rs
verified
)
} else {
info!("Verifying accounts... Skipped.");
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is this comment redundant or incorrect? Seems useful to know that it was skipped.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Before we had it because we always logged:

                info!("Verifying accounts...");

a few lines up, so it was needed to be clear. Now, we only log what we're actually doing. So the absence of any "verifying accounts" logs indicates it was skipped. Wdyt?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I find it can still be useful to know it was skipped. For example when searching through logs of multiple startup cycles trying to figure out why one was failing and another passed.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done. I've put the "skipped" log back in 69f5bb0.

Comment thread runtime/src/bank.rs
};
let is_ok = check_lt_hash(&expected_accounts_lt_hash, &calculated_accounts_lt_hash);
self.set_initial_accounts_hash_verification_completed();
info!(
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Just for consistency what about let total_time = start.elapsed();

I would guess logging the timing is not useful in this case?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I could add a variable, but it seemed unnecessary. We do log the timing here.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I mean in the metrics as above.

Copy link
Copy Markdown
Author

@brooksprumo brooksprumo Aug 6, 2025

Choose a reason for hiding this comment

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

Verifying accounts in the foreground will always be for testing/debugging usages, so I don't think we need to submit metrics in this case.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Makes sense!

@brooksprumo brooksprumo requested a review from roryharr August 5, 2025 22:56
@brooksprumo brooksprumo merged commit dbb0c6d into anza-xyz:master Aug 6, 2025
41 checks passed
@brooksprumo brooksprumo deleted the verify-accounts/logs branch August 6, 2025 16:02
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.

3 participants