search

0xPolygonMiden / miden-node

Reference implementation of the node for the Polygon Miden rollup
MIT License
53 stars 37 forks source link

chore: update installation instructions #436

Closed Mirko-von-Leipzig closed 3 months ago

Mirko-von-Leipzig commented 3 months ago

Closes #355

The various 

> #[!TIP]
> ...

annotations can be viewed by inspecting the whole file and not the diff which imo is a better way of reading this PR :)

Mirko-von-Leipzig commented 3 months ago

Looks good! Thank you! I left one comment for the future inline. Also, one general comment:

We currently have several ways (3?) of installing the node: (1) from crates.io, (2) from Docker image, and (3) from Debian package (a 4th potential way is by cloning and compiling the repo). I am thinking it would be good to say this upfront in the "Usage" section and then have dedicated sub-sections for each approach (and maybe common sub-sub sections for configuring the node - though, not sure to what extent they are common).

Basically, right now we dive right into installing from crates.io and only towards the end we mention other approaches. And it also is not super clear whether "Configuration" section, for example, is applicable only to the crates.io approach, or if, for example, it should also be used in the Debian package approach.

How do you feel about restricting our officially supported options here? Restrict the guide to formal packages for Debian, cargo and an official docker image uploaded to dockerhub?

Advanced users are welcome to compile from source, or to create their own docker image but they shouldn't really need a guide for that beyond the makefile and dockerfile.

Mirko-von-Leipzig commented 3 months ago

Hmm. I think our docker file is missing the init configuration file bit. It currently only copies the genesis data during the build process.

In addition, its missing the --locked so it can break when miden-base has breaking upstream changes.

bobbinth commented 3 months ago

How do you feel about restricting our officially supported options here? Restrict the guide to formal packages for Debian, cargo and an official docker image uploaded to dockerhub?

Advanced users are welcome to compile from source, or to create their own docker image but they shouldn't really need a guide for that beyond the makefile and dockerfile.

Yep - that's fine. We could even the number of options to 2 for now if bring up the docker option to the latest will take non-trivial amount of work.

Mirko-von-Leipzig commented 3 months ago

I've rewritten the instructions to only focus on the debian packages and cargo install - though the latter does include from source instructions as well.

I've removed docker (pending re-adding it as a separate section if we fix the build).

I've also removed mention of the separate process vs single process. What I'm thinking is (and please correct me if this is a bad idea):

Make the single process version very simple. Anything complicated ito configuration can be handled using the multi-process setup instead.

I'm thinking the simple version could just take in the rpc endpoint and a store directory. At this point it also doesn't really make sense to take in config file, so maybe we just have that subcommand take in --rpc-endpoint and --data-directory cli args. The store and block-producer endpoints don't have to public for a simple setup, so we can just localhost:0 them in the code instead.

The biggest change here is really just that store and block-producer endpoints can't be configured.