-
Notifications
You must be signed in to change notification settings - Fork 715
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
Fix DB serialization order for Ord types #4619
Conversation
For slots that are used in a db key: use Slot::to_bytes_key() and Slot::from_bytes_key() For integers, use to_be_bytes() and from_be_bytes() |
My part is done. |
(assumes keys are sorted in DB)
My independent analysis (I went through pos state): |
final state and ledger db done |
I checked every change made in https://github.com/massalabs/massa/pull/4620/files , this PR seems to take into account the same ser/deser. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #4619 +/- ##
=======================================
Coverage 71.78% 71.78%
=======================================
Files 291 291
Lines 51642 51642
=======================================
Hits 37069 37069
Misses 14573 14573 ☔ View full report in Codecov by Sentry. |
All those variables on slices are stressing me out a bit. fn buf_to_array_ctr<F: Fn(&[u8; N]) -> V, V, const N: usize>(
buf: &[u8],
ctr: F,
) -> Option<(&[u8], V)> {
Some((&buf[N..], ctr(&buf.get(..N)?.try_into().ok()?)))
} Then you can do: let (rest_key, slot) =
buf_to_array_ctr(rest_key, Slot::from_bytes_key).expect("error");
let (rest_key, cycle) =
buf_to_array_ctr(rest_key, |v| u64::from_be_bytes(*v)).expect("error"); |
Only the genesis files update remains. Waiting for external approval |
9505422
An error prone behaviour in our code is that when serializing numbers (or slots) for the keys of the DB, we use varint serializer. As a result, the ordering of such numbers is not garantied in the DB's ordering (for instance when the number is around a power of two).
This is e.g. related to the following issue and associated PR: #4101 and #4104
In this PR, we will replace every instance of such serialization (and deserialization).
TODO:
Potentially impacted files list. For each file, list the potentially impacted keys:
https://github.com/massalabs/massa/blob/main/massa-async-pool/src/pool.rs @Leo-Besancon
https://github.com/massalabs/massa/blob/main/massa-db-worker/src/massa_db.rs @AurelienFT
https://github.com/massalabs/massa/blob/main/massa-executed-ops/src/executed_ops.rs @AurelienFT => => No problem for me as keys contains only prefix + operation Id which doesn’t have a variant and are not meant to be sorted
https://github.com/massalabs/massa/blob/main/massa-executed-ops/src/executed_denunciations.rs @AurelienFT
https://github.com/massalabs/massa/blob/main/massa-final-state/src/final_state.rs @Eitu33
pos_state.create_new_cycle_from_last
: function from PoS, looks like a potential issue but haven’t looked all the way as I was not assigned to that filelines are:
L234
L303
L351
L380
pos_state.apply_changes_to_batch
: takes slot as input and uses its serialized version when extending deferred creditsside note: other apply_changes functions in finalize taking slot as input are only using it for pruning purposes
lines are:
L450
when checking db validity, iterates over the whole database and deserializes values to check validity, order should have no impact here
L556-688
deletes:
reset values using constant prefixes
L128-135
L848-858
delete a cycle info with prefix, prefix is built with cycle number
L224
L280
L398
https://github.com/massalabs/massa/blob/main/massa-ledger-worker/src/ledger_db.rs @Eitu33
key_serializer_db & key_deserializer_db usage:
takes care only of ledger keys, which isnt the problem here (version byte + addr serialize + key type)
side note: to see all of them you can just query “serializer_db” the file
iterators based on prefixes (again only using ledger key formatting)
L191
L463
L490 (test_export)
L526 (test_export)
delete_prefix with base prefix input (basically just a clear)
L214
https://github.com/massalabs/massa/blob/main/massa-module-cache/src/hd_cache.rs @Leo-Besancon
https://github.com/massalabs/massa/blob/main/massa-pos-exports/src/pos_final_state.rs @Leo-Besancon
https://github.com/massalabs/massa/blob/main/massa-versioning/src/versioning.rs @sydhds
Impacted keys:
OTHER FACTORS