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

Uses Display for error in VerifyAccountsHashInBackground's panic message #34219

Merged

Conversation

brooksprumo
Copy link
Contributor

Problem

If the initial accounts hash verification fails, the panic message prints out the error, but with Debug. This may not include useful information that Display would. Since this message is user/dev-facing, Display is more helpful.

Summary of Changes

Change the error to be printed with Display instead of Debug.

Also, change the panic text to match the logs when the bg process starts/stops:

info!("Initial background accounts hash verification has started");

Backports

I'd also like to backport this to v1.17, to aid in debugging any issues sooner.

@brooksprumo brooksprumo added the v1.17 PRs that should be backported to v1.17 label Nov 26, 2023
@brooksprumo brooksprumo self-assigned this Nov 26, 2023
Copy link
Contributor

mergify bot commented Nov 26, 2023

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

Copy link

codecov bot commented Nov 26, 2023

Codecov Report

Merging #34219 (4411d1d) into master (c6451e9) will decrease coverage by 0.1%.
The diff coverage is 100.0%.

Additional details and impacted files
@@            Coverage Diff            @@
##           master   #34219     +/-   ##
=========================================
- Coverage    81.9%    81.9%   -0.1%     
=========================================
  Files         819      819             
  Lines      219338   219338             
=========================================
- Hits       179695   179664     -31     
- Misses      39643    39674     +31     

@brooksprumo brooksprumo marked this pull request as ready for review November 26, 2023 02:18
Copy link
Contributor

@HaoranYi HaoranYi left a comment

Choose a reason for hiding this comment

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

lgtm

@brooksprumo brooksprumo merged commit deee5f8 into solana-labs:master Nov 27, 2023
33 checks passed
@brooksprumo brooksprumo deleted the bg-accts-hash/panic-result branch November 27, 2023 14:51
mergify bot pushed a commit that referenced this pull request Nov 27, 2023
brooksprumo added a commit that referenced this pull request Nov 27, 2023
…ic message (backport of #34219) (#34224)

Uses Display for error in VerifyAccountsHashInBackground's panic message (#34219)

(cherry picked from commit deee5f8)

Co-authored-by: Brooks <brooks@solana.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v1.17 PRs that should be backported to v1.17
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants