Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix two bugs related to write notifications in masks #1794

Merged
merged 9 commits into from Mar 6, 2019
Merged

Fix two bugs related to write notifications in masks #1794

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7834a0a
maskable_merkle_tree: make sure to notify children about batch writes
Mar 5, 2019
5a141f3
masking_merkle_tree: rename some private funcs to self_
Mar 5, 2019
616049a
masking_merkle_tree: only unmask an account if it becomes identical
Mar 5, 2019
aea0c99
add Database.set assertion that would have prevented my folly
Mar 5, 2019
7510ac5
batch_notify_children ->batch_notify_mask_children
Mar 5, 2019
2fed13a
Update test_mask.ml
Mar 5, 2019
6a910ca
Update test_mask.ml
Mar 6, 2019
4e53a9b
remove that assertion for now
Mar 6, 2019
dd1ecf5
Merge branch 'master' into 'fix/notify-parent-bug'
Mar 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 15 additions & 0 deletions src/lib/merkle_ledger_tests/test_mask.ml
View file
Open in desktop
Expand Up @@ -697,6 +697,21 @@ module Make (Test : Test_intf) = struct
~message:"All accounts are accessible from m2"
~expect:(Some a) (Mask.Attached.get m2 loc) )
| _ -> failwith "unexpected" )

let%test_unit "setting an account in the parent doesn't remove the masked \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should the different balances be mentioned?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I've clarified the string a bit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's some leftover text in the string

copy" =
Test.with_instances (fun maskable mask ->
let attached_mask = Maskable.register_mask maskable mask in
let k = Key.gen_keys 1 |> List.hd_exn in
let acct1 = Account.create k (Balance.of_int 10) in
let loc =
snd (Mask.Attached.get_or_create_account_exn attached_mask k acct1)
in
let acct2 = Account.create k (Balance.of_int 5) in
Maskable.set maskable loc acct2 ;
[%test_result: Account.t]
~message:"account in mask should be unchanged" ~expect:acct1
(Mask.Attached.get attached_mask loc |> Option.value_exn) )
end

module type Depth_S = sig
Expand Down
20 changes: 20 additions & 0 deletions src/lib/merkle_mask/maskable_merkle_tree.ml
View file
Open in desktop
Expand Up @@ -117,4 +117,24 @@ struct
in
ignore (unregister_mask_exn parent t_as_mask) ;
List.iter dangling_masks ~f:(fun m -> ignore (register_mask parent m))

let batch_notify_children t accounts =
Copy link
Member

@psteckler psteckler Mar 5, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've noticed people, including me, sometimes confuse which is parent and which is child.

Can we name this batch_notify_mask_children ?

match Uuid.Table.find registered_masks (get_uuid t) with
| None -> ()
| Some masks ->
List.iter masks ~f:(fun mask ->
List.iter accounts ~f:(fun account ->
Mask.Attached.parent_set_notify mask account ) )

let set_batch t locations_and_accounts =
Base.set_batch t locations_and_accounts ;
batch_notify_children t (List.map locations_and_accounts ~f:snd)

let set_batch_accounts t addresses_and_accounts =
Base.set_batch_accounts t addresses_and_accounts ;
batch_notify_children t (List.map addresses_and_accounts ~f:snd)

let set_all_accounts_rooted_at_exn t address accounts =
Base.set_all_accounts_rooted_at_exn t address accounts ;
batch_notify_children t accounts
end
63 changes: 30 additions & 33 deletions src/lib/merkle_mask/masking_merkle_tree.ml
View file
Open in desktop
Expand Up @@ -125,47 +125,47 @@ struct
let get_uuid t = assert_is_attached t ; t.uuid

