feat(accountsdb): Generate snapshots, fix UAF, improve bincode #179
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
InKryption
force-pushed
the
ink/generate-snapshot
branch
2 times, most recently
from
119b44e
to
3ccd3e1
Compare
InKryption
force-pushed
the
ink/generate-snapshot
branch
from
3ccd3e1
to
c7a8088
Compare
InKryption
changed the title
0xNineteen
requested changes
Jul 7, 2024
InKryption
changed the title
0xNineteen
requested changes
Jul 9, 2024
0xNineteen
reviewed
Jul 9, 2024
InKryption
force-pushed
the
ink/generate-snapshot
branch
2 times, most recently
from
4ca848b
to
fc07181
Compare
InKryption
force-pushed
the
ink/generate-snapshot
branch
2 times, most recently
from
f59b5a7
to
c6c3ab9
Compare
|
While writing up docs for snapshot generation and putting the semantics to written word, I realized some of the logic actually didn't make sense - I fixed it, realized some obvious improvements, and decided it made sense to bundle them up in this PR. |
0xNineteen
reviewed
Jul 10, 2024
There was a problem hiding this comment.
just some dead code and comments on what we need in future prs - otherwise lgtm after you address the other convos - also should be good to get @dnut approval since its a fairly large pr and we walked through it already
InKryption
force-pushed
the
ink/generate-snapshot
branch
from
b7191d9
to
8b9a468
Compare
0xNineteen
reviewed
Jul 11, 2024
InKryption
force-pushed
the
ink/generate-snapshot
branch
from
afbc94f
to
40daf4a
Compare
dnut
reviewed
Jul 16, 2024
The string returned by `@typeName` has no guaranteed nor reliable format, and is not technically guaranteed to be unique to all types. Using these specific helpers to identify arraylists and hashmaps is less brittle.
* add `arraylist.defaultArrayListUnmanagedOnEOFConfig` * Handle hashmap size overflow * Free hashmap key value pair on error * Error on duplicate hash map entries * Add `skip_write_fn` config predicate: kind of a duct-tape solution, but it'll serve us while we use this bincode model.
This commit brings an initial high level implementation for the generation of snapshots based off of currently derivable data in accountsdb. It also makes a number of changes to accommodate testing, and improve code robustness, namely a switch from byte slice paths to directory handles in related & auxiliary code. Also this specifically modifies loadAndVerifyAccountsFiles to take the task indexes in order to slice into the file map data, instead of the file name list. Another notable change is the fixup for the `largest_root_slot` field not actually being assigned the largest available root slot. There are also various other improvements and refactors.
* Make the deserialize allocator parameter non-optional * Move the free function after the write function * Add the optional & list namespaces * Add `readIntAsLength` * Remove `getSerializedSize` * Don't return error.EOF instead of error.EndOfStream * Reorganize struct type reading and writing * Make `write` use the type detection instead of `@typeName` * Add dedicated hash map config
* Pass fields directly instead of passing whole structs * Store a only a single AccountFileInfo per hashmap entry, whilst serializing and deserializing it as a slice. * Fix copy paste error: @"!bincode-config:incremental_snapshot_persistence" @"!bincode-config:snapshot_persistence"
* Renames `fields_file_map` to `file_info_map` * Assign `file_info_map.count()` to `n_account_files`
* Import `sig.utils.types.arrayListInfo`. * Clarify control flow. * Get rid of `MaybeHashMapConfig`. * Document the `skip_write_fn` predicate.
* Iterate over `file_info_map` instead of `file_map`, locking only account files which are definitely going to be written based on the former. * Concretize the fact that we're only properly supporting full snapshot generation at the moment, incremental snapshot generation is future work. * Replace various instances of `id: usize` with `id: FileId` and amend related usage sites. In `AccountFileInfo`, this is also amended to override the bincode field config to serialize and deserialize it as a `usize`.
InKryption
force-pushed
the
ink/generate-snapshot
branch
from
40daf4a
to
551eab3
Compare
dnut
approved these changes
Jul 16, 2024
0xNineteen
approved these changes
Jul 16, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Note: there are a number of auxiliary changes that were made in the course of this change set as a result of running into and needing to fix issues, mainly involving manual testing, and minor amendments, alongside some small refactors.
This implements the most fundamental building blocks for generating snap shot tar files, and sets up our infrastructure proper for then compressing said tar files into zstd archives. At present none of this is plugged in anywhere, it's just unit tested. As such, this code will still probably need to experience much evolution as we sculpt the architecture of consensus and any other related components.
Edit:
the scope of this PR increased from the initial goal of generating snapshots, to include indirect but auxiliary improvements to lay the infrastructure and fix things that will likely be in the scope of snapshot generation.
Also I fixed a UAF in
validator.