PR #16186, split for re-review #16502

georgeee · 2025-01-22T10:43:51Z

This is not intended to be merged into any of major branches.

Created for the sole purpose of visibility.

This PR has the same code changes as #16186, but organized in a granular fashion.

To test against merge commit e130ac7: git diff e130ac77a50149ebb0ff3840e4ac5dae32afe201 (returns zero code and no output).

Rename: init_from_config_file -> Config_loader.init_from_config_file_legacy

Accept config file parameter in `ledger hash`, 'ledger test apply' and 'advanced itn-create-accounts' commands.

…un-snark-worker-single'

georgeee · 2025-01-23T12:31:49Z

src/app/batch_txn_tool/batch_txn_tool.ml

@@ -390,23 +387,32 @@ let output_there_and_back_cmds =
            transactions, if this is not present then we use the env var \
            MINA_PRIVKEY_PASS"
         (optional string)
+     and config_file = Cli_lib.Flag.config_files


Clarification: commit f070475 (Remove default_transaction_fee from node_config) also adds --config-file to the batch_txn_tool app

georgeee · 2025-01-23T12:49:39Z

src/lib/genesis_proof/genesis_proof.ml

@@ -85,6 +86,7 @@ module T = struct
    ; constraint_constants : Genesis_constants.Constraint_constants.t
    ; genesis_constants : Genesis_constants.t
    ; proof_level : Genesis_constants.Proof_level.t
+    ; compile_config : Mina_compile_config.t


RE commit 788ab19

This field is added to Genesis_proof.t so that it later appears in Precomputed_values.t and in all of the places that get it.

I don't like this solution. I tried to revert it on compatible, but there turned out to be many places which need to be updated.

I suggest to leave it "as is" and later find a better solution for passing Mina_compile_config.t to various structures. BTW, the name will soon be completely pointless, as this config is configured in runtime now as much as in runtime. I suggest after refactoring to get rid of any compile-time configuration whatsoever we reconsider use of both Mina_compile_config.t and Precomputed_values.t. And skip spending time on partial solutions now

I agree that this is terrible. The original PR should not have landed in this state.

Are you against it so much as to ask us to rework this immediately?

Or are you okay with this change to live in the codebase for a bit longer? Until we have more progress on dismantling the compile-time config and then we plan to remove Mina_compile_config.t, so it will naturally disappear from Inputs.t as well.

georgeee · 2025-01-23T13:27:46Z

A NIT: reversion of commit 60b5ab3 would be good to have in compatible

georgeee · 2025-01-23T12:54:06Z

src/lib/mina_lib/tests/tests.ml

-          ; proof = None
+          ; proof =
+              Some
+                { Runtime_config.Proof_keys.default with level = Some No_check }


Comment about the change: we set the proof level used in mina_lib test to be No_check through runtime config and not through constants (which would be a 1:1 rewrite from previous code).

It'd be somewhat more straightforward to use the following code in the call below:

~constants:({(Runtime_config.Constants.magic_for_unit_tests runtime_config) with proof_level = No_check})

But it would work just the same as what is implemented now.

georgeee · 2025-01-23T13:09:03Z

src/lib/snark_worker/standalone/run_snark_worker.ml

-         Genesis_constants.Compiled.constraint_constants
+       let open Deferred.Let_syntax in
+       let%bind constraint_constants, proof_level =
+         let logger = Logger.create () in


This is a correct usage of logger, as it's a daemon-like runner rather than a CLI command.

This is inaccurate, this really is a CLI command. That said, I added it for my own debugging purposes, so if you want to pollute the output then go for it.

For a command used for debugging some log output should be helpful

georgeee · 2025-01-23T13:11:37Z

src/lib/crypto/snarky_tests/snarky_tests.ml

  let range_checks =
    List.map ~f:QCheck_alcotest.to_alcotest [ RangeCircuits.test_range_gates ]
  in
+  let logger = Logger.create () in


Unnecessary use of logging, removed in compatible.

georgeee · 2025-01-23T13:12:19Z

src/lib/cli_lib/commands.ml

-        Genesis_constants.Compiled.constraint_constants
+      let open Deferred.Let_syntax in
+      let%bind constraint_constants =
+        let logger = Logger.create () in


Unnecessary use of logging, removed in compatible.

georgeee · 2025-01-23T13:12:24Z

src/lib/cli_lib/commands.ml

+      let%bind constraint_constants =
+        let logger = Logger.create () in
+        let%map conf =
+          Runtime_config.Constants.load_constants ~logger config_file


Unnecessary use of logging, removed in compatible.

georgeee · 2025-01-23T13:24:51Z

src/app/cli/src/cli_entrypoint/mina_cli_entrypoint.ml

-            Genesis_constants.Compiled.constraint_constants
+          let%bind constraint_constants, proof_level =
+            let%map conf =
+              Runtime_config.Constants.load_constants ~logger config_file


