Refactor node from 4 to 1 binary #323

phklive · 2024-04-15T17:34:13Z

In this PR I propose the refactor of the node internal structure to enable/disable services using flags and reduce from 4 to 1 binary.

closes: #92 #322

polydez

Looks good to me, thank you! I've left a few inline comments. The main idea here is that we should move config parsing and service starting functionality to the node binary (leave only serve in the libraries). We also should union top-level configs in one single structure in the node binary with optional field for each corresponding service:

struct Config {
    rpc: Option<RpcConfig>,
    faucet: Option<FaucetConfig>,
   ....
}

polydez · 2024-04-16T12:33:17Z

bin/node/Cargo.toml

+miden-node-rpc = { path = "../../crates/rpc", version = "0.2" }
+miden-node-store = { path = "../../crates/store", version = "0.2" }
+miden-node-utils = { path = "../../crates/utils", version = "0.2" }
+miden-node-faucet = { path = "../../crates/faucet", version = "0.1" }


I would probably move these local dependencies to workspace's Cargo.toml and specify here just workspace = true. And we don't need to specify versions for them.

polydez · 2024-04-16T12:39:43Z

crates/block-producer/src/lib.rs

+// MAIN FUNCTION
+// =================================================================================================
+
+pub async fn start_block_producer(config_filepath: &Path) -> Result<()> {


I think, we should move such functions from libraries to binary and get rid of anyhow dependency in libraries. server::serve can return tonic::transport::Error. We also should process config file once in the node binary, not in each library.

Could you explain where in the binary you would add such functions?

We have 4 for now:

start_block_producer

start_rpc

start_store

start_faucet

Also to note that the faucet is not a Tonic server but an Actix web server.

Could you explain where in the binary you would add such functions?

I think, commands/start would be a good place for them.

Also to note that the faucet is not a Tonic server but an Actix web server.

Not a problem, it can return Actix's status messages. The idea is to keep flexible errors in the binary, but return error codes from the library.

Understood. Changed.

polydez · 2024-04-16T12:50:37Z

crates/faucet/src/config.rs

+
+/// Faucet top-level configuration.
+#[derive(Clone, Eq, PartialEq, Ord, PartialOrd, Hash, Debug, Deserialize)]
+pub struct FaucetTopLevelConfig {


This can be substituted by single node's config structure with optional elements for block-producer, store, faucet and rpc. Config of each service (library) stays in the library.

polydez · 2024-04-16T12:50:50Z

crates/faucet/src/config.rs

+
+/// Faucet top-level configuration.
+#[derive(Clone, Eq, PartialEq, Ord, PartialOrd, Hash, Debug, Deserialize)]
+pub struct FaucetTopLevelConfig {


This can be substituted by single node's config structure with optional elements for block-producer, store, faucet and rpc. Config struct of each service (library) stays in the library.

polydez · 2024-04-16T12:56:31Z

crates/block-producer/Cargo.toml

-path = "src/main.rs"
-bench = false
-doctest = false
-
 [features]
 tracing-forest = ["miden-node-utils/tracing-forest"]

 [dependencies]
 anyhow = { version = "1.0" }


We should get rid of anyhow in libraries.

Could you explain further why we should remove it in the libraries and not in the binary?

Using anyhow or eyre in libraries is considered as a bad practice, because it's hard to handle different errors by library users. But binary is a different case: we usually just print error message from the binary to the user and don't need to handle it, so anyhow is suitable there.

Understood.

polydez · 2024-04-16T12:57:27Z

crates/rpc/src/lib.rs

+// MAIN FUNCTION
+// =================================================================================================
+
+pub async fn start_rpc(config_filepath: &Path) -> Result<()> {


Move this functionality to the binary.

polydez · 2024-04-16T12:58:14Z

crates/store/src/lib.rs

+// MAIN FUNCTION
+// =================================================================================================
+
+pub async fn start_store(config_filepath: &Path) -> Result<()> {


We should move this functionality to the binary.

igamigo · 2024-04-16T14:20:29Z

crates/faucet/src/handlers.rs

+    let bytes = match created_notes.first() {
+        Some(note) => {
+            note_id = note.id();
+            note.to_bytes()
+        },


Have you tested this on the client? I think in order to import this you might need an InputNoteRecord.

Indeed, fixed.

phklive · 2024-04-17T13:47:11Z

Fixed all requested changes and comments.

I have used .expect quite extensively considering that multiple states are irrecoverable ( example if we load the configuration file incorrectly or if the server fails to run ) let me know what you think about it or if you have a better solution in mind.

Now the commands used to run the node are the following:

cargo run --release --features testing start node (or faucet, store, block-producer, rpc. You can also pass the --config flag to start to specify a specific config file usually located in bin/node/miden-node.toml)

polydez

Looks good, thanks! I've added several inline comments here.

polydez · 2024-04-17T14:15:41Z

Cargo.toml

@@ -25,6 +26,13 @@ exclude = [".github/"]
 [workspace.dependencies]
 miden-air = { version = "0.9", default-features = false }
 miden-lib = { version = "0.2"}
+miden-node-block-producer = { path = "crates/block-producer", version = "0.2" }


I think, it's not needed to specify version here.

polydez · 2024-04-17T14:18:32Z

bin/node/src/commands/start.rs

+
+pub async fn start_node(config: NodeConfig) -> Result<()> {
+    let mut join_set = JoinSet::new();
+    let db = Db::setup(config.store.clone().expect("Missing store configuration.")).await?;


I would rather not panic here, but raise error with message:

let db = Db::setup(config.store.clone().context("Missing store configuration.")?).await?;

polydez · 2024-04-17T14:22:11Z

bin/node/src/main.rs

+                    .await
+                },
+                StartCommand::Rpc => {
+                    start_rpc(config.rpc.expect("Missing rpc configuration.")).await


The same as before, I prefer to raise errors, not panics. But for binaries it's not a mandatory.

polydez · 2024-04-17T14:27:39Z

crates/block-producer/src/server/mod.rs

@@ -56,9 +55,11 @@ pub async fn serve(config: BlockProducerConfig) -> Result<()> {

    let addr = config
        .endpoint
-        .to_socket_addrs()?
+        .to_socket_addrs()
+        .expect("Failed to turn address into socket address.")


Please, avoid panics in the library code. Consider adding new error enumeration, if this function can return different error types.

igamigo

This LGTM in general. I'm a bit confused with some of the error handling changes on component code but once that's handled it should be good to merge. Another thing that we might want to look at in the future is to have top-level commands for each component instead of them being a subcommand of the start (ie, faucet start instead of start faucet; could allow for more component-specific commands in the future), but probably not many good reasons right now to do so.

igamigo · 2024-04-17T14:30:38Z

crates/faucet/Cargo.toml

+repository.workspace = true
+
+[features]
+# Makes `make-genesis` subcommand run faster. Is only suitable for testing.


Let's document (here and elsewhere) that this should be set to match the node's configuration as well

igamigo · 2024-04-17T14:41:46Z

crates/faucet/src/handlers.rs

+    // Instantiate transaction request
+    let tx_request = client
+        .lock()
+        .await
+        .build_transaction_request(tx_template)
+        .expect("Failed to build transaction request.");


I think ideally we would not panic on any of the faucet's code

How do you handle situations like missing config file or others? It is going to panic at one point right?

In general I think handling error is better overall, although I agree that missing config files would be an anomaly. However what I meant is that in this scenario specifically where we are handling HTTP requests, there should ideally be no Rust panics but rather the server should return the HTTP error code that best describes the situation. In this case the transaction template to transaction request conversion should work fine but it does rely on various validations, etc. that could fail and so the code should ideally not panic.

igamigo · 2024-04-17T14:42:17Z

crates/faucet/src/handlers.rs

+    // Serialize note into bytes
+    let bytes = match created_notes.first() {
+        Some(note) => {
+            note_id = note.id();
+            InputNoteRecord::from(note.clone()).to_bytes()
+        },
+        None => {
+            return Err(
+                FaucetError::InternalServerError("Failed to generate note.".to_string()).into()
+            )
+        },
+    };
+
+    info!("A new note has been created: {}", note_id);


This looks good, were you able to test this with the client?

Yes it works.

igamigo · 2024-04-17T14:46:49Z

crates/block-producer/src/server/mod.rs

        .next()
-        .ok_or(anyhow!("Couldn't resolve server address"))?;
+        .expect("Failed to resolve address.");


I'm not sure I agree with changing these errors into expect()s for these services

I agree, @polydez requested to remove the anyhow inside the libs. Looking into a better way to handle errors atm.

polydez

I'd like to improve error representation here:

Use thiserror for all error enumerations and write Display representation for each error variant.
Try to keep correct error chain without converting concrete types to String. Sometimes, String is also okay, but not for cases when we have a single error structure/enumeration which caused the error.

I've hilighted some of examples inline.

polydez · 2024-04-17T16:23:39Z

crates/utils/src/errors.rs

+    ApiInitialisationFailed(String),
+    ApiServeFailed(String),
+    AddressResolutionFailed(String),
+    EndpointToSocketFailed(String),


We can use io::Error here instead of String.

polydez · 2024-04-17T16:27:34Z

crates/utils/src/errors.rs

+    DatabaseConnectionFailed(String),
+}
+
+impl std::fmt::Display for ApiError {


Please use thiserror for ApiError and write display implementation using its macro. We use intensively thiserror, for example: https://github.com/0xPolygonMiden/miden-node/blob/next/block-producer/src/errors.rs#L16-L47

polydez · 2024-04-17T16:29:30Z

crates/utils/src/errors.rs

+pub enum ApiError {
+    ApiInitialisationFailed(String),
+    ApiServeFailed(String),
+    AddressResolutionFailed(String),


Currently this String is used only for error message about address resolution fail. It would be better to not to provide String here, but output error message for this variant in Display implementation.

The issue is still here. It would be better to just remove this String argument.

polydez · 2024-04-17T16:30:36Z

crates/utils/src/errors.rs

@@ -0,0 +1,14 @@
+#[derive(Debug)]
+pub enum ApiError {
+    ApiInitialisationFailed(String),


Here would be better to keep tonic::Error, don't need to convert it to string.

polydez · 2024-04-17T16:30:52Z

crates/utils/src/errors.rs

+#[derive(Debug)]
+pub enum ApiError {
+    ApiInitialisationFailed(String),
+    ApiServeFailed(String),


The same as above.

polydez · 2024-04-18T11:13:11Z

bin/node/src/commands/start.rs

+
+pub async fn start_node(config: NodeConfig) -> Result<()> {
+    let mut join_set = JoinSet::new();
+    let db = Db::setup(config.store.clone().context("Missing store configuration.")?).await?;


I would rather spawn start_* functions here. This would help us to reduce/simplify code.

polydez · 2024-04-18T11:16:06Z

crates/faucet/src/errors.rs

+
+#[derive(Debug, Error)]
+pub enum FaucetError {
+    /// Client has submitted a bad request


I think, these docs are excessive here.

polydez · 2024-04-18T11:27:54Z

crates/utils/src/errors.rs

+pub enum ApiError {
+    ApiInitialisationFailed(String),
+    ApiServeFailed(String),
+    AddressResolutionFailed(String),


The issue is still here. It would be better to just remove this String argument.

phklive · 2024-04-18T13:04:45Z

#323 (comment)

Which errors are you talking about? All of them or specifically this one?

polydez · 2024-04-18T13:14:48Z

#323 (comment)

Which errors are you talking about? All of them or specifically this one?

Just the pointed error variant. If you look at the code which raises this error, you will see that this string initialized with error message similar to what is implemented in Display for this enum variant:

miden-node/crates/store/src/server/mod.rs

Line 32 in ebc4c0d

.ok_or("Failed to resolve address.")

I'd probably set this string with endpoint.to_string() or Debug representation of endpoint in order to give information to the user about which endpoint was failed to convert.

polydez

Looks good, thanks! I've left a couple of nit comments.

polydez · 2024-04-18T13:23:08Z

bin/node/src/commands/start.rs

+    // block on all tasks
+    while let Some(res) = join_set.join_next().await {
+        // For now, if one of the components fails, crash the node
+        res.unwrap().unwrap();


I think, it would be better to raise error here:

res??;

polydez · 2024-04-18T13:30:46Z

crates/rpc/src/server/mod.rs

+        .map_err(ApiError::EndpointToSocketFailed)?
+        .next()
+        .ok_or(config.endpoint.to_string())
+        .map_err(|err| ApiError::AddressResolutionFailed(err))?;


I don't like such chained conversion here. We firstly convert None to Err("https://host:port") and then wrap it with ApiError::AddressResolutionFailed.

It would be better to just do:

let addr = config .endpoint .to_socket_addrs() .map_err(ApiError::EndpointToSocketFailed)? .next() .ok_or_else(|| ApiError::AddressResolutionFailed(config.endpoint.to_string()))?;

The second disadvantage of current solution is that we format endpoint as string even if no errors occurred.

polydez

Looks good to me, thank you!

bobbinth

Looks great! Thank you! I know this has already been merged, but I left a few small comments. Most of these can be separate issues which we can address in future PRs.

bobbinth · 2024-04-22T06:51:25Z

README.md

 To install for production use cases, run:

 ```sh
-cargo install --path node
+cargo install --path bin/node
 ```

 This will install the executable `miden-node` in your PATH, at `~/.cargo/bin/miden-node`.

 Otherwise, if only to try the node out for testing, run:

 ```sh
-cargo install --features testing --path node
+cargo install --features testing --path bin/node
 ```

 Currently, the only difference between the two is how long the `make-genesis` command will take to run (see next subsection).


Is cargo install --path bin/node still the right command to use? We can now install directly from crates.io and so I think this could be cargo install miden-node.

This would be the right command to use in case you would want to run the software using the cloned repo.

If you would like to use the binary directly installed from crates.io then yes indeed cargo install miden-node would be the right command.

I believe that both command could / should be documented for completeness ( having multiple ways to download / install the software )

Let me know what is your pov.

I think for cargo install downloading from crates.io is probably the main use case - but I'm not against also having a section which describes how to deploy a node by cloning the repo. In general, we should probably update the section about deploying the node because we now have multiple ways (installing from crates.io, via docker, via .deb package, by cloning the repo) and we should describe in a more comprehensive way than we do now.

bobbinth · 2024-04-22T06:53:44Z

Cargo.toml

+miden-node-block-producer = { path = "crates/block-producer" }
+miden-node-faucet = { path = "crates/faucet" }
+miden-node-proto = { path = "crates/proto" }
+miden-node-rpc = { path = "crates/rpc" }
+miden-node-store = { path = "crates/store" }
+miden-node-test-macro = { path = "crates/test-macro" }
+miden-node-utils = { path = "crates/utils" }


Should we not include version here as well? For example:

miden-node-block-producer = { path = "crates/block-producer", version = "0.2" }

I think the version is needed for crates published to crates.io (but I'm not 100% sure).

I believe that the version is inferred from the workspace cargo.toml that has the version number added.

bin/node/miden-node.toml

crates/rpc/src/config.rs

bin/node/miden-node.toml

* New working structure * Faucet available but note generation broken stalls on input_note * Fix faucet with new client note handling * Lints + added comments and small refactors * Removed anyhow from librairies + moved start fns to binary * Refactored config location * Removed top level config from libs + removed redundant tests * Added config.rs * Added start_faucet + FaucetConfig to binary * Fixed not serialisation issue * Modified cargo.toml's to include workspace deps * Added optionality to the configuration parameters * Added error handling / removed expect and panic * Improved error handling with thiserror * Fixed docker build and run * Update documentation after refactor * Updated start_node to use start_* * Removed irrelevant comments for errors * Made error printing more expressive using Endpoint * Added in store and block-producer * lint * Fixed nits

* fix: use `OutputNote::metadata` and remove `TODO` (#320) * refactor: store migrations into SQL files * formatting: reduce linebreaks * fix: update SQL file after rebasing * feat: implement database settings storage * feat: compare migrations by hashes * formatting: rustfmt * Improve CI & builds (#325) * Improved workflows * Added spacing * Refactor node from 4 to 1 binary (#323) * New working structure * Faucet available but note generation broken stalls on input_note * Fix faucet with new client note handling * Lints + added comments and small refactors * Removed anyhow from librairies + moved start fns to binary * Refactored config location * Removed top level config from libs + removed redundant tests * Added config.rs * Added start_faucet + FaucetConfig to binary * Fixed not serialisation issue * Modified cargo.toml's to include workspace deps * Added optionality to the configuration parameters * Added error handling / removed expect and panic * Improved error handling with thiserror * Fixed docker build and run * Update documentation after refactor * Updated start_node to use start_* * Removed irrelevant comments for errors * Made error printing more expressive using Endpoint * Added in store and block-producer * lint * Fixed nits * fix: remove table constant * feat: add check for changing of schema version outside migrations * feat: settings in human-readable format, improve logging * docs: address review comments * refactor: use `ANY` datatype in settings table with `STRICT` --------- Co-authored-by: Paul-Henry Kajfasz <[email protected]>

phklive added 2 commits April 15, 2024 13:30

New working structure

2f2ec01

Faucet available but note generation broken stalls on input_note

79cd495

phklive changed the base branch from main to next April 15, 2024 17:34

phklive added 2 commits April 16, 2024 12:20

Fix faucet with new client note handling

eca1942

Lints + added comments and small refactors

f5aa16f

phklive marked this pull request as ready for review April 16, 2024 12:00

phklive requested review from hackaugusto, igamigo, Dominik1999 and polydez April 16, 2024 12:01

polydez requested changes Apr 16, 2024

View reviewed changes

igamigo reviewed Apr 16, 2024

View reviewed changes

phklive added 8 commits April 17, 2024 15:38

Removed anyhow from librairies + moved start fns to binary

1c14eba

Refactored config location

ad6cd1c

Removed top level config from libs + removed redundant tests

49c623b

Added config.rs

d7363de

Added start_faucet + FaucetConfig to binary

d2ce921

Fixed not serialisation issue

b5ece28

Modified cargo.toml's to include workspace deps

2eb7bcf

Added optionality to the configuration parameters

ee2232d

phklive requested review from polydez and igamigo April 17, 2024 13:44

polydez requested changes Apr 17, 2024

View reviewed changes

igamigo approved these changes Apr 17, 2024

View reviewed changes

Added error handling / removed expect and panic

7a9c24c

phklive requested a review from polydez April 17, 2024 16:10

polydez requested changes Apr 17, 2024

View reviewed changes

Improved error handling with thiserror

ebc4c0d

phklive requested a review from polydez April 18, 2024 09:25

phklive added 2 commits April 18, 2024 13:52

Fixed docker build and run

23b64a2

Update documentation after refactor

f9890d3

phklive requested a review from bobbinth April 18, 2024 09:56

polydez requested changes Apr 18, 2024

View reviewed changes

phklive added 3 commits April 18, 2024 16:47

Merge next

e6009e3

Updated start_node to use start_*

71136c4

Removed irrelevant comments for errors

a0efa23

Made error printing more expressive using Endpoint

fc36970

phklive requested a review from polydez April 18, 2024 13:18

phklive added 2 commits April 18, 2024 17:19

Added in store and block-producer

1362196

lint

094181e

polydez requested changes Apr 18, 2024

View reviewed changes

Fixed nits

9a8aa83

phklive requested a review from polydez April 18, 2024 13:39

polydez approved these changes Apr 18, 2024

View reviewed changes

phklive merged commit 51f6a54 into next Apr 18, 2024
5 checks passed

phklive deleted the phklive-reduce-binaries branch April 18, 2024 14:31

bobbinth reviewed Apr 22, 2024

View reviewed changes

This was referenced Apr 25, 2024

[Feature]: Transform Faucet from lib to bin #333

Closed

[Feature]: Refactor configuration file for improved inference #334

Closed

bobbinth mentioned this pull request Apr 29, 2024

feature flags to disable/enable services #92

Closed

Refactor node from 4 to 1 binary #323

Refactor node from 4 to 1 binary #323

Conversation

phklive commented Apr 15, 2024

polydez left a comment • 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

polydez Apr 16, 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

phklive commented Apr 17, 2024

polydez 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

polydez Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igamigo 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

igamigo Apr 17, 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

polydez left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

polydez Apr 17, 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

Choose a reason for hiding this comment

phklive commented Apr 18, 2024

polydez commented Apr 18, 2024

polydez 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

polydez left a comment

Choose a reason for hiding this comment

bobbinth 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

polydez left a comment •

edited

Loading

polydez Apr 16, 2024 •

edited

Loading

polydez Apr 17, 2024 •

edited

Loading

igamigo Apr 17, 2024 •

edited

Loading

polydez left a comment •

edited

Loading

polydez Apr 17, 2024 •

edited

Loading