Forbid skipping of indices in account set #345
Conversation
| in | ||
| let input = | ||
| let open Random_oracle.Input.Chunked in | ||
| append |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
You are signing the ledger, not its history.
| 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 |
There was a problem hiding this comment.
this is somewhat inefficient, you can generate a list of bools and then check if any of them is false instead of this
| 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 ) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yeah good catch. I need is_valid_right which is supposed to be not equal to the empty hash.
Closes #282