Batch merkle_path lookups in Sparse_ledger #14528
Conversation
|
!ci-build-me |
|
!ci-nightly-me |
|
|
1 similar comment
|
|
|
!ci-build-me |
|
!ci-nightly-me |
eb3db50 to
2fbe1c5
Compare
15a4df4 to
b9c0b92
Compare
| @@ -700,6 +700,58 @@ module Make (Inputs : Inputs_intf) : | |||
| List.map2_exn dependency_dirs dependency_hashes ~f:(fun dir hash -> | |||
| Direction.map dir ~left:(`Left hash) ~right:(`Right hash) ) | |||
|
|
|||
| let merkle_path_batch mdb locations = | |||
There was a problem hiding this comment.
This function seems over-complicated. If we just want to batch the get of hashes, I think we should keep the original structure of merkle_path function. I would suggest change the definition of the function like following:
let merkle_path_batch mdb locations =
let locations =
List.map locations ~f:(fun location ->
if Location.is_account location then
Location.Hash (Location.to_path_exn location)
else (
assert (Location.is_hash location) ;
location ) )
in
let list_of_dependencies = List.map locations ~f:(Location.merkle_path_dependencies_exn) in
let dependency_locs = List.concat list_of_dependencies |> List.map ~f:fst in
let dependency_hashes = get_hash_batch mdb dependency_locs in
let rec go list_of_dependencies hashes acc =
match list_of_dependencies with
| [] -> acc
| deps :: rest_of_deps ->
let locs, dirs = List.unzip deps in
let hashes, rest_of_hashes = List.(split_n hashes (length locs)) in
let path =
List.map2_exn dirs hashes ~f:(fun dir hash ->
Direction.map dir ~left:(`Left hash) ~right:(`Right hash) )
in
go rest_of_deps rest_of_hashes (path :: acc)
in
go list_of_dependencies dependency_hashes []
|> List.rev
There was a problem hiding this comment.
@ghost-not-in-the-shell could you check the version I have in #14594
I also wanted to refactor it but did that at a higher layer of PR chain to avoid conflicts
There was a problem hiding this comment.
But your version is even easier to read
There was a problem hiding this comment.
Do you mind if I use your code at PR #14594 and keep this one as it is?
There was a problem hiding this comment.
In fact version in that PR becomes a bit more general because of wide paths. Anyway, I suggest having a discussion on final version there as here is only an intermediate step and overall this function as here seems to be correct.
2fbe1c5 to
daa60aa
Compare
b9c0b92 to
07af60b
Compare
daa60aa to
4271394
Compare
07af60b to
f644690
Compare
f644690 to
5147f71
Compare
|
!ci-build-me |
| loop locations loc_acc dir_acc (0 :: length :: length_acc) | ||
| else | ||
| let sibling = Location.sibling k in | ||
| let sibling_dir = |
There was a problem hiding this comment.
NIT: rename to something other than sibling_dir. This can be done a layer above this in the PR chain given we are doing the larger refactor there.
| , length :: lengths | ||
| , acc_hd :: acc_tl ) -> | ||
| let dir = | ||
| Direction.map direction ~left:(`Left hash) ~right:(`Right hash) |
There was a problem hiding this comment.
Shouldn't these be flipped? It appears from the function above that we are returning the directions we traverse, and then returning the locations to the sibling hashes. So I would think we want to flip the directions here in order to build the proper merkle path.
There was a problem hiding this comment.
Merkle path convention is that in each element there is a direction to the node itself (from its parent) and hash of sibling.
Because direction = Location.last_direction (Location.to_path_exn k), Direction.map direction ~left:(Left hash) ~right:(Right hash) returns direction to the node itself with hash of a sibling, following the convention.
Checklist: