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

ledger-tool: add warn log if capitalization changes during create-snapshot #35155

Merged
merged 3 commits into from
Feb 16, 2024

Conversation

CriesofCarrots
Copy link
Contributor

Problem

As brought up here, solana-ledger-tool create-snapshot operations that change the capitalization are intended for testing purposes and should be used with caution. The script silently calls Bank::set_capitalization() regardless of whether such operations are included. Node operators use this script on restarts on live clusters, like mainnet-beta, so it's worth taking extra care to ensure capitalization changes are intended.

Summary of Changes

Check capitalization before and after Bank::set_capitalization() and emit a warning if the value changes.

Copy link

codecov bot commented Feb 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (897adb2) 81.6% compared to head (bd1216f) 81.6%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #35155   +/-   ##
=======================================
  Coverage    81.6%    81.6%           
=======================================
  Files         833      833           
  Lines      224827   224827           
=======================================
+ Hits       183523   183562   +39     
+ Misses      41304    41265   -39     

Copy link
Contributor

@steviez steviez left a comment

Choose a reason for hiding this comment

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

Like the idea, I'm afraid the warning will get buried though. Maybe do the actual outputting at the end ?

Given the severity, I thought about suggesting adding a flag like --enable-capitalization-difference. However, I could see that something that just gets included in instructions / copy-pasted blindly. What do you think ?

@CriesofCarrots
Copy link
Contributor Author

Maybe do the actual outputting at the end ?

Maybe both? I agree it's easy to lose the WARNs in the sea of INFOs (that said, it would be nice if INFO wasn't so verbose).

Given the severity, I thought about suggesting adding a flag like --enable-capitalization-difference.

I like it. Blind c+p is always an issue, but at least we have a "should have known better" to point to, instead of a silent footgun. I'll add to this PR.

Copy link
Contributor

@steviez steviez left a comment

Choose a reason for hiding this comment

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

LGTM

@CriesofCarrots CriesofCarrots merged commit 9a69e3a into solana-labs:master Feb 16, 2024
35 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.

2 participants