Logger usage is retained from pre-existing code, the same in compatible

georgeee · 2025-01-23T13:35:06Z

src/app/cli/src/cli_entrypoint/mina_cli_entrypoint.ml

          let%bind precomputed_values, _ =
            Genesis_ledger_helper.Config_loader.load_config_files ~logger
-              ~conf_dir ~genesis_dir ~genesis_constants ~constraint_constants
-              ~proof_level config_files ~cli_proof_level:None
+              ~conf_dir ?genesis_dir ~cli_proof_level:Full ~itn_features


cli_proof_level is used here so that whatever is provided in compile-time config and initialized via load_config_files is overridden with Full.

georgeee · 2025-01-23T13:36:05Z

src/lib/genesis_ledger_helper/genesis_ledger_helper.ml

    let%bind config =
-      Runtime_config.Json_loader.load_config_files ~conf_dir ~logger
+      Runtime_config.Json_loader.load_config_files ?conf_dir ~logger


Logging behavior doesn't change comparing to previous version, which was present in mina_cli_entrypoint

georgeee · 2025-01-23T13:36:11Z

src/lib/genesis_ledger_helper/genesis_ledger_helper.ml

      Option.value ~default:(conf_dir ^/ "genesis") genesis_dir
    in
+    let%bind.Deferred constants =
+      Runtime_config.Constants.load_constants ?conf_dir ?cli_proof_level


Note Json_loader.load_config_files is called twice: within load_constants and via a direct call below.

This results in Reading configuration files being printed twice when the node starts. Worth fixing in compatible.

Not only did this changeset break commands, but it also added output at the start of the program that has embarrassing duplication? Fantastic.

I would like this to be fixed before the release.

This is already fixed in compatible

georgeee · 2025-01-23T13:40:33Z

src/app/cli/src/cli_entrypoint/mina_cli_entrypoint.ml

  ; ("client", Client.client)
  ; ("advanced", Client.advanced ~itn_features)
  ; ("ledger", Client.ledger)
  ; ("libp2p", Client.libp2p)
  ; ( "internal"
-    , Command.group ~summary:"Internal commands" (internal_commands logger) )
+    , Command.group ~summary:"Internal commands"


Commit cb395b2 also propagates ~itn_features down daemon and internal_commands -> replay_blocks so that it's read from compile time config in the only place in mina_cli_entrypoint

georgeee · 2025-01-23T14:01:26Z

Do not load config twice in mina_cli_entrypoint.ml #16512

Created a PR to address minor "issues" spotted while reviewing this PR

mrmr1993 · 2025-01-22T16:00:38Z

src/lib/runtime_config/runtime_config.ml

+                unit tests pass.
+          *)
+          1
+          + Core_kernel.Int.ceil_log2


Nit: Core_kernel is already open.
(But I appreciate the faithful movement of the function instead of modifying it.)

Addressed in 1db6920

mrmr1993 · 2025-01-27T23:44:24Z

src/lib/runtime_config/runtime_config.ml

+    in
+    { genesis_constants; constraint_constants; proof_level; compile_config }
+
+  let load_constants' ?itn_features ?cli_proof_level runtime_config =


This function name is very misleading. It appears to be doing no loading at all.

Addressed in 7198098 (another PR)

mrmr1993 · 2025-01-27T23:50:26Z

src/lib/genesis_proof/genesis_proof.ml

@@ -85,6 +86,7 @@ module T = struct
    ; constraint_constants : Genesis_constants.Constraint_constants.t
    ; genesis_constants : Genesis_constants.t
    ; proof_level : Genesis_constants.Proof_level.t
+    ; compile_config : Mina_compile_config.t


I agree that this is terrible. The original PR should not have landed in this state.

mrmr1993 · 2025-01-27T23:51:40Z

src/lib/cli_lib/flag.ml

@@ -33,6 +33,15 @@ let conf_dir =
  flag "--config-directory" ~aliases:[ "config-directory" ]
    ~doc:"DIR Configuration directory" (optional string)

+let config_files =
+  let open Command.Param in
+  flag "--config-file" ~aliases:[ "config-file" ]


Does this mean that we've added the junk -config-file flag alias everywhere now, just because it was on the Mina CLI for backcompat? That's annoying.

Addressed in 259824f

mrmr1993 · 2025-01-27T23:57:13Z

src/app/batch_txn_tool/batch_txn_tool.ml

@@ -390,23 +387,32 @@ let output_there_and_back_cmds =
            transactions, if this is not present then we use the env var \
            MINA_PRIVKEY_PASS"
         (optional string)
+     and config_file = Cli_lib.Flag.config_files


We're calling a list of config files here config_file? That feels confusing at the very least.

This is a typo, there are a few more places like this one, fixed in 6d26821

mrmr1993 · 2025-01-28T00:40:20Z

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

