GraphQL: add a query to fetch specific pending snark work #16244

anne-laure-s · 2024-10-15T14:41:28Z

This PR adds the possibility to fetch a specific range of work in the pending snark work list with GraphQL.

svv232 · 2024-10-15T15:02:03Z

src/lib/mina_graphql/mina_graphql.ml

+                 (included). If not specified, only the work at index \
+                 [startingIndex] will be returned. An empty list will be \
+                 returned if startingIndex > endingIndex"
+              ~typ:Types.Input.UInt32.arg_typ ~default:Unsigned.UInt32.max_int


My personal preference, would for this to also be non-null.

I would expect this working if startingIndex = endingIndex = 0, so I would not be in favor to expect endingIndex ≠ 0

What if the upper bound was exclusive instead? i.e 0-1 gets the first index. Also If I am calling an API end point that returns a range, I would expect n-null to return the list from the index n to the end.

I agree, it is counter intuitive for me when for the range n to max-int it returns only the element at index n

I changed the default behavior. Now

if endingIndex ≥ length(pending_work), everything from startingIndex is returned. (max_int is still the default)

endingIndex is excluded

[] is returned when startingIndex ≥ endingIndex or startingIndex ≥ length(pending_work)

I'm still thinking having max-int is unnecessary logic. I think we can come up with a solution that removes this and enforces the invariants we know to be true. For example

let pending_snark_work_range = field "pendingSnarkWorkRange" ~doc: "Find any sequence of pending snark works between two indexes in the \ pending snark work pool" ~args: Arg. [ arg "startingIndex" ~doc: "The first index to be taken from the pending snark work pool" ~typ:(non_null Types.Input.UInt32.arg_typ) ; arg "endingIndex" ~doc: "The last index to be taken from the pending snark work pool \ (excluded). If not specified or greater than the pending \ snark work list, all elements from index [startingIndex] will \ be returned. An empty list will be returned if \ [startingIndex, endingIndex] is not a valid range of the \ pending snark work list." ~typ:Types.Input.UInt32.arg_typ ] ~typ:(non_null @@ list @@ non_null Types.pending_work) ~resolve:(fun { ctx = mina; _ } () start_idx end_idx -> let pending_work = get_pending_work mina in let pending_work_size = pending_work |> List.length |> Unsigned.UInt32.of_int in let less_than uint1 uint2 = Unsigned.UInt32.compare uint1 uint2 < 0 in match end_idx with | None -> (* drop handles case when start_idx is greater than pending work and is O(start_idx)*) let start = Unsigned.UInt32.to_int start_idx in List.drop pending_work start | Some end_idx when less_than start_idx end_idx && less_than end_idx pending_work_size -> let pos = Unsigned.UInt32.to_int start_idx in let len = Unsigned.UInt32.(sub end_idx start_idx |> to_int) in List.sub ~pos ~len pending_work | _ -> [] )

Is there a reason for prefering UInt32 manipulation over to_int conversion from the start ?

I didn't want to run any transformations on user controlled data before checking, and it was more convenient to write with the match case. You can do uint to int conversions safely, but I wasn't sure how the js one was implemented. I just took the safe route without reading the implementation. Maybe it's also worth having some error checking in the None case as well for start idx.

I pushed a new version with your suggestion but I modified some match cases to match the spec.

svv232 · 2024-10-15T15:06:30Z

src/lib/mina_graphql/mina_graphql.ml

        in
-        Work_selector.pending_work_statements ~snark_pool ~fee_opt
-          snark_job_state )
+        try List.sub ~pos:start_idx ~len @@ get_pending_work mina with _ -> []


I am fine with a slice here because the Janestreet runtime is O(n), and we won't actually save on significant ram usage because work selection stores state.available jobs anyway in work_lib. Since state.availabe jobs is a super set of the pending work, we wouldn't save much space overall i.e worst case O(2n)

this is also currently a low frequency graphql call

svv232 · 2024-10-15T15:08:03Z

src/app/cli/src/init/client.ml

+let pending_snark_work_range =
+  Command.async
+    ~summary:
+      "Range of snark works in JSON format that are not available in the pool \
+       yet"
+    pending_snark_work_list


shouldn't this also splice with a start and end?

