Enabling snapshot generation when loading from ledger tool#10064
Enabling snapshot generation when loading from ledger tool#10064roryharr merged 2 commits intoanza-xyz:masterfrom
Conversation
…eneration and loading is enabled
abda4d0 to
d852207
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10064 +/- ##
===========================================
+ Coverage 70.9% 82.6% +11.6%
===========================================
Files 711 844 +133
Lines 211507 316672 +105165
===========================================
+ Hits 150014 261611 +111597
+ Misses 61493 55061 -6432 🚀 New features to boost your workflow:
|
|
I'm not sure who should be on the review. Add anyone else if required. |
|
I think we should backport to v3.1. We could do v3.0 as well, but not sure if it is needed. The workaround is to only create full snapshots with ledger-tool, and not incremental snapshots. |
Replaced assert!(matches!()) with assert_eq()
|
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. |
Need a review from steve on this one? |
Yeah, I think that'd be good. Also for the backport-ness. |
steviez
left a comment
There was a problem hiding this comment.
I see Brooks already approved so LGTM.
One question - was there something in AccountsDb that changed that "broke" making incrementals with ledger-tool ? Or, has this always been broken with ledger-tool ? Thinking about if/how this could have been avoided
I think we should backport to v3.1.
Agreed - the change is focused/minimal and create-snapshot is "critical tooling" by virtue of being a part of the cluster restart workflow.
We could do v3.0 as well, but not sure if it is needed. The workaround is to only create full snapshots with ledger-tool, and not incremental snapshots.
Yeah, I think we hold on v3.0 for now. We'll get some runtime for this change in v3.1 with a planned testnet halt+restart. Assuming no issues, we'll be looking to take v3.1 to MNB very soon after so we hopefully won't need the v3.0 BP
SnapshotConfig is too convenient. Default impl, SnapshotUsage enum, convenience constructors. Config structs should force review of call sites whenever changed |
* When creating a snapshot from the ledger tool, ensure that snapshot generation and loading is enabled * Seperated out common code in ledger_utils Replaced assert!(matches!()) with assert_eq() (cherry picked from commit 22889ed)
…kport of #10064) (#10123) Enabling snapshot generation when loading from ledger tool (#10064) * When creating a snapshot from the ledger tool, ensure that snapshot generation and loading is enabled * Seperated out common code in ledger_utils Replaced assert!(matches!()) with assert_eq() (cherry picked from commit 22889ed) Co-authored-by: Rory Harris <rory.harris@anza.xyz>
…kport of anza-xyz#10064) (anza-xyz#10123) Enabling snapshot generation when loading from ledger tool (anza-xyz#10064) * When creating a snapshot from the ledger tool, ensure that snapshot generation and loading is enabled * Seperated out common code in ledger_utils Replaced assert!(matches!()) with assert_eq() (cherry picked from commit 22889ed) Co-authored-by: Rory Harris <rory.harris@anza.xyz>
Problem
#9496
When creating a snapshot with ledger-tool, snapshot generation is disabled, and the last full snapshot slot isn't set. This allows zero lamport single reference accounts can be removed improperly
Summary of Changes
Fixes #9496