consolidate ppx-optcomp usage to single module #15837

martyall · 2024-07-17T14:39:08Z

High level

I consolidated all uses of ppx_optcomp to a single module Node_config and removed the dependency on ppx_optcomp and config.mlh from every other library.

This will allow us to easily replace the compile time configuration with loading a configuration file -- simply replace the implementation of Node_config to do this.

related to #15538

Changes

The vast majority of changes are due to

remove [%%import "/src/config.mlh"] wherever possible
remove use of ppx_optcomp wherever possible
remove preprocessor_deps <path_to_config.mlh> wherever possible

I tried to keep other changes minimal, just to recover from what happens when you do the above. This manifests mostly as:

Rather than use the %%inject directive, use the Node_config module values
Whenever %%ifdef is used, just use simple if-then-else statements in the code.
remove all dead code if I detected it

martyall · 2024-07-17T14:39:32Z

!ci-build-me

martyall · 2024-07-17T17:11:42Z

!ci-build-me

martyall · 2024-07-17T18:02:50Z

!ci-build-me

martyall

Some comments to help the reviewers

martyall · 2024-07-17T18:06:14Z

buildkite/src/Jobs/Lint/Fast.dhall

@@ -18,7 +18,6 @@ let commands =
      [ Cmd.run "./scripts/lint_codeowners.sh"
      , Cmd.run "./scripts/lint_rfcs.sh"
      , Cmd.run "make check-snarky-submodule"
-      , Cmd.run "./scripts/lint_preprocessor_deps.sh"


The only library which uses ppx_optcomp is now mina_node_config. It turns out that this script doesn't work when there is only one lib to check, and it also doesn't seem relevant anymore.

martyall · 2024-07-17T18:06:35Z

nix/checks.nix

@@ -20,13 +20,6 @@ inputs: pkgs: {
    touch $out
  '';
  # todo: ./scripts/check-snarky-submodule.sh # submodule issue
-  lint-preprocessor-deps = pkgs.runCommand "lint-preprocessor-deps" {


see note above about lint check script

martyall · 2024-07-17T18:07:54Z

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

@@ -1662,79 +1653,11 @@ let audit_type_shapes : Command.t =
         Core.printf "good shapes:\n\t%d\nbad shapes:\n\t%d\n%!" !good !bad ;
         if !bad > 0 then Core.exit 1 ) )

-[%%if force_updates]


force_updates is set to false in every profile, so this code is dead. It no longer compiled, rather than update it I just deleted it.

martyall · 2024-07-17T18:12:31Z

src/lib/block_time/block_time.ml

@@ -39,8 +37,6 @@ module Make_str (_ : Wire_types.Concrete) = struct
    let zero = UInt64.zero

    module Controller = struct
-      [%%if time_offsets]


time_offsets is set to true for every profile

martyall · 2024-07-17T18:13:49Z

src/lib/consensus/proof_of_stake.ml

@@ -1621,25 +1619,6 @@ module Make_str (A : Wire_types.Concrete) = struct
        end )
    end

-    [%%else]


martyall · 2024-07-17T18:21:33Z

src/lib/genesis_constants/genesis_constants.ml

-      [%%endif]
-
-      [%%inject "supercharged_coinbase_factor", supercharged_coinbase_factor]
+        match


implicitly in the config it seems like you have one of two disjoint cases:

scan_state_with_tps_goal && is_some scan_state_tps_goal_x10 && is_none scan_state_transaction_capacity_log_2

or

not scan_state_with_tps_goal && is_none scan_state_tps_goal_x10 && is_some scan_state_transaction_capacity_log_2

what if we remove scan_state_with_tps_goal, scan_state_tps_goal_x10 and retain only scan_state_transaction_capacity_log_2 ?

you probably could, but my goal here was to do the smallest transformation and keep any config values code paths that were being used. IMO it would be easier to simplify the config which is being used in a follow up -- i.e. taking a look at what is in node config and seeing if we still care about each field.

For example, there are some fields which only act as a flag to log a simple statement, and other kind of "do nothing" fields we can likely remove.

martyall · 2024-07-17T18:22:08Z

src/lib/genesis_constants/genesis_constants.ml

-
-      let fork = None
-
-      [%%else]


martyall · 2024-07-17T18:25:19Z

src/lib/mina_compile_config/mina_compile_config.ml

@@ -67,44 +55,14 @@ let max_event_elements = 100

 let max_action_elements = 100

-[%%inject "network_id", network]
-
-[%%ifndef zkapp_cmd_limit]


These kinds of conditional assignments are all pushed into the Node_config module

martyall · 2024-07-17T18:26:12Z

src/lib/mina_compile_config/mina_compile_config.mli

@@ -0,0 +1,49 @@
+val curve_size : int


I felt it was better to explicitly give the Mina_compile_config interface because there are several values in here which are hard coded and seem like they should be considered as configuration

martyall · 2024-07-17T18:29:01Z

