Move unpacking related code to agave_snapshots #8428
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #8428 +/- ##
=========================================
- Coverage 83.2% 83.2% -0.1%
=========================================
Files 840 841 +1
Lines 367879 367882 +3
=========================================
- Hits 306108 306097 -11
- Misses 61771 61785 +14 🚀 New features to boost your workflow:
|
kskalski
force-pushed
the
ks/snapshots_untar_move
branch
from
08eb88d to
520af0d
Compare
kskalski
force-pushed
the
ks/snapshots_untar_move
branch
from
14cf5ca to
d4874bd
Compare
| log = { workspace = true } | ||
| lz4 = { workspace = true } | ||
| rand = { workspace = true } | ||
| solana-accounts-db = { workspace = true } |
There was a problem hiding this comment.
This is now necessary only to access file_io / buffered reader APIs, it should be possible to remove once we get a better place for those.
kskalski
requested review from
brooksprumo,
roryharr and
steviez
and removed request for
brooksprumo
| agave_snapshots::SnapshotInterval, | ||
| agave_snapshots::{ | ||
| hardened_unpack::{ | ||
| open_genesis_config, OpenGenesisConfigError, MAX_GENESIS_ARCHIVE_UNPACKED_SIZE, |
There was a problem hiding this comment.
What a mess MAX_GENESIS_ARCHIVE_UNPACKED_SIZE makes this!
Is hardened unpack even the right place for it to live?
I guess it started out as a hard limit on the genesis size, but then became a configurable parameter, but was left in the hardened_unpack function.
Should it be part of genesis_config instead? I'm not familiar with that part of the code at all, so my suggestion may be very off base.
There was a problem hiding this comment.
I think this is a kind of sanity check limit, but you are right, this is technically a global constraint that could as well be exposed as public constant in genesis_config...
Moving it there seems like a few steps, but I will prepare the change if that is desired, @steviez what do you think?
There was a problem hiding this comment.
hm, actually this file doesn't really need this constant - it's only used in tests and can be replaced by a more aggressive constant easily (also can add test that actually hits the limit)
so for now I will extract it to a place closer to where it's actually used
There was a problem hiding this comment.
Haven't had the chance to review this yet, but happened to see #8489; will try to chime in tomorrow
Can you add a link back the first PR? Sounds like this is intended to be an ongoing refactor over time to move everything. |
4 participants
Problem
Code for unpacking snapshots is spread across solana-accounts-db and solana-runtime, especially the use of
tarcrate's objects and creating IO reader/writers spans those two crates.Summary of Changes
Move unpacking code to agave-snapshots
(this is one step in factoring out snapshot-related code to a new crate, started in #8382)