[Part 1 of 2] Constants loader and Config loader #16186

martyall · 2024-10-04T08:51:12Z

NOTE to reviewer

All of the important changes in this PR are in runtime_config.ml and genesis_ledger_helper.ml, start there. You will see that all other changes are switching an executable to use these new load methods, and it always looks the same.

Background

In previous work we removed all/as much direct access to *_constants.Compiled.t as possible throughout the codebase (with the exception being the For_unit_tests variant). We were able to push those all the way to the various executable entrypoints, but we were still using the compiled config directly

The problem

Invoking the compile config directly across many files doesn't provide an easy transition to json-only config
Mina_cli_entrypoint and a few other executables allow you to take a --config-file parameter and apply the json config to the compile time constants. Most executables do not allow this.
We have (at least) two different implicit kinds of config (i.e. you will not find these types anywhere, but they are implied):

(* Common in exectuables that we use as tests, benchmarks, etc *)
type constants = 
  { genesis_constants : Genesis_constants.t
  ; constraint_constants : Genesis_constants.Constraint_constants.t
  ; proof_level : Genesis_constants.Proof_level.t
  ; compile_config : Mina_compile_config.t

(* less commonly required, e.g. you find in it `Mina_cli_entrypoint`, `Archive.Archive_cli`, things that aren't tests or tools *)
type config = 
  { constants : constants
  ; daemon : Runtime_ledger.Daemon.t (* has overlap with compile_config *)
  ; ledger : Runtime_config.Ledger.t
  ; epoch_data : Runtime_config.Epoch_data.t
  }

Most executables only requires the constants config, so we don't want to require the above config type in places where no ledger is required. We need to separate these cases in the type system.

The Solution

Introduce a Runtime_config.Constants module to manage the constants type above and manage loading and applying the json config file for these values.
Introduce a Genesis_ledger_helper.Config_loader module to manage loading runtime config that is needed for the ledger.
Require that code in src/lib and Mina_cli_executable uses these new loaders exclusively.
Add a --config-file option to CLI contexts, or create this context it if it didn't exist before

martyall · 2024-10-04T09:01:27Z

!ci-nightly-me

martyall · 2024-10-04T14:40:07Z

!ci-nightly-me

martyall · 2024-10-04T16:24:49Z

all important tests passing on nightlies
https://buildkite.com/o-1-labs-2/mina-end-to-end-nightlies/builds/2666

src/lib/runtime_config/runtime_config.ml

src/lib/cli_lib/commands.ml

src/app/archive/lib/processor.ml

src/app/archive/cli/archive_cli.ml

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

georgeee · 2024-10-08T21:13:41Z

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

+           Runtime_config.Constants.compile_config conf
+         in
+         let fee =
+           Option.value ~default:compile_config.default_transaction_fee fee


This looks a bit odd to me: we're adding a config file parameter just to extract a single number.

I reviewed usages of default_transaction_fee in the codebase and it seems right to do the following:

Remove default_transaction_fee from node config, compile config and everywhere else

For CLI flag defaults (only place where the values is used) just specify manual default 0.25 (this value is in devnet and mainnet, and dev profile isn't really used for running CLI tools)

Or we could place 0.25 to unconfigurable constants

specify manual default 0.25

I am noticing that in practice you can get away with a lot less: https://minascan.io/mainnet/txs/pending-txs
IMO the default fee should be based on what's in the mempool, and neither the old solution, my solution, nor suggested solutions stand out as amazing.

Can I suggest that we just leave it? Loading a config file is not a super high barrier at this point, and we can always use "sensible defaults" as part of the json parser.

Or we could place 0.25 to unconfigurable constants

These are eventually configurable anyway

IMO the default fee should be based on what's in the mempool, and neither the old solution, my solution, nor suggested solutions stand out as amazing.

This is definitely for future.

Neither solution stands out as amazing, but hardcoding 0.25 as the default seems still better. It feels awkward to have a config file parameter to take only default fee for a command that has a fee argument as well. And no other command uses this default transaction fee value from the config.

So, I'm removing default transaction fee from all configs and hardcode the existing value.

martyall · 2024-10-09T04:20:58Z

!ci-build-me

martyall · 2024-10-09T04:21:06Z

!ci-nightly-me

martyall · 2024-10-09T04:21:46Z

nightly results: https://buildkite.com/o-1-labs-2/mina-end-to-end-nightlies/builds/2702

coveralls · 2024-10-09T16:26:38Z

coverage: 33.042% (-28.0%) from 61.035%
when pulling 48603df on martin/create-constants-loader-part-1
into 0b12932 on compatible.

…onstants-loader-part-1

Value is only used in two CLI commands to supply the default value for transaction creation. Value is always 0.25 MINA (in lightnet, devnet, mainnet). It makes no sense to supply this value from config (fee can be set directly, so it was simply hardcoded to preserve existing behavior.

dkijania · 2024-10-17T20:28:42Z

Remove default_transaction_fee from node config looks goood

georgeee · 2024-10-17T20:53:02Z

!ci-build-me

georgeee · 2024-10-17T20:53:08Z

!ci-nightly-me

georgeee · 2024-10-18T11:59:38Z

!ci-nightly-me

volhovm

OK on the crypto side (not much crypto). The mappend/combine function is the most tricky one in my previous experience to get right. Eventually this should be tested by time, it's a bit hard to be fully convinced mentally that everything is OK when you do a merger of dozens of fields. Cool!

volhovm · 2024-10-18T11:34:46Z

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

@@ -4,6 +4,10 @@ open Signature_lib
 open Mina_base
 open Mina_transaction

+(* TODO consider a better way of setting a default transaction fee than
+   a fixed compile-time value *)
+let default_transaction_fee = Currency.Fee.of_nanomina_int_exn 250000000


I assume this change was reviewed before by someone else (I see a previous discussion). Not crypto related.

Yeah, @dkijania reviewed this one specifically

volhovm · 2024-10-18T11:37:38Z

src/lib/runtime_config/runtime_config.ml

+      { Genesis_constants.protocol =
+          { k =
+              Option.value ~default:a.genesis_constants.protocol.k
+                Option.(b.genesis >>= fun g -> g.k)


Offtop: doesn't ocaml have fmap? Hmm... https://blog.janestreet.com/generic-mapping-and-folding-in-ocaml/

This is not fmap though. fmap f x is equivalent to x >>= return . f, wherease here we don't have return

But yes, it's simply map in OCaml parlance

volhovm · 2024-10-18T11:46:17Z

src/lib/runtime_config/runtime_config.ml

+
+  let compile_config t = t.compile_config
+
+  let combine (a : constants) (b : t) : constants =


I will not block this PR because of the writing style, but couldn't this at least be left and right? conf_a and conf_b even? Our codebase has too much non-descriptive names.

I'd go for combine_constants or mappend to make it more precise that this is an associative overriding operation. It is associative, right?

Here t and constants are different types, so no associativity.

As for name/docs: probably constants and conf would be better names, I'll make a short follow-up PR for the matter

volhovm · 2024-10-18T11:50:22Z

src/lib/runtime_config/runtime_config.ml

+
+  let compile_config t = t.compile_config
+
+  let combine (a : constants) (b : t) : constants =


Now, serious ocaml noob type of question: what is t in the function type (b:t)? I don't see any t defined in scope, I assume it's also constants... isn't it? Is it just an open polymorphic type of "anything that has fields that we access within combine"?.....

Type t is defined further above in the file, wasn't touched by the PR. Se line 1477

volhovm · 2024-10-18T11:51:15Z

src/lib/runtime_config/runtime_config.ml

+
+  val compile_config : constants -> Mina_compile_config.t
+
+  val magic_for_unit_tests : t -> constants


Can .... this have a documentation please? Or a more descriptive name?

georgeee · 2024-10-18T12:53:58Z

This is quite unexpected, but libp2p unit tests gets timed out on the nightly when run against this PR. This doesn't happen for the compatible.

This PR seemingly doesn't change anything that affects libp2p unit tests (only Go code and scripts could've affected that). So I'm not sure why does this happen.

georgeee · 2024-10-20T15:07:20Z

I was able to reproduce failure of libp2p unit tests on pure compatible. Nightly of compatible is suspiciously passing, but the point is that there is evidence that this PR is not responsible for the failure of libp2p unit tests job.

I'll make a separate PR to alter timeouts in libp2p unit tests to ensure tests pass.

build minimal set with constants and config loader

8451dc8

martyall changed the title ~~build minimal set with constants and config loader~~ Constans loader and Config loader Oct 4, 2024

martyall changed the title ~~Constans loader and Config loader~~ Constants loader and Config loader Oct 4, 2024

fix state_hash parser

6d10813

martyall marked this pull request as ready for review October 4, 2024 16:25

martyall requested review from a team as code owners October 4, 2024 16:25

martyall changed the title ~~Constants loader and Config loader~~ [Part 1 of 2] Constants loader and Config loader Oct 5, 2024

martyall mentioned this pull request Oct 7, 2024

[Part 2 of 2] Constants loader and Config loader #16187

Closed

martyall force-pushed the martin/create-constants-loader-part-1 branch from 16119d1 to 6d10813 Compare October 7, 2024 21:01

mergify bot mentioned this pull request Oct 7, 2024

Remove unconfigurable_constants module #16196

Merged

Base automatically changed from martin/unify-json-config-loading to compatible October 8, 2024 19:36

georgeee requested changes Oct 8, 2024

View reviewed changes

martyall added 2 commits October 8, 2024 15:31

Merge branch 'compatible' into martin/create-constants-loader-part-1

e390651

Address George's PR comments

48603df

martyall force-pushed the martin/create-constants-loader-part-1 branch from ec99eb0 to 48603df Compare October 9, 2024 04:20

martyall requested a review from georgeee October 9, 2024 05:25

georgeee added 2 commits October 17, 2024 19:19

Merge remote-tracking branch 'origin/compatible' into martin/create-c…

f4e1e6d

…onstants-loader-part-1

georgeee requested a review from a team as a code owner October 17, 2024 19:52

georgeee approved these changes Oct 17, 2024

View reviewed changes

Fix command_line_tests after merge

749ca36

dkijania approved these changes Oct 17, 2024

View reviewed changes

volhovm approved these changes Oct 18, 2024

View reviewed changes

georgeee merged commit e130ac7 into compatible Oct 20, 2024
46 checks passed

georgeee deleted the martin/create-constants-loader-part-1 branch October 20, 2024 15:07

georgeee mentioned this pull request Jan 9, 2025

removing compile time config load logging from cli #16403

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Part 1 of 2] Constants loader and Config loader #16186

[Part 1 of 2] Constants loader and Config loader #16186

martyall commented Oct 4, 2024 •

edited

Loading

martyall commented Oct 4, 2024

martyall commented Oct 4, 2024

martyall commented Oct 4, 2024

georgeee Oct 8, 2024

georgeee Oct 8, 2024

martyall Oct 9, 2024 •

edited

Loading

georgeee Oct 17, 2024

martyall commented Oct 9, 2024

martyall commented Oct 9, 2024

martyall commented Oct 9, 2024

coveralls commented Oct 9, 2024 •

edited

Loading

dkijania commented Oct 17, 2024

georgeee commented Oct 17, 2024

georgeee commented Oct 17, 2024

georgeee commented Oct 18, 2024

volhovm left a comment

volhovm Oct 18, 2024

georgeee Oct 18, 2024

volhovm Oct 18, 2024

georgeee Oct 18, 2024

volhovm Oct 18, 2024

volhovm Oct 18, 2024

georgeee Oct 18, 2024

volhovm Oct 18, 2024

georgeee Oct 18, 2024

volhovm Oct 18, 2024

georgeee Oct 18, 2024

georgeee commented Oct 18, 2024

georgeee commented Oct 20, 2024


		let compile_config t = t.compile_config

		let combine (a : constants) (b : t) : constants =


		val compile_config : constants -> Mina_compile_config.t

		val magic_for_unit_tests : t -> constants

[Part 1 of 2] Constants loader and Config loader #16186

[Part 1 of 2] Constants loader and Config loader #16186

Conversation

martyall commented Oct 4, 2024 • edited Loading

NOTE to reviewer

Background

The problem

The Solution

martyall commented Oct 4, 2024

martyall commented Oct 4, 2024

martyall commented Oct 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martyall Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martyall commented Oct 9, 2024

martyall commented Oct 9, 2024

martyall commented Oct 9, 2024

coveralls commented Oct 9, 2024 • edited Loading

dkijania commented Oct 17, 2024

georgeee commented Oct 17, 2024

georgeee commented Oct 17, 2024

georgeee commented Oct 18, 2024

volhovm 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

georgeee commented Oct 18, 2024

georgeee commented Oct 20, 2024

martyall commented Oct 4, 2024 •

edited

Loading

martyall Oct 9, 2024 •

edited

Loading

coveralls commented Oct 9, 2024 •

edited

Loading