You are right, & this is actually unrelated, I’ll remove it from these commits

that makes sense, I thought the client and graphql changes could be split up but i didn't really mind.

svv232 · 2024-10-16T01:36:25Z

src/app/cli/src/init/graphql_queries.ml

+module Pending_snark_work_range =
+[%graphql
+{|
+query pendingSnarkWorkRange {
+  pendingSnarkWork {
+    workBundle {
+      source_first_pass_ledger_hash: sourceFirstPassLedgerHash
+      target_first_pass_ledger_hash: targetFirstPassLedgerHash
+      source_second_pass_ledger_hash: sourceSecondPassLedgerHash
+      target_second_pass_ledger_hash: targetSecondPassLedgerHash
+      fee_excess: feeExcess {
+        feeTokenLeft
+        feeExcessLeft {
+          sign
+          feeMagnitude
+        }
+        feeTokenRight
+        feeExcessRight {
+          sign
+          feeMagnitude
+        }
+      }
+      supply_increase : supplyIncrease
+      work_id: workId
+      }
+    }
+  }
+|}]
+


i think should also be deleted for now, until/if the client command is added back

Right, deleted

svv232 · 2024-10-22T07:48:34Z

src/lib/mina_graphql/mina_graphql.ml

+        | None when less_than start_idx pending_work_size ->
+            (* drop handles case when start_idx is greater than pending work and is O(start_idx)*)
+            let start = Unsigned.UInt32.to_int start_idx in
+            List.drop pending_work start


nit
drop returns empty list when the start index is greater than the list size, so you don't really need the check, but it is fine to leave it.

That’s good to know !

I’m in favor to leave it to make the behavior explicit, but I don’t have a strong opinion about it.

ya that is fine, i mentioned it in the comment to clarify as well on line 2247

svv232 · 2024-10-22T07:54:42Z

!ci-build-me

svv232 · 2024-10-22T17:27:02Z

!ci-nightly-me

coveralls · 2024-10-22T21:01:28Z

coverage: 61.0% (+0.01%) from 60.989%
when pulling d5882ba on al/snark-work-graphql
into 4f78a48 on compatible.

deepthiskumar · 2024-11-19T19:41:10Z

!ci-build-me

svv232 · 2024-11-20T14:16:37Z

src/lib/mina_graphql/mina_graphql.ml

@@ -2202,19 +2202,69 @@ module Queries = struct
        Mina_lib.snark_pool mina |> Network_pool.Snark_pool.resource_pool
        |> Network_pool.Snark_pool.Resource_pool.all_completed_work )

+  (* Auxiliary function to fetch all pending work *)
+  let get_pending_work mina =


I feel this is slightly misnamed now. The design has changed from the previous intention. Previously we obtained all pending work without fees, where as now it seems we obtain all work, including work that was previously completed in case another snark worker want to provide a better bid. I feel this change in intention should be documented and explained in this doc comment.

perhaps something like:
get_pending_and_completed_work

svv232

The logic looks good, I just wanted you to change the pending_snark_work graphql name to something more apt. It seems the goal now is to get all work indexes in the case an external snark work script want to re-do work at a better price.

This function was initially added to unify code. At this point this is no longer relevant, as it is now used only in one place.

…raphql

…ding and completed work

svv232 · 2024-12-09T20:24:28Z

!ci-build-me

svv232 · 2024-12-09T20:30:42Z

!ci-build-me

anne-laure-s added GraphQL Snark work labels Oct 15, 2024

anne-laure-s requested review from mrmr1993, deepthiskumar and svv232 October 15, 2024 14:41

anne-laure-s self-assigned this Oct 15, 2024

anne-laure-s requested a review from a team as a code owner October 15, 2024 14:41

anne-laure-s mentioned this pull request Oct 15, 2024

Expose pending snark work query and proof submission in CLI/GraphQL #16214

Closed

svv232 reviewed Oct 15, 2024

View reviewed changes

anne-laure-s force-pushed the al/snark-work-graphql branch from 65f30d1 to bed927c Compare October 15, 2024 15:33

svv232 reviewed Oct 16, 2024

View reviewed changes

anne-laure-s force-pushed the al/snark-work-graphql branch from bed927c to 2d977f5 Compare October 16, 2024 10:30