(* don't rely on a particular implementation *)
let find_hash t address =
let self_find_hash t address =
Copy link
Member

@psteckler psteckler Mar 5, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, these name changes help make it clear that these operations work just on the mask tables.

assert_is_attached t ;
Addr.Table.find t.hash_tbl address

let set_hash t address hash =
let self_set_hash t address hash =
assert_is_attached t ;
Addr.Table.set t.hash_tbl ~key:address ~data:hash

let set_inner_hash_at_addr_exn t address hash =
assert_is_attached t ;
assert (Addr.depth address <= Base.depth) ;
set_hash t address hash
self_set_hash t address hash

(* don't rely on a particular implementation *)
let find_location t public_key =
let self_find_location t public_key =
assert_is_attached t ;
Key.Table.find t.location_tbl public_key

let set_location t public_key location =
let self_set_location t public_key location =
assert_is_attached t ;
Key.Table.set t.location_tbl ~key:public_key ~data:location

(* don't rely on a particular implementation *)
let find_account t location =
let self_find_account t location =
assert_is_attached t ;
Location.Table.find t.account_tbl location

let find_all_accounts t =
let self_find_all_accounts t =
assert_is_attached t ;
Location.Table.data t.account_tbl

let set_account t location account =
let self_set_account t location account =
assert_is_attached t ;
Location.Table.set t.account_tbl ~key:location ~data:account ;
set_location t (Account.public_key account) location
self_set_location t (Account.public_key account) location

(* a read does a lookup in the account_tbl; if that fails, delegate to
parent *)
let get t location =
assert_is_attached t ;
match find_account t location with
match self_find_account t location with
| Some account -> Some account
| None -> Base.get (get_parent t) location

Expand All @@ -179,7 +179,7 @@ struct
(* first element in the path contains hash at sibling of address *)
let curr_element = List.hd_exn path in
let merkle_node_address = Addr.sibling address in
let mask_hash = find_hash t merkle_node_address in
let mask_hash = self_find_hash t merkle_node_address in
let parent_hash =
match curr_element with `Left h | `Right h -> h
in
Expand Down Expand Up @@ -240,14 +240,14 @@ struct
(* use mask Merkle root, if it exists, else get from parent *)
let merkle_root t =
assert_is_attached t ;
match find_hash t (Addr.root ()) with
match self_find_hash t (Addr.root ()) with
| Some hash -> hash
| None -> Base.merkle_root (get_parent t)

let remove_account_and_update_hashes t location =
assert_is_attached t ;
(* remove account and key from tables *)
let account = Option.value_exn (find_account t location) in
let account = Option.value_exn (self_find_account t location) in
Location.Table.remove t.account_tbl location ;
(* TODO : use stack database to save unused location, which can be used
when allocating a location *)
Expand All @@ -267,13 +267,13 @@ struct
account_hash
in
List.iter addresses_and_hashes ~f:(fun (addr, hash) ->
set_hash t addr hash )
self_set_hash t addr hash )

(* a write writes only to the mask, parent is not involved need to update
both account and hash pieces of the mask *)
let set t location account =
assert_is_attached t ;
set_account t location account ;
self_set_account t location account ;
let account_address = Location.to_path_exn location in
let account_hash = Hash.hash_account account in
let merkle_path = merkle_path t location in
Expand All @@ -282,29 +282,26 @@ struct
account_hash
in
List.iter addresses_and_hashes ~f:(fun (addr, hash) ->
set_hash t addr hash )
self_set_hash t addr hash )

(* if the mask's parent sets an account, we can prune an entry in the mask
if the account in the parent is the same in the mask *)
let parent_set_notify t account =
assert_is_attached t ;
match find_location t (Account.public_key account) with
match self_find_location t (Account.public_key account) with
| None -> ()
| Some location -> (
match find_account t location with
match self_find_account t location with
| Some existing_account ->
if
Key.equal
(Account.public_key account)
(Account.public_key existing_account)
then remove_account_and_update_hashes t location
if Account.equal account existing_account then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do all fields have to match? Is it enough that the key and balance are the same? I don't understand the receipt chain hash, so I don't know, myself.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, otherwise we're losing information.

remove_account_and_update_hashes t location
| None -> () )

(* as for accounts, we see if we have it in the mask, else delegate to
parent *)
let get_hash t addr =
assert_is_attached t ;
match find_hash t addr with
match self_find_hash t addr with
| Some hash -> Some hash
| None -> (
try
Expand All @@ -323,7 +320,7 @@ struct
let get_hash_batch_exn t addrs =
assert_is_attached t ;
List.map addrs ~f:(fun addr ->
match find_hash t addr with
match self_find_hash t addr with
| Some account -> Some account
| None -> (
try Some (Base.get_inner_hash_at_addr_exn (get_parent t) addr)
Expand Down Expand Up @@ -394,11 +391,11 @@ struct

let set_raw_hash_batch t locations_and_hashes =
List.iter locations_and_hashes ~f:(fun (location, hash) ->
set_hash t (Location.to_path_exn location) hash )
self_set_hash t (Location.to_path_exn location) hash )

let set_raw_account_batch t locations_and_accounts =
List.iter locations_and_accounts ~f:(fun (location, account) ->
set_account t location account )
self_set_account t location account )
end)

let set_batch_accounts t addresses_and_accounts =
Expand Down Expand Up @@ -435,7 +432,7 @@ struct

let location_of_key t key =
assert_is_attached t ;
let mask_result = find_location t key in
let mask_result = self_find_location t key in
match mask_result with
| Some _ -> mask_result
| None -> Base.location_of_key (get_parent t) key
Expand All @@ -456,7 +453,7 @@ struct
match keys with
| [] -> (parent_keys, mask_locations)
| key :: rest -> (
match find_location t key with
match self_find_location t key with
| None -> loop rest (key :: parent_keys) mask_locations
| Some loc -> loop rest parent_keys (loc :: mask_locations) )
in
Expand Down Expand Up @@ -561,9 +558,9 @@ struct

module For_testing = struct
let location_in_mask t location =
Option.is_some (find_account t location)
Option.is_some (self_find_account t location)

let address_in_mask t addr = Option.is_some (find_hash t addr)
let address_in_mask t addr = Option.is_some (self_find_hash t addr)

let current_location t = t.current_location
end
Expand All @@ -582,7 +579,7 @@ struct
(* NB: updates the mutable current_location field in t *)
let get_or_create_account t key account =
assert_is_attached t ;
match find_location t key with
match self_find_location t key with
| None -> (
(* not in mask, maybe in parent *)
match Base.location_of_key (get_parent t) key with
Expand All @@ -598,7 +595,7 @@ struct
| None -> Or_error.error_string "Db_error.Out_of_leaves"
| Some location ->
set t location account ;
set_location t key location ;
self_set_location t key location ;
t.current_location <- Some location ;
Ok (`Added, location) ) )
| Some location -> Ok (`Existed, location)
Expand Down