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.

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 two bugs related to write notifications in masks #1794

Merged
merged 9 commits into from Mar 6, 2019

Conversation

Projects
None yet
3 participants
@cmr
Copy link
Contributor

cmr commented Mar 5, 2019

The first is that set_batch and related methods on maskable weren't causing write notifications. The second is that write notification handling was too liberal. It would forget about the mask's copy if just the PKs match. For correctness, the entire account needs to match.

cmr added some commits Mar 5, 2019

masking_merkle_tree: rename some private funcs to self_
this helped me while I was scanning through some of this code for bugs.
it wasn't clear which functions just mutated inner state vs integrate
the base, except via their comments.

@cmr cmr requested a review from psteckler Mar 5, 2019

@@ -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 \

This comment has been minimized.

@psteckler

psteckler Mar 5, 2019

Contributor

should the different balances be mentioned?

This comment has been minimized.

@cmr

cmr Mar 5, 2019

Author Contributor

Yeah. I've clarified the string a bit.

This comment has been minimized.

@psteckler

psteckler Mar 6, 2019

Contributor

there's some leftover text in the string

@@ -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 =

This comment has been minimized.

@psteckler

psteckler Mar 5, 2019

Contributor

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

Can we name this batch_notify_mask_children ?

@@ -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 =

This comment has been minimized.

@psteckler

psteckler Mar 5, 2019

Contributor

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

(Account.public_key account)
(Account.public_key existing_account)
then remove_account_and_update_hashes t location
if Account.equal account existing_account then

This comment has been minimized.

@psteckler

psteckler Mar 5, 2019

Contributor

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.

This comment has been minimized.

@cmr

cmr Mar 5, 2019

Author Contributor

Yeah, otherwise we're losing information.

@psteckler
Copy link
Contributor

psteckler left a comment

I can approve if you can fix the dangling text in test_mask.ml.

The unit tests are not passing, though.

cmr added some commits Mar 6, 2019

remove that assertion for now
our tests violate it a lot.

@cmr cmr added the ready-to-merge label Mar 6, 2019

@bkase

bkase approved these changes Mar 6, 2019

outdated

@mergify mergify bot merged commit c16c1f8 into master Mar 6, 2019

16 of 17 checks passed

ci/circleci: build-macos CircleCI is running your tests
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
ci/circleci: build-wallet Your tests passed on CircleCI!
Details
ci/circleci: build_dev Your tests passed on CircleCI!
Details
ci/circleci: build_testnet_posig Your tests passed on CircleCI!
Details
ci/circleci: build_testnet_postake Your tests passed on CircleCI!
Details
ci/circleci: build_testnet_postake_snarkless_fake_hash Your tests passed on CircleCI!
Details
ci/circleci: build_testnet_public Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test-fake_hash_full_test Your tests passed on CircleCI!
Details
ci/circleci: test-posig_integration_tests Your tests passed on CircleCI!
Details
ci/circleci: test-postake_integration_tests Your tests passed on CircleCI!
Details
ci/circleci: test-postake_split_integration_tests Your tests passed on CircleCI!
Details
ci/circleci: test-unit-test Your tests passed on CircleCI!
Details
ci/circleci: test-withsnark-sig Your tests passed on CircleCI!
Details
ci/circleci: test-withsnark-stake Your tests passed on CircleCI!
Details
ci/circleci: tracetool Your tests passed on CircleCI!
Details

@mergify mergify bot deleted the fix/notify-parent-bug branch Mar 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.