anne-laure-s added 2 commits October 21, 2024 15:53

GraphQL: add a query for a range of pending snark work

16bc9db

GraphQL: unify code for pending work functions

d5882ba

anne-laure-s force-pushed the al/snark-work-graphql branch from 2d977f5 to d5882ba Compare October 21, 2024 14:58

svv232 approved these changes Oct 22, 2024

View reviewed changes

deepthiskumar added 3 commits November 13, 2024 14:22

Merge branch 'compatible' into al/snark-work-graphql

8ad693e

output snark work spec for the enw pending snark work command

0191ce9

Merge branch 'compatible' into al/snark-work-graphql

85b4ffc

deepthiskumar requested a review from svv232 November 20, 2024 00:50

svv232 reviewed Nov 20, 2024

View reviewed changes

svv232 requested changes Nov 20, 2024

View reviewed changes

anne-laure-s and others added 3 commits December 2, 2024 21:03

Mina_graphql: remove get_pending_work function

246872b

This function was initially added to unify code. At this point this is no longer relevant, as it is now used only in one place.

Mina_graphql: update pending_snark_work_range doc

5809c3f

Merge remote-tracking branch 'origin/compatible' into al/snark-work-g…

8b97a48

…raphql

svv232 added 2 commits December 9, 2024 15:11

updating pending snark work range documentation based off doc comment

f6c3946

changing pending work snark work range in graphql to reflect both ben…

3a5a43a

…ding and completed work

updating graphql schema with new documentation

405fb7b

svv232 approved these changes Dec 9, 2024

View reviewed changes

svv232 merged commit c1634c3 into compatible Dec 9, 2024
44 checks passed

svv232 deleted the al/snark-work-graphql branch December 9, 2024 21:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GraphQL: add a query to fetch specific pending snark work #16244

GraphQL: add a query to fetch specific pending snark work #16244

anne-laure-s commented Oct 15, 2024

svv232 Oct 15, 2024

anne-laure-s Oct 15, 2024

svv232 Oct 15, 2024 •

edited

Loading

deepthiskumar Oct 15, 2024 •

edited

Loading

anne-laure-s Oct 16, 2024

svv232 Oct 16, 2024 •

edited

Loading

anne-laure-s Oct 17, 2024

svv232 Oct 18, 2024 •

edited

Loading

anne-laure-s Oct 21, 2024

svv232 Oct 15, 2024 •

edited

Loading

svv232 Oct 15, 2024

svv232 Oct 15, 2024

anne-laure-s Oct 15, 2024 •

edited

Loading

svv232 Oct 15, 2024

svv232 Oct 16, 2024

anne-laure-s Oct 16, 2024

svv232 Oct 22, 2024

anne-laure-s Oct 22, 2024

svv232 Oct 22, 2024 •

edited

Loading

svv232 commented Oct 22, 2024

svv232 commented Oct 22, 2024

coveralls commented Oct 22, 2024

deepthiskumar commented Nov 19, 2024

svv232 Nov 20, 2024

svv232 left a comment

svv232 commented Dec 9, 2024

svv232 commented Dec 9, 2024

GraphQL: add a query to fetch specific pending snark work #16244

GraphQL: add a query to fetch specific pending snark work #16244

Conversation

anne-laure-s commented Oct 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svv232 Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

deepthiskumar Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svv232 Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svv232 Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svv232 Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anne-laure-s Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svv232 Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

svv232 commented Oct 22, 2024

svv232 commented Oct 22, 2024

coveralls commented Oct 22, 2024

deepthiskumar commented Nov 19, 2024

Choose a reason for hiding this comment

svv232 left a comment

Choose a reason for hiding this comment

svv232 commented Dec 9, 2024

svv232 commented Dec 9, 2024

svv232 Oct 15, 2024 •

edited

Loading

deepthiskumar Oct 15, 2024 •

edited

Loading

svv232 Oct 16, 2024 •

edited

Loading

svv232 Oct 18, 2024 •

edited

Loading

svv232 Oct 15, 2024 •

edited

Loading

anne-laure-s Oct 15, 2024 •

edited

Loading

svv232 Oct 22, 2024 •

edited

Loading