src/lib/test_genesis_ledger/test_genesis_ledger.ml


  let directory = `Ephemeral

  let depth = Genesis_constants.Constraint_constants.compiled.ledger_depth
 end)
-
-[%%else]


martyall · 2024-07-17T18:30:49Z

!ci-build-me

martyall · 2024-07-17T18:39:26Z

!ci-build-me

georgeee

Approving this PR with acknowledgment that in follow-up PRs we'll get rid of a few unused options completely.

georgeee · 2024-07-23T17:53:37Z

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

-
-[%%else]
-
+(*NOTE A previous version of this function included compile time ppx that didn't compile, and was never


Let's remove ensure_testnet_id_still_good function and its only callsite

SanabriaRusso · 2024-07-23T18:26:20Z

Great job!

martyall and others added 20 commits July 15, 2024 19:04

added node_config package and mina_compile_config.mli file

9ab083e

Remove config.mlh dependency for mina_compile_config package

0daa4c3

remove dep genesis_constants dependency on config

4a69cfb

remove all preprocessor deps on config.mlh for app dir

9cc617e

remove for cache_lib

85cc13d

remove dep from src/lib/consensus

a671f44

add src/lib/crypto_params

95160ad

remove from src/lib/currency

66aedf0

remove from src/lib/data_hash_lib

dd2f7bd

remove from src/lib/fields_derivers_zkapps

2c367d8

remove from src/lib/genesis_ledger

f5f6e23

remove for src/lib/hash_prefix src/lib/key_cache

a3cc1fd

remove for src/lib/mina_base dirs

b765b99

removed for mina_commands, mina_generators, mina_graphql, mina_metrics

3932bd2

removed for mina_numbers and mina_state

0d99845

remove for wire_types, network_pool

7336b37

gen_values

1f26822

remove all other uses of inject for node config fields

e2db7d5

remove config.mlh and ppx-optcomp flags everywhere possible

fdc9491

Merge branch 'compatible' into martin/further-consolidate-mlh-files

717beb9

martyall changed the title ~~Martin/further consolidate mlh files~~ consolidate ppx-optcomp usage to single module Jul 17, 2024

martyall added 2 commits July 17, 2024 09:25

capture logic previously used in optcomp

592d313

the lint_preprocessor script is no longer working / relevant

7036165

martyall added 2 commits July 17, 2024 10:57

run formater

58d67ab

add undefs for things to work

a0e734b

more formatting

87e18c9

martyall commented Jul 17, 2024

View reviewed changes

fix mina_compile_config dune file

777f3e5

martyall marked this pull request as ready for review July 17, 2024 19:19

martyall requested review from a team, bkase, psteckler and mrmr1993 as code owners July 17, 2024 19:19

This was referenced Jul 23, 2024

Remove plugins support #15788

Closed

Remove snark key download #15789

Closed

georgeee approved these changes Jul 23, 2024

View reviewed changes

SanabriaRusso approved these changes Jul 23, 2024

View reviewed changes

mrmr1993 approved these changes Jul 24, 2024

View reviewed changes

mrmr1993 merged commit 4f31693 into compatible Jul 24, 2024
48 checks passed

mrmr1993 deleted the martin/further-consolidate-mlh-files branch July 24, 2024 16:30

This was referenced Aug 31, 2024

Replace ppx_optcomp #11618

Closed

modernize or remove ensure_testnet_id_still_good #14807

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

consolidate ppx-optcomp usage to single module #15837

consolidate ppx-optcomp usage to single module #15837

martyall commented Jul 17, 2024 •

edited

Loading

martyall commented Jul 17, 2024

martyall commented Jul 17, 2024

martyall commented Jul 17, 2024

martyall left a comment

martyall Jul 17, 2024

martyall Jul 17, 2024

martyall Jul 17, 2024

martyall Jul 17, 2024

martyall Jul 17, 2024

martyall Jul 17, 2024

georgeee Jul 17, 2024

martyall Jul 22, 2024

martyall Jul 17, 2024

martyall Jul 17, 2024

martyall Jul 17, 2024

martyall Jul 17, 2024

martyall commented Jul 17, 2024

martyall commented Jul 17, 2024

georgeee left a comment

georgeee Jul 23, 2024

SanabriaRusso commented Jul 23, 2024


		[%%else]

		(*NOTE A previous version of this function included compile time ppx that didn't compile, and was never

consolidate ppx-optcomp usage to single module #15837

consolidate ppx-optcomp usage to single module #15837

Conversation

martyall commented Jul 17, 2024 • edited Loading

High level

Changes

martyall commented Jul 17, 2024

martyall commented Jul 17, 2024

martyall commented Jul 17, 2024

martyall left a 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

martyall commented Jul 17, 2024

martyall commented Jul 17, 2024

georgeee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SanabriaRusso commented Jul 23, 2024

martyall commented Jul 17, 2024 •

edited

Loading