Adds --recalculate-accounts-lt-hash to leder-tool create-snapshot

[brooksprumo]

Problem

Creating a minimized snapshot with ledger-tool create-snapshot breaks firedancer's regression testing, because we currently recalculate the accounts lt hash. Since we recalculate the accounts lt hash, this causes the bank hashes to change. And since FD is testing against known/expected bank hashes, their tests fail.

See #7087 for more context.

Summary of Changes

Switches the default to not recalculate the accounts lt hash, and adds --recalculate-accounts-lt-hash to leder-tool create-snapshot when creating a minimized snapshot, which does recalculate the accounts lt hash.

Fixes #7087

Note, since this issue is in v2.3, I plan to backport as well.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.8%. Comparing base (1c9366c) to head (340a465).
⚠️ Report is 31 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##           master    #7305     +/-   ##
=========================================
- Coverage    82.8%    82.8%   -0.1%     
=========================================
  Files         801      801             
  Lines      363392   363410     +18     
=========================================
- Hits       300960   300954      -6     
- Misses      62432    62456     +24     

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[apfitzge]

should we add a test with false so it doesn't break?

[brooksprumo]

Yeah, I can add that. I didn't think it was very valuable, as then it's a "regular" snapshot load. Wdyt? Just for test coverage? Or should I check the accounts lt hash remains unchanged?

[apfitzge]

yeah think just check the lt hash remains unchanged if we have that option. and IS changed if the option is there?

[brooksprumo]

Done in 25fe7e2.

[apfitzge]

Kind of curious about default behavior here.
Is it better to have not recalculate by default? Seems that was effectively the original intention of minimized snapshots - make sure you get same result as network but just faster to replicate replay of slot range.

recaclulating it serves a different purpose - it's effectively creating a new self-contained snapshot to start from.

wdyt?

[brooksprumo]

Yeah... I guess it depends on what the main usage of minimized snapshots is then.

We could revert the PR that added the accounts lt hash recalculation. Note that this means loading the minimized snapshots will always fail the startup accounts verification, and the user must then pass a hidden cli flag to skip this startup verification.

With recalculation, then loading the minimized snapshot will succeed, but bank hashes will be different. Are there use cases where people want this? The minimized snapshot could still be used to process the transactions from the ledger, and basically everything other than the bank hash.

I dunno which one to prioritize though. Who's the right person to make this call?

[steviez]

With recalculation, then loading the minimized snapshot will succeed, but bank hashes will be different. Are there use cases where people want this? The minimized snapshot could still be used to process the transactions from the ledger, and basically everything other than the bank hash.

I'm not aware of any uses cases. Even if you are playing the "user" transactions properly, having an incorrect bank hash will lead to votes getting dropped in subsequent slots. I think Andrew's comment captures the intent of this feature:
"make sure you get same result as network but just faster to replicate replay of slot range."

We could revert the PR that added the accounts lt hash recalculation. Note that this means loading the minimized snapshots will always fail the startup accounts verification, and the user must then pass a hidden cli flag to skip this startup verification.

Is there any way to tell a minimized snapshot from a regular snapshot ? I don't believe there is, but figured I'd ask to make sure as this would make things easier

[brooksprumo]

I'm not aware of any uses cases. Even if you are playing the "user" transactions properly, having an incorrect bank hash will lead to votes getting dropped in subsequent slots. I think Andrew's comment captures the intent of this feature: "make sure you get same result as network but just faster to replicate replay of slot range."

Maybe this means we revert the change to alway recalculate then?

Is there any way to tell a minimized snapshot from a regular snapshot ? I don't believe there is, but figured I'd ask to make sure as this would make things easier

Not that I'm aware of, unfortunately. Otherwise we could programmatically skip the startup accounts verification.

[apfitzge]

My gut tells me if we revert we may find someone does want the forced recalculation case. Maybe we just reverse the CLI option you have here to do the recalc instead of skip it?

[brooksprumo]

Maybe we just reverse the CLI option you have here to do the recalc instead of skip it?

That works for me.

[brooksprumo]

Done. The CLI option has been reversed in 340a465.

[mergify]

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.