Make zeko circuits compilable to js#394
Conversation
MartinOndejka
force-pushed
the
zeko-to-js
branch
from
a9d723c to
0b63e89
Compare
Summary of ChangesHello @MartinOndejka, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request undertakes a significant refactoring effort to make Zeko circuits compilable to JavaScript. The changes primarily involve migrating core library dependencies from Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the codebase to improve JavaScript compatibility by replacing Core with Core_kernel, Async with Async_kernel, and Uuid_unix with Uuid.create_random. It also replaces Rocksdb.Database with Mina_ledger_kvdb.Database for key-value storage. The rmrf function, which uses Caml.Sys.command, is flagged as problematic for JS compilation and should be abstracted. The of_yojson implementations for Inet_addr and Peer modules are vulnerable to DoS attacks due to uncaught exceptions when parsing malformed IP addresses. The t_of_sexp function in Incoming module is also vulnerable to DoS attacks due to uncaught exceptions when parsing malformed time formats. The zeko_prev_key function's performance is a concern due to sorting the entire database table on every invocation. The logic for measuring ledger sync duration has been removed, resulting in a loss of observability. Finally, the tokens test case has been removed, decreasing test coverage.
| let rmrf path = | ||
| ignore | ||
| (Caml.Sys.command | ||
| (sprintf "rm -rf %s" (Filename.quote path)) | ||
| : int ) |
There was a problem hiding this comment.
The rmrf function uses Caml.Sys.command, which is not available in JavaScript and will cause a runtime error. Given that the goal of this PR is to make circuits JS-compilable, this is a critical issue. This functionality should be abstracted away, perhaps into a new virtual module similar to Mina_ledger_kvdb, with a no-op implementation for JavaScript and the current implementation for native builds.
| let of_yojson = function | ||
| | `String s -> | ||
| Ok (Unix.Inet_addr.of_string s) | ||
| Ok (Caml_unix.inet_addr_of_string s) | ||
| | _ -> | ||
| Error "expected string" |
There was a problem hiding this comment.
The of_yojson implementation for Inet_addr calls Caml_unix.inet_addr_of_string directly on the input string. This function raises a Failure exception if the string is not a valid IP address. Since this exception is not caught, providing a malformed IP address in a JSON payload (e.g., from a network peer or a configuration file) will cause the entire application to crash, leading to a Denial of Service (DoS).
let of_yojson = function
| `String s ->
(try Ok (Caml_unix.inet_addr_of_string s)
with _ -> Error "invalid IP address")
| _ ->
Error "expected string"
| List.Assoc.find ls "libp2p_port" ~equal:String.equal >>= lift_int | ||
| in | ||
| let host = Unix.Inet_addr.of_string host_str in | ||
| let host = Caml_unix.inet_addr_of_string host_str in |
There was a problem hiding this comment.
Similar to the Inet_addr module, the of_yojson implementation for the Peer module calls Caml_unix.inet_addr_of_string on host_str without handling potential exceptions. A malformed 'host' field in the JSON will crash the application.
let host = try Caml_unix.inet_addr_of_string host_str with _ -> failwith "invalid host" in
| let t_of_sexp a_of_sexp = function | ||
| | Sexp.List [ data; sender; received_at ] -> | ||
| { data = a_of_sexp data | ||
| ; sender = Sender.t_of_sexp sender | ||
| ; received_at = | ||
| ( match received_at with | ||
| | Sexp.Atom s -> | ||
| Time.of_string s | ||
| | _ -> | ||
| failwith "Incoming.t_of_sexp: received_at must be atom" ) | ||
| } | ||
| | _ -> | ||
| failwith "Incoming.t_of_sexp: expected 3-tuple list" |
There was a problem hiding this comment.
The t_of_sexp function calls Time.of_string on an atom from the Sexp input. Time.of_string raises an exception if the string is not a valid time format. If this function is used to deserialize data from an untrusted source, it can be used to trigger a crash (DoS).
| let zeko_prev_key t ~key = | ||
| let rec loop prev = function | ||
| | [] -> ( | ||
| match prev with | ||
| | Some k -> | ||
| k | ||
| | None -> | ||
| failwith "zeko_prev_key: no previous key found" ) | ||
| | (k, _) :: rest -> | ||
| if Bigstring.compare k key >= 0 then ( | ||
| match prev with | ||
| | Some k -> | ||
| k | ||
| | None -> | ||
| failwith "zeko_prev_key: no previous key found" ) | ||
| else loop (Some k) rest | ||
| in | ||
| loop None (to_alist t) | ||
| end |
There was a problem hiding this comment.
The zeko_prev_key function calls to_alist, which performs a full sort of the entire database table (List.sort) on every invocation. In the context of an indexed Merkle tree, this function is called frequently during insertions and lookups. As the number of entries (N) in the ledger grows, the
| match requested_syncs with | ||
| | One required_sync -> | ||
| let open Async.Deferred.Let_syntax in | ||
| let start = Core.Time.now () in | ||
| let open Deferred.Let_syntax in | ||
| let%map result = sync required_sync in | ||
| let { snapshot_id; _ } = required_sync in | ||
| ( match snapshot_id with | ||
| | Staking_epoch_snapshot -> | ||
| Mina_metrics.( | ||
| Counter.inc Bootstrap.staking_epoch_ledger_sync_ms | ||
| Core.Time.(diff (now ()) start |> Span.to_ms)) | ||
| Counter.inc Bootstrap.staking_epoch_ledger_sync_ms 0.0) | ||
| | Next_epoch_snapshot -> | ||
| Mina_metrics.( | ||
| Counter.inc Bootstrap.next_epoch_ledger_sync_ms | ||
| Core.Time.(diff (now ()) start |> Span.to_ms)) ) ; | ||
| Counter.inc Bootstrap.next_epoch_ledger_sync_ms 0.0) ) ; | ||
| result | ||
| | Both { staking; next } -> | ||
| (*Sync staking ledger before syncing the next ledger*) | ||
| let open Deferred.Or_error.Let_syntax in | ||
| let start = Core.Time.now () in | ||
| let%bind () = | ||
| sync | ||
| { snapshot_id = Staking_epoch_snapshot | ||
| ; expected_root = staking | ||
| } | ||
| in | ||
| Mina_metrics.( | ||
| Counter.inc Bootstrap.staking_epoch_ledger_sync_ms | ||
| Core.Time.(diff (now ()) start |> Span.to_ms)) ; | ||
| let start = Core.Time.now () in | ||
| Counter.inc Bootstrap.staking_epoch_ledger_sync_ms 0.0) ; | ||
| let%map () = | ||
| sync { snapshot_id = Next_epoch_snapshot; expected_root = next } | ||
| in | ||
| Mina_metrics.( | ||
| Counter.inc Bootstrap.next_epoch_ledger_sync_ms | ||
| Core.Time.(diff (now ()) start |> Span.to_ms)) ) | ||
| Counter.inc Bootstrap.next_epoch_ledger_sync_ms 0.0) ) |
There was a problem hiding this comment.
The logic for measuring ledger sync duration has been removed, and the corresponding metrics are now always reported as 0.0. This results in a loss of observability. Since Core_kernel.Time is available, the timing functionality can be restored in a JS-compatible way.
match requested_syncs with
| One required_sync ->
let open Deferred.Let_syntax in
let start = Time.now () in
let%map result = sync required_sync in
let { snapshot_id; _ } = required_sync in
( match snapshot_id with
| Staking_epoch_snapshot ->
Mina_metrics.(
Counter.inc Bootstrap.staking_epoch_ledger_sync_ms
Time.(diff (now ()) start |> Span.to_ms))
| Next_epoch_snapshot ->
Mina_metrics.(
Counter.inc Bootstrap.next_epoch_ledger_sync_ms
Time.(diff (now ()) start |> Span.to_ms)) ) ;
result
| Both { staking; next } ->
(*Sync staking ledger before syncing the next ledger*)
let open Deferred.Or_error.Let_syntax in
let start = Time.now () in
let%bind () =
sync
{ snapshot_id = Staking_epoch_snapshot
; expected_root = staking
}
in
Mina_metrics.(
Counter.inc Bootstrap.staking_epoch_ledger_sync_ms
Time.(diff (now ()) start |> Span.to_ms)) ;
let start = Time.now () in
let%map () =
sync { snapshot_id = Next_epoch_snapshot; expected_root = next }
in
Mina_metrics.(
Counter.inc Bootstrap.next_epoch_ledger_sync_ms
Time.(diff (now ()) start |> Span.to_ms)) )
| [| keypair_and_amounts.(0); keypair_and_amounts.(1) |] | ||
| ledger ; | ||
| main ledger ) | ||
| let%test_unit "tokens test" = () |
There was a problem hiding this comment.
The test case "tokens test" has been removed, which results in a loss of test coverage for token functionality with zkApps. This was likely done to remove the Zkapp_command_builder dependency for JS compatibility. Please consider refactoring the test to work without this dependency or add a TODO comment to restore it in the future.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| { staking = Uuid.create_random Random.State.default | ||
| ; next = Uuid.create_random Random.State.default |
There was a problem hiding this comment.
Bug: The switch to Uuid.create_random Random.State.default for UUID generation is not thread-safe and can cause collisions in concurrent environments, leading to data corruption.
Severity: CRITICAL
Suggested Fix
Revert the use of Uuid.create_random Random.State.default in multi-threaded contexts. If Uuid_unix cannot be used due to JS-compatibility, a thread-safe random number generator, such as one based on Splittable_random.State, should be used to ensure uniqueness. Alternatively, implement synchronization mechanisms like mutexes around the UUID generation calls.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/lib/consensus/proof_of_stake.ml#L478-L479
Potential issue: The systematic replacement of `Uuid_unix.create()` with
`Uuid.create_random Random.State.default` introduces a race condition. The new function
uses a shared, process-local pseudo-random number generator that is not thread-safe. In
a concurrent environment, such as the Mina node's consensus module, simultaneous calls
from different threads can produce identical UUIDs. Because these UUIDs are used to
construct filesystem paths for epoch ledgers, a collision will cause data from different
ledgers to overwrite each other, leading to silent data corruption.
Did we get this right? 👍 / 👎 to inform future reviews.
1 participant
No description provided.