Make converting ledger sync check more defensive #17819
Conversation
|
!ci-build-me |
cjjdespres
force-pushed
the
cjjdespres/make-sync-check-more-defensive
branch
from
b6d20f9 to
c59c699
Compare
|
!ci-build-me |
src/lib/merkle_ledger/database.ml
Outdated
| get mdb (Location.Account addr) |> Option.value_exn | ||
| get mdb (Location.Account addr) | ||
|
|
||
| let get_at_index_exn mdb index = get_at_index mdb index |> Option.value_exn |
There was a problem hiding this comment.
Nit: add ~message and ~here for Option.value_exn please? ~here should be propagated from outside.
| Ok (`Existed, location) | ||
|
|
||
| let iteri_untrusted t ~f = | ||
| match Account_location.last_location_address t with |
| get t (Location.Account addr) |> Option.value_exn | ||
| get t (Location.Account addr) | ||
|
|
||
| let get_at_index_exn t index = |
There was a problem hiding this comment.
Nit: add ~message and ~here for Option.value_exn please? ~here should be propagated from outside.
| assert_is_attached t ; | ||
| let num_accounts = num_accounts t in | ||
| Sequence.range ~stop:`exclusive 0 num_accounts | ||
| |> Sequence.iter ~f:(fun i -> f i (get_at_index t i)) |
There was a problem hiding this comment.
Can we do a simple for loop here? It seems more readable than this. I'll have to check the API of sequence to understand if this is written correctly.
There was a problem hiding this comment.
let range ?(stride = 1) ?(start = `inclusive) ?(stop = `exclusive) start_v stop_v =
let step =
match stop with
| `inclusive when stride >= 0 ->
fun i -> if i > stop_v then Done else Yield { value = i; state = i + stride }
| `inclusive ->
fun i -> if i < stop_v then Done else Yield { value = i; state = i + stride }
| `exclusive when stride >= 0 ->
fun i -> if i >= stop_v then Done else Yield { value = i; state = i + stride }
| `exclusive ->
fun i -> if i <= stop_v then Done else Yield { value = i; state = i + stride }
in
let init =
match start with
| `inclusive -> start_v
| `exclusive -> start_v + stride
in
unfold_step ~init ~f:step
;;it seems stop being exclusive is the default. Thought I'm looking at Base v0.17.2
|
!ci-build-me |
|
I've now implemented I agree with your |
|
Okay, could we at least add |
If a daemon is stopped at certain points while syncing a converting ledger to a network, the underlying databases can be left in an unexpected state: the num_accounts in the database will be set to a high value (because the daemon added a batch of accounts at high addresses) but not all accounts at lower indices will be present (because the daemon didn't get to filling them in yet). This will cause functions like iteri and get_at_index_exn to throw unexpected exceptions when they get to addresses that are missing accounts. A couple of new ledger functions have been added to account for this possibility, which are used in an updated ledger database sync check implementation. A few related unit tests have been added to the converting merkle tree, one of which does not pass without these changes being in place.
The ~here is set to the point of usage of Option.value_exn itself to conform with our code base's current practices, but we might want to have it passed in from the caller of these functions.
cjjdespres
force-pushed
the
cjjdespres/make-sync-check-more-defensive
branch
from
669e742 to
688e286
Compare
|
I added the one commit to add a |
|
!ci-build-me |
If a daemon is stopped at certain points while syncing a converting ledger to a network, the underlying databases can be left in an unexpected state: the num_accounts in the database will be set to a high value (because the daemon added a batch of accounts at high addresses) but not all accounts at lower indices will be present (because the daemon didn't get to filling them in yet). This will cause functions like iteri and get_at_index_exn to throw unexpected exceptions when they get to addresses that are missing accounts.
A couple of new ledger functions have been added to account for this possibility, which are used in an updated ledger database sync check implementation. The sync check ensure that for any index under
num_accounts, either both databases are missing an account at that index, or both databases have the same account at that index up to account conversion.A few related unit tests have been added to the converting merkle tree, one of which does not pass without these changes being in place.