-
Notifications
You must be signed in to change notification settings - Fork 11
Forbid skipping of indices in account set #345
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
Changes from all commits
215332c
12bed7d
f23a4bf
e7174ee
8fc1cdd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,7 @@ | ||
| open Core_kernel | ||
| open Snark_params.Tick | ||
| open Zeko_util | ||
| open Checked.Let_syntax | ||
|
|
||
| module Make (Inputs : sig | ||
| module Key : SnarkType | ||
|
|
@@ -36,6 +38,19 @@ struct | |
| let hash_entry = | ||
| var_to_hash ~init:Zeko_constants.indexed_merkle_tree_salt Entry.typ | ||
|
|
||
| let empty_path = | ||
| List.init height ~f:Fn.id | ||
| |> List.fold_map ~init:Field.zero ~f:(fun acc _ -> | ||
| let next = | ||
| Random_oracle.hash | ||
| ~init: | ||
| (Hash_prefix_create.salt | ||
| Zeko_constants.indexed_merkle_tree_merge_salt ) | ||
| [| acc; acc |] | ||
| in | ||
| (next, constant Field.typ acc) ) | ||
| |> snd | ||
|
|
||
| (* TODO: consider different salt per level. *) | ||
| (* NB: The first element in the list is the neighbor of init, and the next element | ||
| is a level up, and so on. This is the same as what the Mina code base does. *) | ||
|
|
@@ -46,13 +61,15 @@ struct | |
| ~typ:Typ.(F.typ * F.typ) | ||
| ~then_:(hash_other, acc) ~else_:(acc, hash_other) | ||
| in | ||
| var_to_hash ~init:"indexed merkle tree" Typ.(F.typ * F.typ) (left, right) ) | ||
| var_to_hash ~init:Zeko_constants.indexed_merkle_tree_merge_salt | ||
| Typ.(F.typ * F.typ) | ||
| (left, right) ) | ||
|
|
||
| let implied_root (entry : Entry.var) (path : Path.var) : F.var Checked.t = | ||
| let* init = hash_entry entry in | ||
| implied_root_raw init path | ||
|
|
||
| let add_key_var ?check ~x ~path_x ~y ~path_y ~z () = | ||
| let add_key_var ~check ~x ~path_x ~y ~path_y ~z () = | ||
| let* () = | ||
| with_label __LOC__ (fun () -> assert_x_less_than_y_less_than_z ~x ~y ~z) | ||
| in | ||
|
|
@@ -62,23 +79,25 @@ struct | |
| implied_root_raw Field.(constant typ zero) path_y | ||
| in | ||
| let* root_intermediate = | ||
| match check with | ||
| | Some check -> | ||
| if_ check ~typ:F.typ ~then_:root_intermediate | ||
| ~else_:root_intermediate' | ||
| | None -> | ||
| Checked.return root_intermediate | ||
| if_ check ~typ:F.typ ~then_:root_intermediate ~else_:root_intermediate' | ||
| in | ||
| let* () = | ||
| assert_equal ~label:__LOC__ F.typ root_intermediate root_intermediate' | ||
| in | ||
| let* root_new = implied_root { key = y; next_key = z } path_y in | ||
| let* root = | ||
| match check with | ||
| | Some check -> | ||
| if_ check ~typ:F.typ ~then_:root ~else_:root_new | ||
| | None -> | ||
| Checked.return root | ||
| let* root = if_ check ~typ:F.typ ~then_:root ~else_:root_new in | ||
| (* Check that no empty indices have been skipped. *) | ||
| let* is_y_most_left = | ||
| foldl (List.zip_exn path_y empty_path) ~init:Boolean.true_ | ||
| ~f:(fun acc (PathStep.{ hash_other; is_right }, empty_hash) -> | ||
| let* is_valid_left = | ||
| Field.Checked.equal empty_hash hash_other >>= Boolean.( &&& ) acc | ||
| in | ||
| if_ is_right ~typ:Boolean.typ ~then_:acc ~else_:is_valid_left ) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if is_right is true for all PathStep, then acc is always returned, and it's true, right? Isn't this wrong? Am I missing something?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah good catch. I need |
||
| in | ||
| let* () = | ||
| if_ check ~typ:Boolean.typ ~then_:is_y_most_left ~else_:Boolean.true_ | ||
| >>= Boolean.Assert.is_true | ||
| in | ||
| Checked.return (`Before_adding_y root, `After_adding_y root_new) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -166,9 +166,7 @@ struct | |
| in | ||
| let input = | ||
| let open Random_oracle.Input.Chunked in | ||
| append | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure this is sound because the order in which you insert accounts into the ledger isn't deterministic, so you can first insert an account into index 1, then index 0, which will probably result in the acc set being ordered inversely of the ledger.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but I'm not seeing the unsoundness. Acc set ordering is now supposed to be deterministic, and for ledger sequencer is sending diff with indices of accounts and da node is singing on it. Nowhere we require the ledger and accset indices to be same.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are signing the ledger, not its history. |
||
| (Ledger_hash.var_to_field target_ledger |> field) | ||
| (Account_set.to_input_var target_acc_set) | ||
| Ledger_hash.var_to_field target_ledger |> field | ||
| in | ||
| let* payload = | ||
| make_checked (fun () -> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is somewhat inefficient, you can generate a list of bools and then check if any of them is false instead of this