-             ~genesis_constants ~constraint_constants ~logger:(Logger.null ())
-             ~proof_level ~cli_proof_level:None ~genesis_dir runtime_config
-           >>| Or_error.ok_exn
+         let%bind m_conf =


What is an m_conf?

Renamed this variable, see a5a39eb

mrmr1993 · 2025-01-28T00:41:15Z

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

+           Runtime_config.Constants.load_constants' runtime_config
+         in
+         Genesis_ledger_helper.Config_loader.init_from_config_file ~genesis_dir
+           ~logger ~constants runtime_config


This change is particularly irritating, changing Logger.null to logger (non-null above).

In compatible we set logger to Logger.null () for the entire command

mrmr1993 · 2025-01-28T00:50:46Z

src/lib/genesis_ledger_helper/genesis_ledger_helper.ml

      Option.value ~default:(conf_dir ^/ "genesis") genesis_dir
    in
+    let%bind.Deferred constants =
+      Runtime_config.Constants.load_constants ?conf_dir ?cli_proof_level


Not only did this changeset break commands, but it also added output at the start of the program that has embarrassing duplication? Fantastic.

mrmr1993 · 2025-01-28T00:51:06Z

src/lib/genesis_ledger_helper/genesis_ledger_helper.ml

      Option.value ~default:(conf_dir ^/ "genesis") genesis_dir
    in
+    let%bind.Deferred constants =
+      Runtime_config.Constants.load_constants ?conf_dir ?cli_proof_level


I would like this to be fixed before the release.

mrmr1993 · 2025-01-28T00:52:18Z

src/app/cli/src/cli_entrypoint/mina_cli_entrypoint.ml

@@ -216,8 +216,9 @@ let setup_daemon logger ~itn_features =
  and snark_work_fee =
    flag "--snark-worker-fee" ~aliases:[ "snark-worker-fee" ]
      ~doc:
-        "FEE Amount a worker wants to get compensated for generating a snark \
-         proof"
+        (sprintf


I love a no-op sprintf.

Fixed in compatible

georgeee added 25 commits January 22, 2025 14:21

Move transaction_capacity_log_2 to Runtime_config

37fde44

Introduce Runtime_config.Constants

33ce3c3

Rename Genesis_ledger_helper.init_from_config_file

71bd217

Rename: init_from_config_file -> Config_loader.init_from_config_file_legacy

Split out legacy part from inputs_from_config_file

e053563

Add compile_config to Genesis_proof.Inputs

788ab19

Add config_files flag to Cli_lib

37e3584

Remove default_transaction_fee from node_config

f070475

Implement init_from_config_file (non-legacy)

ffbfe45

Use init_from_config_file in mina_lib tests

b2eb3df

Use init_from_config_file in archive

34d7a98

Accept optional --config-file in standalone SW

d5a56e3

Accept optional --config-file in 'internal snark-worker'

7860cdf

Use load_constants in snarky_tests

3062393

Accept optional --config-file in vrf cli commands

eaba812

Use load_constants in delegation_verify app

09bd9bd

Accept --config-file in hash ledger and two other

2249d75

Accept config file parameter in `ledger hash`, 'ledger test apply' and 'advanced itn-create-accounts' commands.

Accept optional --config-file for compile-time-constants

10df606

Accept optional --config-file in constraint_system_digests

39b7c74

Accept optional --config-file in 'internal run-prover'

eefcc50

Accept optional --config-file in 'internal run-verifier', 'internal r…

bb8ebe1

…un-snark-worker-single'

Move load_config_files to Genesis_ledger_helpers

c1d7fbd

Remove default_snark_worker_fee usage

b47cf86

Use non-legacy inputs_from_config_file in load_config_files

cb395b2

Remove legacy functions from Config_loader

b98f42f

Remainder of changes from PR #16186

60b5ab3

georgeee force-pushed the georgeee/redo-16186-for-better-review branch from cf46526 to 60b5ab3 Compare January 22, 2025 14:21

georgeee marked this pull request as ready for review January 22, 2025 14:24

georgeee requested review from a team as code owners January 22, 2025 14:24

georgeee changed the title ~~PR #16186, split for re-review, commit 1/N~~ PR #16186, split for re-review Jan 22, 2025

georgeee commented Jan 23, 2025

View reviewed changes

mrmr1993 reviewed Jan 28, 2025

View reviewed changes

georgeee mentioned this pull request Jan 28, 2025

Address #16502 review #16542

Open

7 tasks

mrmr1993 mentioned this pull request Jan 28, 2025

Revert 16186 and all subsequent runtime config changes #16545

Closed

PR #16186, split for re-review #16502

Are you sure you want to change the base?

PR #16186, split for re-review #16502

Conversation

georgeee commented Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

georgeee Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgeee commented Jan 23, 2025

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgeee commented Jan 23, 2025 • 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

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

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

georgeee commented Jan 22, 2025 •

edited

Loading

georgeee Jan 23, 2025 •

edited

Loading

georgeee commented Jan 23, 2025 •

edited

Loading